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

Added BlockingConnectionSemaphoreFactory #1586

Merged
merged 5 commits into from
Dec 6, 2018

Conversation

maltalex
Copy link
Contributor

In response to #1527 here's a simple implementation of a blocking ConnectionSemaphoreFactory using plain old Java semaphores.
In some cases where a limit on the number of connections is needed, it's preferable to wait for a free channel over throwing an exception. In such cases this factory can be set in the configuration.

I also think it might be a good idea to add a simpler blocking/non-blocking boolean flag in the configuration for this behavior. It would be simpler for users to find as opposed to setConnectionSemaphoreFactory(new BlockingConnectionSemaphoreFactory())).

@slandelle
Copy link
Contributor

Actually, I've been concerned with such strategy for a long time.
Depending on how you are using AHC, you might be trying to send requests from the Netty IO threads: automatic retry on premature socket close, automatic redirect following, people sending requests form an AsyncHandler, etc.
Because of Netty threading model (sockets are assigned to a given thread), blocking such thread would block processing for all other sockets assigned on the same thread. I've seen people running into deadlocks this way.

At the very least, could you use tryAcquire(timeout) instead of infinite acquire?
Also, could you add some tests please?

@maltalex
Copy link
Contributor Author

That's not how I'm using AHC. But sure, it's a good idea to use tryAquire with sane defaults or configurable values instead of aquire, I'll make the change and add some documentation that says that mixing the blocking semaphore factory and sending requests from an AsyncHander are a bad combination. I can also add some tests.

Regarding redirects and similar functionality that's implemented within AHC itself - could you clarify the issue? From what I can tell, channel locks are released once the whole request completes (whether normally or abnormally). Requests following a redirect are sent by the netty thread while the the channel lock is still being held.

@maltalex
Copy link
Contributor Author

maltalex commented Dec 3, 2018

@slandelle I've made some changes. The blocking semaphores are now a configuration option. Channel acquisition goes through a tryAcquire() with a timeout. The default is zero.

@slandelle slandelle merged commit 16bca5c into AsyncHttpClient:master Dec 6, 2018
@slandelle slandelle added this to the 2.6.1 milestone Dec 6, 2018
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.

None yet

2 participants