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

Release ReferenceCounted when timer expires. #314

Closed
wants to merge 1 commit into from

Conversation

daschl
Copy link
Contributor

@daschl daschl commented Jan 15, 2015

This changeset subscribes when the "no subscriber" timer fires and
if the item is actually a reference counted item, completely releases
it and prevents it from leaking out of the pool.

This changeset subscribes when the "no subscriber" timer fires and
if the item is actually a reference counted item, completely releases
it and prevents it from leaking out of the pool.
@daschl
Copy link
Contributor Author

daschl commented Jan 15, 2015

fixes #311

@@ -138,7 +137,14 @@ private UnicastContentSubject(final State<T> state, long noSubscriptionTimeout,
*/
public boolean disposeIfNotSubscribed() {
if (state.casState(State.STATES.UNSUBSCRIBED, State.STATES.DISPOSED)) {
state.bufferedSubject.subscribe(Subscribers.empty()); // Drain all items so that ByteBuf gets released.
state.bufferedSubject.subscribe(new Action1<T>() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make it safer for future issues like this, I think it will be better to split the subject into an Observable and Observer with Observable always wired with auto-release. Something like:

        private final Observer<T> bufferedObserver;
        private final Observable<T> bufferedObservable;

        public State(Action0 onUnsubscribe) {
            this.onUnsubscribe = onUnsubscribe;
            final BufferUntilSubscriber<T> bufferedSubject = BufferUntilSubscriber.create();
            bufferedObservable = bufferedSubject.lift(new AutoReleaseByteBufOperator<T>());
            bufferedObserver = bufferedSubject;
        }

and then we do not store the Subject variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, I'll try to modify it

@NiteshKant
Copy link
Member

Thanks @daschl for the fix. Let me know what your views are on my comments.

@daschl
Copy link
Contributor Author

daschl commented Jan 15, 2015

Great! Will make the changes.

Btw, we also should unsubscribe from the timer when a subscrption comes along, right?

@NiteshKant
Copy link
Member

Btw, we also should unsubscribe from the timer when a subscrption comes along, right?

Yes, that will remove the redundant tick from the scheduler. Makes sense.

@NiteshKant
Copy link
Member

@daschl ping, are you working on this? I would like to fix this before I do a release for 0.4.5

@daschl
Copy link
Contributor Author

daschl commented Jan 26, 2015

@NiteshKant I can revisit it this week hopefully, but I'm a little dragged into something else. So if you want to pick it up and drag it over the finish line please go ahead :)

@NiteshKant
Copy link
Member

Yep, lemme do that. Thanks!

@NiteshKant
Copy link
Member

Closing this in favor of PR #320

@NiteshKant NiteshKant closed this Jan 28, 2015
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