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

API Compatibility with AsyncWebSocketMessageBuffer and makeBuffer() #3

Merged
merged 1 commit into from
Jan 25, 2024

Conversation

mathieucarbou
Copy link
Owner

@mathieucarbou mathieucarbou commented Jan 25, 2024

This PR brings back API-compatibility to the original fork regarding the use of AsyncWebSocketMessageBuffer.

Note that this is not optimised since it wraps the shared buffer mechanism in this temporary class.

See discussion here about what has motivated this PR:

yubox-node-org#8 (comment)

@mathieucarbou
Copy link
Owner Author

FYI @vortigont

@mathieucarbou
Copy link
Owner Author

These changes have been tested in an app doing a lot of WS calls and I see no memory leak.

  • from 23:15: testing the changes
  • 23:42 and after is when I've switched back to the release

spikes causes by more intensive tests.

This is the used heap.

image

@mathieucarbou mathieucarbou merged commit 984284d into main Jan 25, 2024
4 checks passed
@mathieucarbou mathieucarbou deleted the AsyncWebSocketMessageBuffer branch January 25, 2024 23:04
@mathieucarbou mathieucarbou added this to the v2.5.0 milestone Jan 25, 2024
@vortigont
Copy link
Collaborator

Looks pretty nice indeed! Thanks!

@mathieucarbou
Copy link
Owner Author

Note that I didn't do any allocation verification on the pr : just a straightforward implementation.

I will add more safety checks today - i.e. to return nullptr when the vector cannot be allocated.

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.

2 participants