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

WebSocket13FrameEncoder should overrides encode instead of encodeAndClose #12780

Conversation

pderop
Copy link
Contributor

@pderop pderop commented Sep 7, 2022

Motivation:

The #12773 PR has fixed a memory leak from the WebSocket13FrameEncoder.encodeAndClose method, where the websocket frame to be encoded was not always closed properly.

Now, @amizurov has suggested to do a better fix by just letting the WebSocket13FrameEncoder override the encode method instead of the encodeAndClose. It give an autoclose for the input argument and don't need to do this manually, behaviour will be similar with 4.1.

However, care must be taken when the frame is itself appended to the "out" list of produced data. In 4.x, this was done using "data.retain", with the new Buffer API, the data can be added to the "out" list using "data.split".

Modification:

The WebSocket13FrameEncoder is now overriding the encode method instead of the encodeAndClose, and the frame message is not closed anymore at all (this is done by the encodeAndClose method from the superclass).
And when the frame message data has to be appended into the "out" list of produced encoded data, then it is done using "data.split"

Result:

The WebSocket13FrameEncoder class is simpler and don't have to close the frame data parameter.

@amizurov
Copy link
Sponsor Contributor

amizurov commented Sep 7, 2022

@pderop Thanks a lot, also i'll try to port the ws-benchmark to check how much split is cost.

@chrisvest
Copy link
Contributor

Test failures didn't look related, so I restarted the build.

@normanmaurer normanmaurer added this to the 5.0.0.Alpha5 milestone Sep 8, 2022
@normanmaurer normanmaurer merged commit 22a6cb2 into netty:main Sep 8, 2022
@pderop
Copy link
Contributor Author

pderop commented Sep 8, 2022

@normanmaurer, @chrisvest, thanks for the review and merge !

@pderop pderop deleted the netty5-websocket13frameencoder-overrides-encode branch September 8, 2022 07:32
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

4 participants