Simplify semaphore logic in ChannelManager #1372
Conversation
| private Semaphore getFreeConnectionsForHost(Object partitionKey) { | ||
| return freeChannelsPerHost.computeIfAbsent(partitionKey, pk -> new Semaphore(config.getMaxConnectionsPerHost())); | ||
| private NonBlockingSemaphoreLike getFreeConnectionsForHost(Object partitionKey) { | ||
| if (maxConnectionsPerHostEnabled) { |
| @@ -139,18 +138,23 @@ public ChannelManager(final AsyncHttpClientConfig config, Timer nettyTimer) { | |||
| maxTotalConnectionsEnabled = config.getMaxConnections() > 0; | |||
| maxConnectionsPerHostEnabled = config.getMaxConnectionsPerHost() > 0; | |||
|
|
|||
| if (maxTotalConnectionsEnabled) { | |||
| @@ -0,0 +1,35 @@ | |||
| package org.asynchttpclient.netty.channel; | |||
There was a problem hiding this comment.
Please add package header, granting copyright to "The AsyncHttpClient" project, just like in ChannelManager but with inception year 2017.
| @@ -0,0 +1,21 @@ | |||
| package org.asynchttpclient.netty.channel; | |||
There was a problem hiding this comment.
Please add missing file header
| @@ -0,0 +1,12 @@ | |||
| package org.asynchttpclient.netty.channel; | |||
There was a problem hiding this comment.
Please add missing file header
| @@ -0,0 +1,63 @@ | |||
| package org.asynchttpclient.netty.channel; | |||
There was a problem hiding this comment.
Please add missing file header
| * | ||
| * @author Stepan Koltsov | ||
| */ | ||
| enum NonBlockingSemaphoreInfinite implements NonBlockingSemaphoreLike { |
There was a problem hiding this comment.
NoopNonBlockingSemaphore?
There was a problem hiding this comment.
No-op does not make it clear that tryAcquire returns true. Infinite make it look like a semaphore with infinite number of permits, so tryAcquire always returns true.
I put Infinite part at the end of the class name so classes were together in the file list in the IDE.
So I think names are good, but I can change both name and placement if you still think it is better.
| * @author Stepan Koltsov | ||
| */ | ||
| enum NonBlockingSemaphoreInfinite implements NonBlockingSemaphoreLike { | ||
| I; |
|
@stepancheg Thanks! Could you please address comments? |
* avoid using `java.util.concurrent.Semaphore` which is blocking, and its blocking part is not used, which is confusing. * Use `NonBlockingSemaphoreInfinite` to avoid nulls and ifs when connection count is not limited.
|
Addressed all the issues except name of semaphore class, see comments above. |
|
Great job, thanks! |
* avoid using `java.util.concurrent.Semaphore` which is blocking, and its blocking part is not used, which is confusing. * Use `NonBlockingSemaphoreInfinite` to avoid nulls and ifs when connection count is not limited.
* avoid using `java.util.concurrent.Semaphore` which is blocking, and its blocking part is not used, which is confusing. * Use `NonBlockingSemaphoreInfinite` to avoid nulls and ifs when connection count is not limited.
java.util.concurrent.Semaphorewhich is blocking,and its blocking part is not used, which is confusing.
NonBlockingSemaphoreInfiniteto avoid nulls and ifswhen connection count is not limited.