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

Metrics not disposed for GracefulShutdownInstrumentedPool #3226

Closed
rs017991 opened this issue May 6, 2024 · 1 comment · Fixed by #3235
Closed

Metrics not disposed for GracefulShutdownInstrumentedPool #3226

rs017991 opened this issue May 6, 2024 · 1 comment · Fixed by #3235
Assignees
Labels
type/bug A general bug
Milestone

Comments

@rs017991
Copy link

rs017991 commented May 6, 2024

When working with various ConnectionProvider/MetricRegistrar arrangements, I am able to verify deRegistration of Metrics when I dispose the provider, but only if I do not set anything for disposeTimeout. Once I add that, it loses the ability to deregister its Metrics.

Expected Behavior

ConnectionProvider Metrics should be deRegistered on dispose, regardless of disposeTimeout use.

Actual Behavior

When adding disposeTimeout to a ConnectionProvider, Metrics are no longer deRegistered when the ConnectionProvider is disposed

Steps to Reproduce

Can reproduce this problem with a tweak to an existing reactor-netty-core test:
Add to ConnectionProvider setup in PooledConnectionProviderDefaultMetricsTest.testConnectionPoolPendingAcquireSize:

.disposeTimeout(Duration.ofSeconds(1))

Possible Solution

In PooledConnectionProvider, I see that Metric cleanup is only performed in onErrorResume for the GracefulShutdownInstrumentedPool block:

if (pool instanceof GracefulShutdownInstrumentedPool) {
return ((GracefulShutdownInstrumentedPool<T>) pool)
.disposeGracefully(disposeTimeout)
.onErrorResume(t -> {
log.error("Connection pool for [{}] didn't shut down gracefully", e.getKey(), t);
return Mono.fromRunnable(() -> {
if (poolFactory.registrar != null) {
poolFactory.registrar.get().deRegisterMetrics(name, id, remoteAddress);

whereas in the de facto else it is done unconditionally:
return pool.disposeLater().then(
Mono.<Void>fromRunnable(() -> {
if (poolFactory.registrar != null) {
poolFactory.registrar.get().deRegisterMetrics(name, id, remoteAddress);

Perhaps GracefulShutdownInstrumentedPool is supposed to perform the deRegistration on its own happy path, but I see nothing like that in its implementation. Without knowing how it was intended to work, not sure else I can glean...

Your Environment

  • Reactor version(s) used: encountered in 1.1.18; reproduction was just done against main
  • JVM version: 21
  • OS and version: Windows 11
@rs017991 rs017991 added status/need-triage A new issue that still need to be evaluated as a whole type/bug A general bug labels May 6, 2024
@violetagg violetagg removed the status/need-triage A new issue that still need to be evaluated as a whole label May 7, 2024
@violetagg violetagg self-assigned this May 7, 2024
@violetagg violetagg added this to the 1.0.45 milestone May 8, 2024
@violetagg
Copy link
Member

@rs017991 Thanks for the report and the reproducible example. I confirm that this is an issue and it will be fixed for the next release.

violetagg added a commit that referenced this issue May 8, 2024
…raceful shutdown

Dispose metrics not only when onErrorResume, but also on a happy path.

Fixes #3226
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug A general bug
Projects
None yet
2 participants