Fix thread deadlock issue in RPC handler. #211
Conversation
|
Also, I noticed the 'Cava is moving' notice on the README - will the recent scuttlebutt module commits also be moved across? (and this one?) |
|
I'll update this PR with something which doesn't block vertx at all (or only blocks on adding to a concurrent queue.) It feels counterintuitive to use locks in the middle of a vertx event loop :) |
|
Yes, Cava is moving. I've been pushing your patches to Apache Tuweni though. |
Happy0
force-pushed
the
deadlock_fix
branch
3 times, most recently
from
06eb6c7
to
d762d20
Compare
|
Thanks @atoulme - I'll make future pull requests after this one to Apache Tuweni instead :). This pull request is ready to review again. |
|
I also updated the pull request to make the I did this because I actually made this mistake of checking for errors in the |
…onse bodies as implementers may think they have to check and handle errors in RPCMessage, rather than handling the future completing exceptionally.
| this.bodyType = bodyType; | ||
| } | ||
|
|
||
| public Bytes getBody() { |
There was a problem hiding this comment.
we opted for short method names "body()" would be best. Same for "bodyType()"
| * We run each each update on this executor to update the request state synchronously, and to handle the underlying | ||
| * connection closing by failing the in progress requests and not accepting future requests | ||
| */ | ||
| private final ExecutorService executor = Executors.newSingleThreadExecutor(); |
There was a problem hiding this comment.
Is this executor closed? If vert.x is in use, you may be able to leverage it for this purpose.
| */ | ||
| package net.consensys.cava.scuttlebutt.rpc.mux.exceptions; | ||
|
|
||
| public class RPCRequestFailedException extends Exception { |
There was a problem hiding this comment.
Please make the class final. Also we try to extend RuntimeException if possible.
… dedicator executor. Fix some styling.
That's a better idea :). I've pushed a commit which removes the executor and uses this instead, and also the other changes that were requested. |
Changes which rely on ConsenSysMesh/cava#211
|
Going to merge this if it works for you and port it to Tuweni. |
There is a thread deadlock condition in
RPCHandlerwhich prevents any further messages being sent or received.Thread 1 wants to send a request via
makeAsyncRequestoropenStreamwhich are synchronized methods.This results in a call to
socket.writein vertx as part of creating the request or stream.Thread 2, the vertx event loop wants to call back the
receivedMessagehandler. This is a synchronized method inRPCHandler, so vertx blocks because thread 1 is waiting onsocket.write- however,socket.writecannot be serviced by the event loop since it's blocked.