search

ela-compil / BACnet

BACnet protocol library for .NET :satellite:
https://www.nuget.org/packages/bacnet/
MIT License
215 stars 95 forks source link

BACnetClient is not thread safe #46

Open sylfab opened 5 years ago

sylfab commented 5 years ago

I have two threads in a same program, each thread is executed every seconds, they call ReadPropertyRequest function. First thread get a BinaryValue (0 or 1), second thread get an AnalogValue (between 20 and 30). Sometimes ReadPropertyRequest of BinaryValue return the AnalogValue! Can someone have a solution of my problem? PS: i call ReadPropertyRequest because COV does'nt seem to work for me

minzdrav commented 4 years ago

Hi Guys, I have same problem with https://github.com/ela-compil/BACnet/tree/v2-netstandard brunch. Could you please try to fix?

gralin commented 4 years ago

Unfortunatelly the client was not written to be thread safe. Try using a lock each time before you use it or write a wrapper that would do it for you for each atomic operation. Since BACnet operations have identifiers it should be possible to allow multiple requests to be sent out in the same time. I will leave this issue open to try to address it in the future.

minzdrav commented 4 years ago

Hi @gralin Thank you. Yes, it's working fine with locks. I have 1 thread per device and customer want to read properties and values from many devices. I'll try to create BACnet client per thread. Should work faster than locks.

MatthijsvdVeer commented 3 years ago

Hey @minzdrav, just wondering if that worked for you? Did you manage to run a client per thread without issue?

minzdrav commented 3 years ago

Hey @minzdrav, just wondering if that worked for you? Did you manage to run a client per thread without issue?

Hi @MatthijsvdVeer Yes.

YarekTyshchenko commented 1 year ago

Yup, can confirm the reads are not threadsafe. It has to do with InvokeId, a uint8 that gets added to every request like a correlation ID, it gets incremented every request, and if you send too many requests it can quickly roll over and cause problems.

Update: On further investigation the reads cause a thread to be blocked waiting for reply because of the use of ManualResetEvent, this is actually a much bigger problem, as the non thread-safe client can be wrapped in a mutex, and multiple clients started in parallel, however, the reads will quickly eat up worker threads.

Fortunately a solution could be to replace ManualResetEvent with a TaskCompletionSource which doesn't block a thread. The rest of the code seems to be non-blocking which is great.