ConsenSysMesh / cava

ConsenSys core libraries for Java & Kotlin
Apache License 2.0
84 stars 34 forks source link

Hand over one complete message to the ClientHandler implementation at a time so that it doesn't have to inspect the headers and calculate how many messages are present #201

Closed Happy0 closed 5 years ago

Happy0 commented 5 years ago

Sometimes multiple RPC responses arrive to the socket read buffer at once. At present, the implementation of ClientHandler and ServerHandler would have to inspect the header for the body size, then delimit the individual messages in this fashion - however, the decryption functionality has already done this so we might as well just hand over the individual message byte buffers from it so that the ClientHandler implementation can process one RPC response at a time.

At a high level, these can then be marshalled into the RPCMessage class, and then the body into an appropriate Java class / stream of classes. I will play around with this and make a separate pull request for it.

Happy0 commented 5 years ago

Hmm... My changes don't quite work, actually. For some reason, it seems to end up with each RPC header arriving as a message, followed by the body as a separate message (decoding into an RPC message when you join them up seems to work though)

 Bytes headerMessage = null;

    @Override
    public void receivedMessage(Bytes message) {
      System.out.println("We received a message?");
      System.out.println(new String(message.toArrayUnsafe(), UTF_8));

      if (headerMessage == null) {
        headerMessage = message;
        return;
      }

      Bytes headerAndBody = Bytes.concatenate(headerMessage, message);

      headerMessage = null;

      RPCMessage msg = new RPCMessage(headerAndBody);
      String body = msg.asString();

      System.out.println(body);

    }

On master, sometimes the first receivedMessage is just an RPC message on its own, but most of the time it's one or more full RPC header + bodies

I'll see if I can work out what the issue is / update this PR.

Happy0 commented 5 years ago

I've just realised from reading the protocol docs that this is because after decrypting a box stream header and body, you can end up with only part of the RPC message - so I guess I'll have to buffer these too :). I'll update this PR.

Split RPC messages

atoulme commented 5 years ago

Indeed messages are chunked on top if box streams. You will need to reassemble them.

On Mar 24, 2019, at 11:11, Gordon Martin notifications@github.com wrote:

I've just realised from reading the protocol docs that this is because after decrypting a box stream header and body, you can end up with only part of the RPC message - so I guess I'll have to buffer these too :). I'll update this PR.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

Happy0 commented 5 years ago

@atoulme - I've just updated this PR with the changes to buffer up partial RPC messages, and hand over one complete one at a time to the handler. It seems to work!

Let me know if you'd like any changes.

Note: I just force pushed to the branch because I changed the approach compared to the previous PR (where I'd assumed you get full RPC messages after decrypting.)

atoulme commented 5 years ago

Rebuilding. CircleCI has been capricious.

atoulme commented 5 years ago

Please look at my comment on body size reading, and if you can add a commit to fix that, I can merge this tomorrow. Cheers!

Happy0 commented 5 years ago

Thanks @atoulme - I wasn't sure if I'd got that right - it's the first time I've done that sort of thing - when I printed it out and compared it to the body size it seemed to print the right thing but .toInt() is certainly more readable. I'll push that change tonight / tomorrow when I'm back from my short holiday.

Happy0 commented 5 years ago

The change to use toInt() seems to work :). I've pushed the change