Skip to content

Pooling managers: offload connection disposal from the pool lock.#726

Merged
arturobernalg merged 1 commit into
apache:masterfrom
arturobernalg:async-disposal-callback
Sep 20, 2025
Merged

Pooling managers: offload connection disposal from the pool lock.#726
arturobernalg merged 1 commit into
apache:masterfrom
arturobernalg:async-disposal-callback

Conversation

@arturobernalg

Copy link
Copy Markdown
Member

Close sockets off the pool lock via an async disposer. Improves lease latency under eviction/state mismatch with slow close.Includes classic/async tests and default-disposer baseline.

@arturobernalg arturobernalg requested a review from ok2c September 17, 2025 09:16
@ok2c

ok2c commented Sep 17, 2025

Copy link
Copy Markdown
Member

@arturobernalg This functionality is necessary for the classic connection management only where connections can get blocked in the #close method. The async connection management is unaffected as #close method there merely enqueues a command to be executed by the i/o reactor asynchronously.

Please reduce the scope to PoolingHttpClientConnectionManager only

@arturobernalg arturobernalg force-pushed the async-disposal-callback branch from 067a831 to 14b0b39 Compare September 17, 2025 11:19
@arturobernalg

Copy link
Copy Markdown
Member Author

@arturobernalg This functionality is necessary for the classic connection management only where connections can get blocked in the #close method. The async connection management is unaffected as #close method there merely enqueues a command to be executed by the i/o reactor asynchronously.

Please reduce the scope to PoolingHttpClientConnectionManager only

Done

@arturobernalg arturobernalg requested a review from ok2c September 17, 2025 18:13
* delegating the actual close to {@link DefaultDisposalCallback}.
*/
@Internal
final class AsyncDisposalCallback<C extends SocketModalCloseable>

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

removed.

@@ -409,6 +415,7 @@ public ConnectionEndpoint get(
}
} finally {
lock.unlock();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@arturobernalg This is a lease lock., not a global pool lock. It is perfectly OK to call #drainDisposals inside the method, for instance, at line 395 (prior to final ManagedHttpClientConnection conn = poolEntry.getConnection();)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@arturobernalg This is a lease lock., not a global pool lock. It is perfectly OK to call #drainDisposals inside the method, for instance, at line 395 (prior to final ManagedHttpClientConnection conn = poolEntry.getConnection();)

@ok2c drainDisposals() now runs inside LeaseRequest#get right before poolEntry.getConnection() (lease lock only), with the final safety drain retained; tests updated accordingly.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@arturobernalg The chances of anything going wrong in the remaining code is super-slim. And if it did happen the queue would get drained by another call / thread. But I think one #drainDisposals call should be enough here. Pick one you prefer. I leave it up to you.

@ok2c ok2c left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@arturobernalg Now the big question is whether this approach is stable enough to be activated by default or shall we mark it experimental and make optional in 5.6 release series?

@arturobernalg arturobernalg force-pushed the async-disposal-callback branch from 35be10e to 05042bc Compare September 19, 2025 12:04
@arturobernalg

Copy link
Copy Markdown
Member Author

Now the big question is whether this approach is stable enough to be activated by default or shall we mark it experimental and make optional in 5.6 release series?

@ok2c
opt-in experimental for 5.6, gather data, then flip on by default in 5.7 if no regressions... Please take another look.

@arturobernalg arturobernalg requested a review from ok2c September 19, 2025 12:04

@ok2c ok2c left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@arturobernalg Looks good to me.

…EFUL, run IMMEDIATE inline

Drain queue after lease/release/closeExpired/closeIdle to avoid holding the pool lock
@arturobernalg arturobernalg force-pushed the async-disposal-callback branch from 05042bc to 16c9d81 Compare September 20, 2025 10:16
@arturobernalg arturobernalg merged commit 88c315d into apache:master Sep 20, 2025
10 checks passed
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