Tie connection counter to the future/channel #1379

stepancheg · 2017-03-28T22:51:31Z

Instead of manually tracking where connection counter should be
released, just do it when future becomes done or connection becomes
closed.

I believe, code is safer against leaks this way.

slandelle · 2017-03-29T09:48:06Z

@stepancheg Great piece of work, thanks! Did it fix your issue? Could you please answer/address comments?

Instead of manually tracking where connection counter should be released, just do it when future becomes done or connection becomes closed. I believe, code is safer against leaks this way.

stepancheg · 2017-03-29T18:21:08Z

Did it fix your issue?

Don't know yet. Probably there was not issue in latest alpha. We used alpha1 before.

So, now we are running our program with alpha9.

Could you please answer/address comments?

Sorry, what comments?

BTW, I've rebased the patch against master.

stepancheg · 2017-03-29T18:42:29Z

No, seems like alpha9 is also leaking:

And funny thing is that on another server it seems to leak in different direction: number of permits + number of open connection exceeds configured max connections.

Now going to try patched version.

stepancheg · 2017-03-29T19:51:01Z

Updated patch to remove now unused partitionKey channel attribute.

stepancheg · 2017-03-29T19:52:35Z

Patched version seem not to leak counter, but it hard to say with 100% confidence after just an hour of running.

slandelle · 2017-03-29T09:31:59Z

client/src/main/java/org/asynchttpclient/netty/NettyResponseFuture.java

+            channelManager.releaseChannelLock(prevKey);
+            releasePartitionKeyLock();
+
+            throw new AssertionError("unreachable");


throw new IllegalStateException("Trying to acquire partition lock twice. Please report.")

throw new IllegalStateException("Trying to acquire partition lock concurrently. Please report."); "Twice" is not proper word, because function is allowed to be called twice.

slandelle · 2017-03-29T09:33:34Z

client/src/main/java/org/asynchttpclient/netty/NettyResponseFuture.java

@@ -56,6 +58,7 @@

    private final long start = unpreciseMillisTime();
    private final ChannelPoolPartitioning connectionPoolPartitioning;
+    private final ChannelManager channelManager;


I think it would make sense to encapsulate all the connection limits code into a dedicated object. ChannelManager is too much of a god object. WDYT?

Something like ConnectionSemaphore. Implementing it now.

slandelle · 2017-03-29T09:46:54Z

client/src/main/java/org/asynchttpclient/netty/request/NettyRequestSender.java

@@ -265,16 +265,10 @@ private Channel getOpenChannel(NettyResponseFuture<?> future, Request request, P

        Object partitionKey = future.getPartitionKey();

-        // we disable channelPreemption when performing next requests
-        final boolean acquireChannelLock = !performingNextRequest;


We're losing something here, but it think it's fine and it didn't make sense in the first place.

Yes, I overlooked it, and I agree, that it did something strange.

The original author's intent was to not require an extra semaphore when being redirected to another host. IMHO, too specific to be worth supporting.

The original author's intent was to not require an extra semaphore when being redirected to another host.

And that is actually wrong IMHO. For example, because of that behaviour you can run off your operating system file descriptor limit which could be configured to be almost equal to connection count limit.

stepancheg · 2017-03-29T23:51:12Z

Updated the patch with you requests.

slandelle · 2017-03-30T07:54:39Z

client/src/main/java/org/asynchttpclient/netty/channel/ChannelManager.java

-    private final boolean maxConnectionsPerHostEnabled;
-    private final ConcurrentHashMap<Object, NonBlockingSemaphore> freeChannelsPerHost = new ConcurrentHashMap<>();
+
+    private final ConnectionSemaphore connectionSemaphore;


This shouldn't be here :)

slandelle · 2017-03-30T07:54:48Z

client/src/main/java/org/asynchttpclient/netty/channel/ChannelManager.java


    private AsyncHttpClientHandler wsHandler;

    public ChannelManager(final AsyncHttpClientConfig config, Timer nettyTimer) {

        this.config = config;
+
+        connectionSemaphore = new ConnectionSemaphore(config);


same as above

slandelle · 2017-03-30T07:58:37Z

@stepancheg How did your tests went? Is the semaphore leak fixed? If so, I'm a bit frustrated that we don't know how the leak happened :) Are we sure that was just the semaphores leaking and not the connections too?
There's just some dead code to be removed, and then I can merge when you want.

Thanks a lot!

stepancheg · 2017-03-30T08:23:52Z

How did your tests went? Is the semaphore leak fixed?

Well, counters seems to be correct for last 8 hours. It means that either problem is fixed or system/network load changed.

And at the same time I see incorrect semaphore values on different servers (with ligher load) on alpha9, on these servers probably semaphore is released extra time.

I'm a bit frustrated that we don't know how the leak happened

I think there were 10s of places where leak could happen. Because it is hard to manually manager locks/ref counters. The idea of the patch is to write code in a way that you don't have to check and recheck every line for corner cases and exception safety.

Are we sure that was just the semaphores leaking and not the connections too?

I think so. We've checked previosly with lsof, there were expected number of open connections.

There's just some dead code to be removed, and then I can merge when you want.

OK, give me a couple of minutes.

stepancheg · 2017-03-30T08:34:23Z

Updated a patch, removed dead code.

slandelle · 2017-03-30T08:35:34Z

Great! I'll merge, try to backport on 2.0 too, and will release both 2.1-alpha and 2.0.

stepancheg · 2017-03-30T08:40:06Z

I'd avoid backporting to 2.0, it's too dangerous IMO. I'd prefer if you released it as alpha, and people (in particular, our project) could perform more tests with this change. And even if this patch fixes issues, some behaiour could be changed, and users could rely on old behavior.

slandelle · 2017-03-30T09:04:03Z

I've just released 2.1.0-alpha10, it should be up on central in 20 min.
I've backported your work in a branch: https://github.com/AsyncHttpClient/async-http-client/tree/2.0-fix1377
Once the fix is confirmed and stable, I still think we should release it. I don't think there's any behavior change.

Thanks a lot!

Motivation: #1379 introduced ConnectionSemaphore to deal with connection limits. When no such limit is set, which is the default, we could bypass all this code. Modification: Don’t create ConnectionSemaphore when no limit is set and bypass all related code when it’s null. Result: More simple code path and possibly better performance

Tie connection counter to the future/channel

ae13225

Instead of manually tracking where connection counter should be released, just do it when future becomes done or connection becomes closed. I believe, code is safer against leaks this way.

stepancheg force-pushed the channel-lock branch from 9bd9b95 to f5a4cb2 Compare March 29, 2017 18:19

stepancheg force-pushed the channel-lock branch from f5a4cb2 to f79efc3 Compare March 29, 2017 19:50

slandelle requested changes Mar 29, 2017

View reviewed changes

stepancheg force-pushed the channel-lock branch from f79efc3 to 103518c Compare March 29, 2017 23:49

slandelle requested changes Mar 30, 2017

View reviewed changes

stepancheg force-pushed the channel-lock branch from 103518c to ae13225 Compare March 30, 2017 08:33

slandelle added the Defect label Mar 30, 2017

slandelle merged commit f240845 into AsyncHttpClient:master Mar 30, 2017

slandelle added this to the 2.0.32 milestone Mar 30, 2017

stepancheg deleted the channel-lock branch March 30, 2017 08:50

slandelle mentioned this pull request Oct 3, 2017

Bypass ConnectionSemaphore when no connection limit is set #1467

Closed

Tie connection counter to the future/channel #1379

Tie connection counter to the future/channel #1379

Uh oh!

Conversation

stepancheg commented Mar 28, 2017

Uh oh!

slandelle commented Mar 29, 2017

Uh oh!

stepancheg commented Mar 29, 2017

Uh oh!

stepancheg commented Mar 29, 2017

Uh oh!

stepancheg commented Mar 29, 2017

Uh oh!

stepancheg commented Mar 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stepancheg commented Mar 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slandelle commented Mar 30, 2017

Uh oh!

stepancheg commented Mar 30, 2017

Uh oh!

stepancheg commented Mar 30, 2017

Uh oh!

slandelle commented Mar 30, 2017

Uh oh!

stepancheg commented Mar 30, 2017

Uh oh!

slandelle commented Mar 30, 2017

Uh oh!

Uh oh!