Fix buffer leak in WebSocket13FrameEncoder #12773
Conversation
|
Thanks! |
|
@chrisvest , thanks for the merge ! |
|
@pderop @chrisvest Hi guys. First of all @pderop thanks for this PR, but i think the fix should be done in another way. We need to override the |
|
Hi @amizurov, @chrisvest So, I understand your point, but there is one thing to take care about: indeed, if we override the WebSocket13FrameEncoder.encode() instead of encodeAndClose(), then the websocket frame to encode will be always closed, see MessageToMessageEncoder.encodeAndClose method: However, from the current WebSocket13FrameEncoder.encodeAndClose method, there is one case where the WebSocketFrame data message is appended to the "out" list of produced data, see here, so closing the frame in this case is likely to a problem (if I'm correct ??) In previous 4.x, when the frame data is appended to out list, then it was done like this, using data.retain() So, then In this case, if we go for overriding encode instead of encodeAndClose, we will have to probably do something like: I'll try to prepare a new PR, let's see what it will give ... |
|
@pderop Oh, i missed that we have not something like |
|
@amizurov , please check the other PR where data.split is used from the new Buffer API. |
Sure |
…ose (#12780) 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.
Motivation:
When encoding a CloseWebSocketFrame frame using the WebSocket13FrameEncoder.encode method, the binary data associated to the frame is not always disposed, causing a memory leak, and when using the leak detector, the following kind of logs can be observed:
The issue only happens when using netty5 latest snapshot version, not using netty 4.x version.
Modification:
Updated the WebSocket13FrameEncoder.encode in order to always close the frame data (when necessary).
Also, update the WebSocketServerProtocolHandlerTest.testExplicitCloseFrameSentWhenClientChannelClosed test
Result:
Using the proposed patch we don't see anymore leaks when encoding CloseWebSocketFrame frames using the WebSocket13FrameEncoder.