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

Properly handle chunked requests on WebsocketServer #1589

Merged
merged 4 commits into from
Aug 24, 2021

Conversation

fernandorubal
Copy link
Contributor

@fernandorubal fernandorubal commented Jul 13, 2021

Description

This PR intends to solve a bug when processing incomplete or chunked requests received through the Websocket endpoint.

Motivation and Context

This PR aims to provide a solution to a bug causing chunked requests sent to the Rskj WebSocket Endpoint to be treated as separate requests.

This bug was discovered when using the official Ethereum Go client to send a transaction. The geth client sends data > 1024 bytes in chunks. That caused the Web3WebSocketServer to read every chunk of 1024 bytes as a new request, failing in consequence to process it as a valid json.

Adding the WebsocketFrameAggregator handler to the pipeline provides a way to properly handle those chunked requests.

How Has This Been Tested?

This patch was tested using geth client with successful results

A new test was added in Web3WebSocketServerTest.java called smokeTestWithBigJson that uses the same logic that the original smokeTest, but generates a JSON message that exceeds 2048 bytes (OkHttpClient buffer) so it gets sent chunked to the WebSocket Server.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • Tests for the changes have been added (for bug fixes / features)
  • Requires Activation Code (Hard Fork)

@fedejinich
Copy link
Contributor

@fernandorubal can you provide some tests for this one?

@Vovchyk
Copy link
Contributor

Vovchyk commented Jul 15, 2021

pipeline: run

@fernandorubal
Copy link
Contributor Author

@fernandorubal can you provide some tests for this one?

A new test was added in Web3WebSocketServerTest.java called smokeTestWithBigJson that uses the same logic that the original smokeTest, but generates a JSON message that exceeds 2048 bytes (OkHttpClient buffer) so it gets sent chunked to the WebSocket Server.

059ea1c

@horca17
Copy link

horca17 commented Jul 26, 2021

Hello again, any news? Thanks.

@Vovchyk
Copy link
Contributor

Vovchyk commented Jul 29, 2021

pipeline: run

@jjtechuy
Copy link

pipeline: run

@sonarcloud
Copy link

sonarcloud bot commented Aug 24, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

64.3% 64.3% Coverage
0.0% 0.0% Duplication

@aeidelman aeidelman merged commit 002c8d6 into rsksmart:master Aug 24, 2021
@aeidelman aeidelman added this to the Iris v3.1.0 milestone Oct 6, 2021
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.

6 participants