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

Remove ByteBuf auto-release #264

Closed
NiteshKant opened this issue Oct 30, 2014 · 8 comments
Closed

Remove ByteBuf auto-release #264

NiteshKant opened this issue Oct 30, 2014 · 8 comments
Assignees
Milestone

Comments

@NiteshKant
Copy link
Member

Today RxNetty tries to alleviate the load of managing ByteBuf lifecycle from the user and employs an auto-release strategy, post invoking onNext on the subscriber listening for data.

The above is convenient in normal scenarios however it becomes non-intuitive in scenarios where the processing of ByteBuf happens in a separate thread as described in the issue #203

A slightly convoluted example of the above is in case of proxies where the request payload from the server is written as a request to the client (calling the origin server) and response from the origin (from HttpClient) is written to the server response. In this case, the thread change is subtle which is the eventloop processing the connection between the client and the server.

The above issues makes me feel that we should completely eliminate this auto-release behavior and follow a simple principle The eventual consumer of the ByteBuf should release the ByteBuf

Doesn't it mean that every client has to worry about ByteBuf management?

Yes, thats true. However, it isn't as bad as it sounds!

Few facts
  • Whenever a ByteBuf is created, it has a ref count of 1. Everytime a ByteBuf is written to netty's channel, netty releases this ByteBuf. So, if the code is only shunting ByteBuf from one channel to another (proxy case), there is no special handling required.
  • After RxNetty provides serialization support, the de-serializers and serializers will release the ByteBuf after use. So, majority of the code should not be dealing with ByteBuf
  • For the users directly dealing with ByteBuf, the simple rule will be to unconditionally release it after they are done processing with the ByteBuf. There are no special cases of thinking about in which scenarios (same thread or different) one should release the ByteBuf and which they should not.
What about unconsumed ByteBuf s?

The above scenarios become a bit more complex when we have to deal with unconsumed ByteBufs. There are two distinct scenarios:

  • RxNetty read a ByteBuf from netty but there was no subscriber. In this case the ByteBuf is unconsumed and RxNetty will release such a ByteBuf
  • RxNetty passed a ByteBuf to a subscriber which cached the ByteBuf, which no code consumed. In this case, there should be an explicit dispose functionality available that should discard all unused ByteBuf. Much as this subject in zuul. In order to make this case easier for user, RxNetty will provide such a subject that can be used by users directly in scenarios where they need to cache.

Over all I feel, hiding the fact that netty expects users to do memory management of ByteBuf and making it easier for users, fills some gaps but exposes plenty more and adds confusion in the mind of users as to how to correctly use them. OTOH, if we are open about the fact that it is required to manage ByteBuf, it will create a much more intuitive system.

@benjchristensen
Copy link
Member

👍

@NiteshKant
Copy link
Member Author

Thanks @benjchristensen for the 👍
\cc @mattrjacobs @diptanu @g9yuayon

@daschl
Copy link
Contributor

daschl commented Oct 30, 2014

@NiteshKant how do you do this in RxNetty?

RxNetty read a ByteBuf from netty but there was no subscriber. In this case the ByteBuf is unconsumed and RxNetty will release such a ByteBuf

/cc @simonbasle

@NiteshKant
Copy link
Member Author

@NiteshKant how do you do this in RxNetty?
RxNetty read a ByteBuf from netty but there was no subscriber. In this case the ByteBuf is unconsumed and RxNetty will release such a ByteBuf

Thats a good question. RxNetty hands off ByteBuf to users in two ways:

  • Via ObservableConnection.getInput()
  • Via HttpContentHolder.getContent()

Both the above methods return a Subject.
HttpContentHolder.getContent() uses a UnicastContentSubject and after this change ObservableConnection.getInput() will also use the same.
UnicastContentSubject disposes the content stream if no one subscribes to it after a configurable timeout. The disposing of content stream includes releasing of ByteBuf

@simonbasle
Copy link

@NiteshKant I've looked at UnicastContentSubject when you linked it before, interesting expansion on the BufferUntilSubscriber subject ;)
I was under the impression that you also wanted to get rid of the timeout auto-release, but if that's not the case, awesome, it means that's a path worth exploring!

@benjchristensen
Copy link
Member

As @NiteshKant and I discuss this further there are two options:

  1. Never Auto-release

When ByteBuf is used the developer must always release at the end.

  1. Auto-Release with Synchronous/Asynchronous Difference

When the handler function is invoked synchronously, then auto-release would work. If the developer chooses to do something async before consuming the ByteBuf then they would be responsible for calling retain/release.

@NiteshKant
Copy link
Member Author

The changes in 0.5.x do not auto-release the ByteBuf. I intend to add an operator that can enable auto-release if added to the content stream.

@NiteshKant
Copy link
Member Author

Available in 0.5.x branch

BenWhitehead added a commit to BenWhitehead/mesos-rxjava that referenced this issue Apr 28, 2017
SinkSubscriber has had it's pipeline slightly changed to ensure that
when trying to read an error message response body that it is able to do
so. The `ByteBuf` from `response.getContent()` has it's ref-counter
automatically decremented when the response object leaves the netty
thread and results in the content being disposed. When SinkSubscriber
would then attempt to read the content to construct the error message
it would fail.

This change moves to logic of reading and mapping over the content
before we move to the compute thread to invoke the callback on the
`SinkOperation`.

See ReactiveX/RxNetty#264
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants