Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use an unpooled buffer for setup payload #816

Closed
wants to merge 1 commit into from

Conversation

Sm0keySa1m0n
Copy link

@Sm0keySa1m0n Sm0keySa1m0n commented Apr 25, 2021

This fixes data corruption issues when reusing event loops for multiple service calls.

A demo of the bug can be found here: https://gist.github.com/Sm0keySa1m0n/f3396a8aa3d5e093d24571ffd5da9c69

You'll notice the byte read from the setup payload is not correct in the second service call.
This was causing ClosedChannelExceptions on the client because the server was silently closing the connection after a StreamCorruptionException was thrown when trying to decode the connection setup payload. Therefore this PR will probably fix #743 and #697.

@segabriel
Copy link
Member

It's not reproducible by the provided gist on my local environment (the last master commit + macOS). Could you provide versions of scalecube, netty, and rsocket for the provided example and OS?
Also, it doesn't look like a solution. Need to clarify the reason and a possible spot where need to create an issue (netty, rsocket).

@Sm0keySa1m0n
Copy link
Author

I'm using Java 15, Windows 10 and these dependencies:

implementation group: 'io.scalecube', name: 'scalecube-transport-netty', version: '2.6.9'
implementation group: 'io.scalecube', name: 'scalecube-services-transport-rsocket', version: '2.10.18'
implementation group: 'io.scalecube', name: 'scalecube-services-discovery', version: '2.10.18' 
implementation group: 'io.scalecube', name: 'scalecube-services', version: '2.10.18'
implementation group: 'io.scalecube', name: 'scalecube-services-bytebuf-codec', version: '2.10.18'

implementation group: 'io.rsocket', name: 'rsocket-core', version: '1.1.0'
implementation group: 'io.rsocket', name: 'rsocket-transport-netty', version: '1.1.0'

@Sm0keySa1m0n
Copy link
Author

I'm going to assume it's an issue with rsocket version 1.1.0 because I've just noticed that ScaleCube is still on 1.0.4.

@Sm0keySa1m0n
Copy link
Author

I have updated my example to remove ScaleCube completely, it's now only using RSocket 1.1.0. https://gist.github.com/Sm0keySa1m0n/bb33abcb32adf0ae9c26d6f640420905

@segabriel
Copy link
Member

segabriel commented Apr 26, 2021

@Sm0keySa1m0n
Finally, when I tried to use the old version of the rsocket 1.1.0 I see the difference.
So my suggestion is to use the latest rsocket version 1.0.4 (https://github.com/rsocket/rsocket-java/releases, 1.0.4 > 1.1.0), due to it's compatible with the last reactor-core and netty versions that scalecube uses (https://projectreactor.io/docs, 2020.0.6). Or wait when the rsocket version 1.1.1 will be released.

@Sm0keySa1m0n
Copy link
Author

That's a strange versioning scheme... I was under the impression that 1.1.0 was newer than 1.0.4.

@Sm0keySa1m0n
Copy link
Author

I can confirm that switching to RSocket 1.0.4 fixes the problem, I'll leave this PR open just in case you want to keep it for supporting the potential use of 1.1.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate warnings due to java.nio.channels.ClosedChannelException from rsocket client
2 participants