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

Lowering max message size in upload worker #365

Merged

Conversation

mkwiatkowskisoldevelo
Copy link
Contributor

@mkwiatkowskisoldevelo mkwiatkowskisoldevelo commented Aug 2, 2022

We've discovered, that while using the embedded js client to run ndt7 tests, the upload speeds are very low for the connections at higher speeds (we were testing it with 4G and 10G). The reason is that the WebSocket connection is closed at the begging of the tests. The connection is closed by the server because its buffer is overflowing. This happens on the following browsers: Chrome, Edge, and Safari. Firefox is completing the upload test normally for the same set-up.

As a fix for this issue, we're proposing lowering the maxMessageSize on the upload worker on the JS client from 16MB to 8MB. This is also a value used in the other JS client: ndt7-js. This client could also replace the current one, if it's OK with you I could add it as a part of this PR. I see that there is a PR that introduces that client, but it is over 2 years old and there were some changes in the other repository recently.

Another way to stop the upload test from failing is to slightly increase MaxMessageSize on the server side, which is set to 16MB. Changing it to 18MB is enough for our testing.

It would help us greatly in using NDT if you could include this change or resolve this issue.

Thank you very much in advance.


This change is Reviewable

this should fix connection closed prematurely for upload test on chrome
@stephen-soltesz
Copy link
Contributor

stephen-soltesz commented Aug 3, 2022

@mkwiatkowskisoldevelo thank you for this change request. @robertodauria is currently on vacation through Aug 15th and has the most experience with performance tuning the server and js clients. He is the best to review your proposed changes.

@SaiedKazemi SaiedKazemi changed the base branch from master to main August 10, 2022 23:29
Copy link
Contributor

@robertodauria robertodauria left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

As far as I know the minimal client included in this repository is only used for e2e testing via nodejs, while ndt7-js is the recommended reference client for integrators. In ndt7-js I changed the maximum message size to 8MB some time ago to work around a different bug (only happening on Safari). It makes sense to also change it here, especially if it's causing problems on other browsers, too.

Reviewed 1 of 1 files at r1.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained

@robertodauria robertodauria merged commit a5e9530 into m-lab:main Aug 18, 2022
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.

3 participants