Reviewing merge request #5: Fix a deadlock.
From the commit message:
commit 1eb7c3df2f0bce3f842e121586fe350c941a937d
Author: Benjamin Berg <benjamin@sipsolutions.net>
Date: Tue Jul 5 16:00:43 2011 +0200
Do not keep the pending_queue locked while dispatching an item.
This fixes a possible deadlock. The case where this can happen is:
1. Application recieves a method call/signal
2. Method call/Signal is dispatched
3. From the dispatch handler, a method call is done
4. Another message came in in the meantime, which needs to be queued
In step for the queue is still locked by the dispatcher, and a deadlock
occurs. The fix is to release the queue.
The patch will only iterate over all messages once, to give the application
a chance to work on other things if a lot of messages are comming in.
- Requested by:
benzea - Status: Open
- Source repository: deadlock-fix:master
- Target repository: mainline:master
- Created at: July 5th, 2011 14:05
Commits that would be merged:
- 1f6f3e6
- 9ac62dc
Do not keep the pending_queue locked while dispatching an item.
1f6f3e6-9ac62dcComments
Pushed new version 1
Pushed new version 2
please reopen with new patch if problem still exists. To many differences meanwhile
The patch still applies cleanly.
Do you mean I am supposed to check whether this bug has been fixed in some other way?
Hm, ok; that is weird. The “Commits that would be merged” is wrong,
Correct is the following commit:
https://gitorious.org/dbus-cplusplus/deadlock-fix/commit/987d0f72a4decd6e64d0dc01612d2801688340f7/diffs/1eb7c3df2f0bce3f842e121586fe350c941a937d
Pushed new version 3
I just “updated” the request and since then it is displayed correctly again.
It would be nice to get this in, Google Chrome for example has workarounds for exactly this issue.
I added a remote branch with your merged changes:
https://gitorious.org/dbus-cplusplus/mainline/trees/merge-requests/5
But if I run my “Test1/TestApp” functional test I get a “NOK” for async calls. So I couldn’t put this into mainline without further discussion.
The test describes one of my async use cases. Please look into the test and explain why you think my test is not correct or why your code fixes another problem.
Hm, what seems to happen is that the sleep in the mainloop is executed. ie. the message sneaks in between the test whether the queue is empty and the sleep in the mainloop.
If you increase the timeout in the test, then it starts working.
ie. see:
// Only check for events, if there is nothing in the queue.
// XXX: It would be better to just prevent the poll from sleeping
// in this case.
Not sure how to correctly fix this right now. We somehow need to prevent the mainloop from sleeping if a new message has arrived in the meantime.
Pushed new version 4
OK. New version. The trouble was, that there was already a fix for the problem pushed earlier (see comment). However, I think my fix is better, as it does not copy the pending queue, but instead realeases the lock when possible.
Add a new comment:
Login or create an account to post a comment