GoogleCodeExporter
commented
9 years ago Ooops, sorry, wrong attachment. This is the correct file.Original comment by martin.g...@gmail.com on 30 Jan 2012 at 2:29
Attachments:
Closed GoogleCodeExporter closed 9 years ago
GoogleCodeExporter
commented
9 years ago Ooops, sorry, wrong attachment. This is the correct file.Original comment by martin.g...@gmail.com on 30 Jan 2012 at 2:29
Attachments:
GoogleCodeExporter
commented
9 years ago You shouldn't block inside the callbacks. Instead, block in a separate thread.
For example, you could supply the ResponseListener with a CountDownLatch and
then call await() on the latch in your thread. The ResponseListener would call
countDown() after the response has been received. At that point, your thread
can continue. You could also use org.ros.Holder for this.Original comment by damonkoh...@google.com on 31 Jan 2012 at 5:40
GoogleCodeExporter
commented
9 years ago Thanks for your quick reply! I've implemented your suggestion, but I believe it
doesn't solve the underlying problem, just hides the Exception.
To demonstrate this, I've updated the JUnit tests (see attachment). Now, there
are three implementations which do the same thing:
1. testNestedServiceCallUnthreaded() -- a simplified version of my original
test. serviceClient.call() is called directly from the callback thread. Fails
with the "dead lock" Exception shown above.
2. testNestedServiceCallWaitOnThread() -- implementation of your suggestion.
serviceClient.call() is called from a separate thread, then we use a
CountDownLatch to wait() for it to finish. Doesn't throw the exception.
3. testNestedServiceCallJoinThread() -- serviceClient.call() is called from a
separate thread, then the thread is immediately joined. Doesn't throw the
exception.
The point is that ALL THREE IMPLEMENTATIONS BLOCK THE CALLBACK THREAD. In all
three, wait() is called at one point or another:
impl. 1: the callback thread directly enters wait() in
DefaultChannelFuture.awaitUninterruptibly. This potential deadlock is detected.
impl. 2: the callback thread enters wait() in CountDownLatch. This potential
deadlock is hidden from Netty.
impl. 3: we don't even need to wait() for very long, because we immediately
join the worker thread. This potential deadlock is also hidden from Netty.
To sum up, whatever we do, the callback thread is blocked until the nested
service call returns. Whether we directly wait in the callback thread, or spawn
an intermediate thread and then wait on that one, shouldn't make a difference
deadlock-wise. And there is no way around it, because we need the result as a
return value.
In my opinion, there are two ways to go about this:
- Either the danger of deadlock is real, and we're using Netty incorrectly. In
that case, none of the three implementations solves the problem, just hides it.
- Or it can be guaranteed that this potential deadlock cannot happen here. Only
in that case the exception should be turned off using
DefaultChannelFuture.setUseDeadLockChecker().
I can't say which of the two is the case, but could you please reopen this
ticket?
Original comment by martin.g...@gmail.com on 2 Feb 2012 at 11:33
Attachments:
GoogleCodeExporter
commented
9 years ago I was actually going to reopen this anyway as I came to the same conclusion
myself. The problem is that the ServiceResponseBuilder has to be asynchronous
as well. Doing that will require the larger fix that ensures service calls are
answered in the order they're received. Right now there's a race condition that
could cause responses to be returned for the wrong request.Original comment by damonkoh...@google.com on 2 Feb 2012 at 3:50
GoogleCodeExporter
commented
9 years ago This issue was closed by revision 17d16f5b5b2e.Original comment by damonkoh...@google.com on 5 Apr 2012 at 9:45
Original issue reported on code.google.com by
martin.g...@gmail.comon 30 Jan 2012 at 2:27Attachments: