DeadLock with the event Notification about ads HOT 5 CLOSED

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:

1. does it fix your issue?
2. 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.