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

Fix buffer handling for large WebSocket messages #88

Merged
merged 2 commits into from
Jan 21, 2022

Conversation

brnbs
Copy link
Contributor

@brnbs brnbs commented Jan 18, 2022

The original code seems to have issues when proxying large messages. Increasing the buffer size using WithBufferSize() helps, but it also significantly increases memory usage. This PR uses a dynamic-sized buffer using a MemoyStream.

AspNetCore.Proxy.Tests running fine and also tested with large JSON messages manually.

@twitchax
Copy link
Owner

LGTM, but would you mind adding a test with a large message?

@codecov-commenter
Copy link

Codecov Report

Merging #88 (6e9cf28) into master (572fc3e) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 6e9cf28 differs from pull request most recent head 5d1ef86. Consider uploading reports for the commit 5d1ef86 to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##            master       #88   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           12        12           
  Lines          660       669    +9     
=========================================
+ Hits           660       669    +9     
Impacted Files Coverage Δ
src/Core/Extensions/Ws.cs 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 572fc3e...5d1ef86. Read the comment docs.

@brnbs
Copy link
Contributor Author

brnbs commented Jan 19, 2022

Added a test, please check and create a new release when you have some time :)

@twitchax twitchax merged commit 804525f into twitchax:master Jan 21, 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.

None yet

3 participants