Easier setup for connection pool events subscription : onLease/onRelease

[neoXfire]

Hi all,

Here is a proposal to enrich PoolingHttpClientConnectionManagerBuilder with a new method to customize the ConnPoolListener<HttpRoute> used in the underlying pool.

That will allow to implement custom instrumentation, to measure the time spent waiting in the pool before a connection get available for example.

Regards,

[ok2c]

@neoXfire Sure. We can do that. Please add a symmetric change to PoolingAsyncClientConnectionManager and its builder and add @since 5.7 tag to all new methods

[ok2c]

@neoXfire Please fix the code style violations. Looks good otherwise.

[neoXfire]

Hello @ok2c,

Thank you for the quick feedback.

Please note one of the new pool implementation in core5 (RouteSegmentedConnPool) do not ship this ConnPoolListener interface.

I was wondering if I should also update the core so that pool event subscription is consistent between all implementations ?

[ok2c]

Hello @ok2c,

Thank you for the quick feedback.

Please note one of the new pool implementation in core5 (RouteSegmentedConnPool) do not ship this ConnPoolListener interface.

I was wondering if I should also update the core so that pool event subscription is consistent between all implementations ?

@neoXfire Yes, please do so. Could you please also add a TODO comment to OFFLOCK policy about missing ConnPoolListener support in both connection managers?

[arturobernalg]

Hello @ok2c,
Thank you for the quick feedback.
Please note one of the new pool implementation in core5 (RouteSegmentedConnPool) do not ship this ConnPoolListener interface.
I was wondering if I should also update the core so that pool event subscription is consistent between all implementations ?

@neoXfire Yes, please do so. Could you please also add a TODO comment to OFFLOCK policy about missing ConnPoolListener support in both connection managers?

I’m not convinced OFFLOCK needs ConnPoolListener at all.
RouteSegmentedConnPool is intentionally lean / low-level: adding listener callbacks means extra branches and memory traffic on the hot lease/release paths, which is exactly what OFFLOCK is trying to minimize.

[ok2c]

I’m not convinced OFFLOCK needs ConnPoolListener at all.
RouteSegmentedConnPool is intentionally lean / low-level: adding listener callbacks means extra branches and memory traffic on the hot lease/release paths, which is exactly what OFFLOCK is trying to minimize.

@arturobernalg This may well be the case, null listener should have no performance impact, should it? And if the user explicitly chooses OFFLOCK policy and a custom listener, why should we deny that? Just because the pool may have sub-optimal performance?

[ok2c]

I am in favor of committing this change-set

[arturobernalg]

custom

@ok2c Agreed. Null listener is a no-op on the hot path apart from a predictable null check, so it should not have any measurable impact.

[neoXfire]

Hello @ok2c,
Thank you for the quick feedback.
Please note one of the new pool implementation in core5 (RouteSegmentedConnPool) do not ship this ConnPoolListener interface.
I was wondering if I should also update the core so that pool event subscription is consistent between all implementations ?

@neoXfire Yes, please do so. Could you please also add a TODO comment to OFFLOCK policy about missing ConnPoolListener support in both connection managers?

I’m not convinced OFFLOCK needs ConnPoolListener at all. RouteSegmentedConnPool is intentionally lean / low-level: adding listener callbacks means extra branches and memory traffic on the hot lease/release paths, which is exactly what OFFLOCK is trying to minimize.

I think with an appropriate implentation (notification callbacks stored in a ConnPoolListener<T> final field), in a sufficiently hot method and for the default case with no callbacks set up (connPoolListener == nul), the JIT can optimize this down to the same as if that if the conditional block for the callback event firing was never added.

If there is no intent to drop STRICT and LAX support in the future to replace it by this new lock-free implementation OFFLOCK only, it is fine and I can probably stick to the two legacy implementations only for my change. However, if it is actually the plan, I think you should try to find the right balance between raw performance and thrid-party plugability (observability, instrumentation...). That last aspect is for a lot of people as important as the first one.

Regards,

[ok2c]

@neoXfire Will you also create a follow-up pull request for OFFLOCK pool in core? @arturobernalg would you rather do it yourself?

[arturobernalg]

@neoXfire Will you also create a follow-up pull request for OFFLOCK pool in core? @arturobernalg would you rather do it yourself?

I'll do the change @ok2c . I'm just waiting that the running the performance test ended .