Refactor ReactiveSocketServerHandler to be not shareable. #94
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
** Problem **
There's a memory leak in `ReactiveSocketServerHandler`, it keep adding entries
in the `duplexConnections` map but never remove them.
** Solution **
Instead of having only one `ReactiveSocketServerHandler`, and manage resources
manually, we let Netty allocate one instance per Channel. Then no resource
management is necessary.
** Modifications **
I refactored all the uage of `ReactiveSocketServerHandler` whithout changing
the logic. I also got rid of the method
```
public void channelReadComplete(ChannelHandlerContext ctx) {
ctx.flush();
}
```
since we only use `writeAndFlush` in the DuplexConnection (we're sure there's
nothing to flush).
| ctx.name(), | ||
| LengthFieldBasedFrameDecoder.class.getName(), | ||
| new LengthFieldBasedFrameDecoder(Integer.MAX_VALUE >> 1, 0, BitUtil.SIZE_OF_INT, -1 * BitUtil.SIZE_OF_INT, 0)); | ||
| LengthFieldBasedFrameDecoder frameDecoder = |
Contributor
There was a problem hiding this comment.
Minor: It doesn't make much of a difference but we can move this to server initialization i.e. add these handlers upfront using ChannelInitializer instead of doing it on handlerAdded
Member
Author
There was a problem hiding this comment.
Good idea, we'll improve that later.
Contributor
|
👍 I will merge this change, my minor comment can be addressed later. |
ilayaperumalg
pushed a commit
to ilayaperumalg/rsocket-java
that referenced
this pull request
Dec 26, 2017
Fixes rsocket#94 (Protocol Status) As discussed in the issue, this change specifies a status of the protocol as draft. This also elaborates on the versioning scheme used for the protocol and how to specify the same in the SETUP frame.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
There's a memory leak in
ReactiveSocketServerHandler, it keep adding entriesin the
duplexConnectionsmap but never remove them.Solution
Instead of having only one
ReactiveSocketServerHandler, and manage resourcesmanually, we let Netty allocate one instance per Channel. Then no resource
management is necessary.
Modifications
I refactored all the uages of
ReactiveSocketServerHandlerwhithout changingthe logic. I also got rid of the method
since we only use
writeAndFlushin the DuplexConnection (we're sure there'snothing to flush).