MeterRegistry throws BeanCreationNotAllowedException on shutdown

[wojciechkedziora]

We use Micrometer with NewRelic in our Spring Boot 3.1.5 applications.

After migration to the newest Spring Boot we noticed that following WARN log always appears during application shutdown:

Failed to apply the value function for the gauge '{{custom_gauge_name}}'.

The exception behind above log looks like:

`org.springframework.beans.factory.BeanCreationNotAllowedException: Error creating bean with name 'entityManagerFactory': Singleton bean creation not allowed while singletons of this factory are in destruction (Do not request a bean from a BeanFactory in a destroy method implementation!)`

App is integrated with NewRelic agent - i checked that mentioned exception is triggered when:

	at com.newrelic.telemetry.micrometer.NewRelicRegistry.publish(NewRelicRegistry.java:168)
	at io.micrometer.core.instrument.push.PushMeterRegistry.publishSafely(PushMeterRegistry.java:67
	at io.micrometer.core.instrument.push.PushMeterRegistry.close(PushMeterRegistry.java:135)
	at io.micrometer.core.instrument.step.StepMeterRegistry.close(StepMeterRegistry.java:156)
	at com.newrelic.telemetry.micrometer.NewRelicRegistry.close(NewRelicRegistry.java:157)

Please note that similar bug for older Spring Boot 2.2 version was reported under #19557. I've reproduced reported bug: https://github.com/wojciechkedziora/spring-boot-3-19557 (check this comment for more details).

[philwebb]

Trying to use "depends on" configuration is turning out to be quite hard. The queueMeterBinder wants to use the QueueItemRepository but we also have MetricsRepositoryMethodInvocationListener which is trying to record metrics against all Spring Data Repository beans.

I'm wondering if the earlier idea floated in #19557 (comment) might be a better solution.

It feels like we should close all MeterRegistry beans before we start the destruction process.

The following seems to prevent the exception:

@Bean
MeterRegistryLifecycle meterRegistryLifecycle(ObjectProvider<MeterRegistry> meterRegistries) {
	return new MeterRegistryLifecycle(meterRegistries);
}

private static class MeterRegistryLifecycle implements SmartLifecycle {

	private volatile boolean running;

	private final List<MeterRegistry> meterRegistries;

	MeterRegistryLifecycle(ObjectProvider<MeterRegistry> meterRegistries) {
		this.meterRegistries = meterRegistries.orderedStream().toList();
	}

	@Override
	public void start() {
		this.running = true;
	}

	@Override
	public void stop() {
		this.running = false;
		meterRegistries.forEach(MeterRegistry::close);
	}

	@Override
	public boolean isRunning() {
		return this.running;
	}

}

[wilkinsona]

Configuring all MeterRegistry beans to depend on all MeterBinder beans also seems to help. I think closing the MeterRegistry beans early is less likely to have unwanted side-effects though. It would come at the cost of losing any metric updates that occur during shutdown but that's probably a price worth paying.

[philwebb]

We talked about this today as team and decided that we'd like to try the MeterRegistryLifecycle approach. Although this is a bug, we're concerned that it's too risky to put into earlier versions.

[wilkinsona]

As pointed out by @shakuzen, the lifecycle approach may not play nicely with coordinated restore at checkpoint. When the checkpoint is taken, lifecycles will be stopped. This will result in all of the meter registries being closed. With nothing to re-open the registries (and no API that even makes that possible) they'll remain closed upon restore and no more metrics will be published.

[mhalbritter]

Something like this works (at least in the example, not sure if it works on every occasion):

    @Bean
    MicrometerRegistryCloser micrometerRegistryCloser(ObjectProvider<MeterRegistry> meterRegistries) {
        return new MicrometerRegistryCloser(meterRegistries.orderedStream().toList());
    }

    private static class MicrometerRegistryCloser implements DisposableBean {
        private final List<MeterRegistry> meterRegistries;

        private MicrometerRegistryCloser(List<MeterRegistry> meterRegistries) {
            this.meterRegistries = meterRegistries;
        }

        @Override
        public void destroy() {
            for (MeterRegistry meterRegistry : this.meterRegistries) {
                if (!meterRegistry.isClosed()) {
                    meterRegistry.close();
                }
            }
        }
    }

[mhalbritter]

And this would work, too:

    private static class MicrometerRegistryCloser implements ApplicationListener<ContextClosedEvent> {
        private final List<MeterRegistry> meterRegistries;

        private MicrometerRegistryCloser(List<MeterRegistry> meterRegistries) {
            this.meterRegistries = meterRegistries;
        }

        @Override
        public void onApplicationEvent(ContextClosedEvent event) {
            for (MeterRegistry meterRegistry : this.meterRegistries) {
                if (!meterRegistry.isClosed()) {
                    meterRegistry.close();
                }
            }
        }
    }

Looking at org.springframework.context.support.AbstractApplicationContext#doClose, the ContextClosedEvent is published before the LifecycleProcessor is called and before destroy methods are invoked.

[wilkinsona]

I prefer using ContextClosedEvent to a DisposableBean. The point during shutdown at which the disposable bean will be disposed is somewhat variable – I think it depends on when the bean's created for one – whereas the ContextClosedEvent will always be published at a well-defined point in the lifecycle.

[mhalbritter]

We talked about it in our meeting and decided to go with the ApplicationListener approach.