Fix thread deadlock issue in RPC handler.

ConsenSysMesh / cava

ConsenSys core libraries for Java & Kotlin

Apache License 2.0

84 stars 34 forks source link

Fix thread deadlock issue in RPC handler. #211

Closed Happy0 closed 5 years ago

Happy0 commented 5 years ago

There is a thread deadlock condition in RPCHandler which prevents any further messages being sent or received.

Thread 1 wants to send a request via makeAsyncRequest or openStream which are synchronized methods.
This results in a call to socket.write in vertx as part of creating the request or stream.
Thread 2, the vertx event loop wants to call back the receivedMessage handler. This is a synchronized method in RPCHandler , so vertx blocks because thread 1 is waiting on socket.write - however, socket.write cannot be serviced by the event loop since it's blocked.

Happy0 commented 5 years ago

Also, I noticed the 'Cava is moving' notice on the README - will the recent scuttlebutt module commits also be moved across? (and this one?)

Happy0 commented 5 years ago

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 :)

atoulme commented 5 years ago

Yes, Cava is moving. I've been pushing your patches to Apache Tuweni though.

Happy0 commented 5 years ago

Thanks @atoulme - I'll make future pull requests after this one to Apache Tuweni instead :). This pull request is ready to review again.

Happy0 commented 5 years ago

I also updated the pull request to make the RPCHandler methods return a type which makes it clear that the result of the future is a successful response body, while any errors mean the future is completed exceptionally. Unlike RPCMessage, it doesn't have the header fields which aren't relevant in the context the result is being handled.

I did this because I actually made this mistake of checking for errors in the RPCMessage in the library I'm consuming this from, so it was unclear even to me as the implementer of RPCHandler.

Happy0 commented 5 years ago

Is this executor closed? If vert.x is in use, you may be able to leverage it for this purpose.

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.

atoulme commented 5 years ago

Going to merge this if it works for you and port it to Tuweni.