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

PooledConnection.close must be subscribed to be executed #607

Merged
merged 2 commits into from May 27, 2018

Conversation

Projects
None yet
5 participants
@JonasHallFnx
Contributor

JonasHallFnx commented May 23, 2018

In RxNetty 0.5.x the PooledConnection will just return a releaseObservable, as opposed to 0.4.x. The returned Observable must be subscribed in order for the connection to be released. There is a leak of connections in case the subscriber has unsubscribed when the connection is to be emitted.

@@ -138,7 +140,13 @@ private void newConnectionReuseEvent(Channel channel, final ConnectionReuseEvent
subscriber.onNext(event.getPooledConnection());
checkEagerSubscriptionIfConfigured(channel);
} else {
event.getPooledConnection().close(false); // If pooled connection not sent to the subscriber, release to the pool.
// If pooled connection not sent to the subscriber, release to the pool.
event.getPooledConnection().close(false).subscribe(Actions.empty(), new Action1<Throwable>() {

This comment has been minimized.

@jepp3

jepp3 May 23, 2018

Can be replaced with lamda style action instead?

This comment has been minimized.

@jepp3

jepp3 May 23, 2018

(throwable -> { logger.error("Error closing connection.", throwable); })

This comment has been minimized.

@JonasHallFnx

JonasHallFnx May 23, 2018

Contributor

No can do. Source level is set to 1.7, lambda not allowed.

@jepp3

jepp3 approved these changes May 23, 2018

@jamesgorman2

LGTM

@jamesgorman2 jamesgorman2 merged commit b5a747b into ReactiveX:0.5.x May 27, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jamesgorman2

This comment has been minimized.

Collaborator

jamesgorman2 commented May 27, 2018

@JonasHallFnx this is in v0.5.3-rc1

I'll be able to have another look at #606 this week too, and if it's all good we'll get that into -rc2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment