|
You have serialized calls to HASPClass.Query, and serialized calls to Tester.function1 and Tester.function2. If Tester is the only class using HASPClass, this should be thread-safe as far as calls to HASPClass.Query() are concerned.
My problem with your code is that function1 and function2 each use TWO calls to HASPClass.Query to perform their work. If you ever have a second class calling HASPClass.Query, it is entirely possiblethat this code will no longer be thread-safe:
class Class2
{
static readonly object m_lock = new object();
public static function3()
{
lock(m_lock);
HASPClass.Query();
HASPClass.Query()
}
}
In this case, it is quite possible that a call to Tester.function1 will intervene, corrupting the data read by Class2.function3.
You should redesign HASPClass so that the double call to Query() is itself an atomic operation.
If you have an important point to make, don't try to be subtle or clever. Use a pile driver. Hit the point once. Then come back and hit it again. Then hit it a third time - a tremendous whack.
--Winston Churchill
|
|
|
|
|
Thanks a lot but currently only function1 and function2 consume HASP class.
No other classes call HASP class functions.
What you say in this case?
Thank you.
|
|
|
|
|
Assuming that you use HASPClass only as described, your code is thread-safe.
If you have an important point to make, don't try to be subtle or clever. Use a pile driver. Hit the point once. Then come back and hit it again. Then hit it a third time - a tremendous whack.
--Winston Churchill
|
|
|
|
|
|
Dear Daniel the only question remaining I have is that other class may call OpenPort method (or similar ClosePort - which has similar lock as OpenPort inside with same lock object). Please see the updated question.
Does this change anything?
modified 8-Feb-16 6:19am.
|
|
|
|
|
For a given port, OpenPort and ClosePort should be called only once. Calling OpenPort twice (with no intervening ClosePort) is an error.
One way to handle this would be to create a separate class which calls OpenPort in its constructor and ClosePort in its destructor. This would guarantee that each function is called only once. It is still your responsibility to ensure that only one instance of the class is created at any one time.
Search for the Singleton pattern for more details on how to do this.
If you have an important point to make, don't try to be subtle or clever. Use a pile driver. Hit the point once. Then come back and hit it again. Then hit it a third time - a tremendous whack.
--Winston Churchill
|
|
|
|
|
As you can see from my HASP class calling OpenPort twice is not possible because of m_isOpen flag. (please see updated question).
|
|
|
|
|
Not the answer to your question but some comments:
Because a serial port can be opened only once, there is no need to lock inside your OpenPort function. If the port is already opened, SerialPort::Open() will throw an exception.
There is also no need to have the m_isOpen member variable. Just use SerialPort::IsOpen() instead.
Regarding the locking question:
If you want to ensure that a series of transfers is not interrupted by another thread, you must lock like shown in your Tester class.
But then there is no need to lock also inside the Query() function. So you can use the locking object of your serial class instead by providing public access.
|
|
|
|
|
Thanks a lot foe your feedback but I didn't get your last sentence 
But then there is no need to lock also inside the Query() function. So you can use the locking object of your serial class instead by providing public access.
You meant locking object of my Tester class?
|
|
|
|
|
Yes. You can provide a function to return that object and pass it to lock().
|
|
|
|
|
didn't get you again sorry
|
|
|
|
|
In your Tester class lock using the object of your serial class (HASPClass::m_locker) and remove the locking inside your Query() function.
|
|
|
|
|
Thanks for suggested improvement. (PS in your case I'd need to make HASPClass lock public?)
Otherwise my case is also correct right?
|
|
|
|
|
It must be public then.
Your case is correct if Tester is the only class using the serial class (as answered by Daniel).
But if you have another class that uses the serial communication from within another thread too, it would be not safe. The general rule is: All locking should use the same object.
|
|
|
|
|
Other Class may ONLY call OpenPort (or Similar ClosePort method - which has similar lock inside as OpenPort) method.
Otherwise indeed only tester uses that.
What you say now?
|
|
|
|
|
Member 12061600 wrote: What you say now?
Nothing else than I had before: Then it would be safe.
But:
Using it that way (a locking object in the Tester class) is bad style. It would not work anymore if you (or even worse anyone else) decide later to use it from within another class.
Again:
There is no need to lock upon opening (and closing). The exclusive behaviour of serial ports makes it unnecessary. If you want to open the port from different code parts, you must handle the execptions (they should be handled anyway). Using locking there does not prevent the exception. It may just delay it.
Conclusion:
Your current code is actually thread safe. But there are unnecessary operations, missing error checks upon opening, and it is prone to be not thread safe when changed.
|
|
|
|
|
"Using locking there does not prevent the exception. It may just delay it."
It prevents because if port is already opened I don't reopen it.
PS did you see the updated version of question? (with open and close calls). Thanks a lot!
PPS I understood your conclusion too, thanks.
|
|
|
|
|
Member 12061600 wrote: It prevents because if port is already opened I don't reopen it.
OK. You are right because you are checking the open state first. But it just returns without any error code or message.
But there may be other reasons that opening fails which should be handled (not existing, already opened by another application or another instance of your application).
So when handling the exception, there is no need for locking.
|
|
|
|
|
So we agree that although my approach maybe redundant at places
at least in its current form it should work (including the Open and Close methods being called like in updated question)
Thanks a lot for help
|
|
|
|
|
I have slightly updated my question with how open port and close port methods could be also called from other methods. Please give feedback on that if it is Okay.
|
|
|
|
|
See my above post. No need to lock when closing and (as noted in my initial answer) use SerialPort::IsOpen() instead of your own member variable.
|
|
|
|
|
I still prefer to have lock in OpenPort to avoid that exception you mention
|
|
|
|
|
Locking protects only against simultaneous calls. Because serial ports are exclusive, they are usually opened only once from within the main thread. When doing so, you would never have the situation of simultaneous calls from within your app. But you may have other open errors that should be handled.
|
|
|
|
|
To be honest, if I were doing this, I'd look at using a different architecture altogether because you're mixing a lot of responsibilities in your code here. What I would be tempted to go with is to have your API exposed so that you have separate writer and reader implementations, and the actual management of the port operation would be the responsibility of a completely separate class altogether. As I tend to compose objects together with IoC, I would constrain the port functionality to be container controlled so that all classes get the same instance and I would put the checking of the port along with its creation in there. I would also hide this as an internal implementation detail so you would have this as an internal interface with an internal implementation. That way, your client code only sees the separate reader and writer interfaces. Something like this internally:
internal interface IHaspPort
{
SerialPort Port { get; }
}
internal class HaspPort : IHaspPort
{
private readonly object SyncLock = new object();
private SerialPort port;
public SerialPort Port
{
get
{
if (port == null)
{
lock(SyncLock)
{
if (port == null)
{
}
}
}
return port;
}
}
}Then, you simply inject this instance into your reader and writer classes and let it take care of itself.
This space for rent
|
|
|
|
|
thanks but I am slightly beginner with .NET and I'd rather stick with my approach.
You can have a look I slightly updated question, if you have opinion - whether my scenario is OK now I'd appreciate. (I added open and close calls)
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
