Comments (5)
Hi Satheesh,
your code snippet was very hard to read. I added two more "`" to your comment to increase readability.
I am not sure, if it is a good idea to change the map of notifications into a map of shared pointers. I have to think more about that and have to run some tests before I can patch that.
Unfortunately I am out of office until 8th of May. But you could run your own patched version until then.
In my opinion the Callback function called by notification.Notify() should be lightweight such as an interrupt service routine. So I would try to avoid dependencies such as Mutex A in them. Is that possible for your application?
Could you provide some example code to trigger your deadlock?
Regards,
Patrick
from ads.
Hi Patrick,
Your proposal of handling these callbacks as if the are service routines is rather valid. Normally for ISR routines it is not allowed to introduce context switches in the callbacks. So in other words it is not allowed to require a lock.
ISR callbacks have one big difference, they are executed in the ISR context. This has the advantage that the cpu is "paused" and no other threads can interfere with the current ISR. The ADS library is using a worker thread for notifying all the clients. So a user of ADS knows one thing for sure, the callback function is not executed on the thread where the subscription was done. So when you want to use the data in another thread than the callback (as what you suggest by using it as an ISR) will require locking or synchronising data via lock free queues. Both solutions have there advantages and disadvantages for the client.
I don't think it has added value to keep the lock while calling the callback function. As the lock is only guarding the notifications list. And as Sateesh describes it can even cause problems for the clients.
So by changing the ownership of the Notification object to a shared_ptr enables to lock only the list find. As the list might be changed after the Notification object is found. So with the shared_ptr you can make sure that there are no race conditions while removing or adding a notification while calling the Callback.
Regards,
Roel Blomjous
from ads.
Hi Roel,
thanks for your detailed comment. You are absolutely right it, makes no sense to keep the lock while calling the callback.
I prepared a patch 0048238 on branch dev-lockfree-callbacks.
It is only compile tested by me, as I have no access to any test environment until 8th of May.
However, Satheesh and other please give it a try and let me know two things:
- does it fix your issue?
- does it bring any regressions or performance drawbacks?
I will test and merge this patch, when I am back in the office in May.
Regards, Patrick
from ads.
Hi Patrick,
I have verified the patch 0048238. I am not seeing the deadlock issue with this patch, but i am still working on the regression and other performance testing.
I will update you soon with the result.
Thanks,
Satheesh
from ads.
Thanks for testing, Satheesh. My tests were successful, too. So I merged to master and pushed it online.
The commit hash changed, but the patch should match exactly.
During my vacation, we had some minor changes on our internal repository, which I didn't want to confuse.
Regards, Patrick
from ads.
Related Issues (20)
- Two application running on the same machine that read a variable from the same plc concurrently HOT 8
- Use std::function for callback HOT 1
- ADS-sum command: Read or Write a list of variables with one single ADS-command HOT 1
- impossible to compile under ubuntu 20.04.5 HOT 1
- CMake Error at CMakeLists.txt:40 (install): install TARGETS given target "ads" which does not exist in this directory. HOT 3
- Unreachable PLC Blocks All Other Operations During Timeout Period HOT 2
- Reading route using `adstool`
- Will beckhoff officially launch the golang version of the ADS API HOT 2
- SymbolAccess for Bool HOT 5
- ADS Server HOT 1
- Async WriteReqEx HOT 5
- AdsNotification cycle time deviation HOT 2
- RGB Image over ADS HOT 1
- Port: 30000 already in used as 0x7ffe55930ed0 terminate called after throwing an instance of 'AdsException'
- Passing IP from terminal
- ADS Connection Error With Docker Container HOT 1
- CMake build failing HOT 2
- ADS READ for TwinCAT Motion
- Info: connection closed by remote in sockets.cpp HOT 2
- twincat3 - ubuntu error code 6 HOT 2
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from ads.