CompositeMeterRegistry#getRegistries data structure is not thread-safe

[ItsFlare]

Describe the bug Global registry is prone to racy access / doesn't offer thread-safe API

Environment

• Micrometer version: 1.14.5
• Micrometer registry: Prometheus
• OS: Ubuntu
• Java version: 21

To Reproduce Iterating over Metrics.globalRegistry.getRegistries() can throw ConcurrentModificationException.

Expected behavior Flawless iteration

Additional context Imo users would benefit from thread-safety for global datastructures as it can be very difficult to externally synchronize all access. There's also risk of this CME being thrown when calling Metrics.globalRegistry.close(), which also does a racy traversal of the underlying IdentityHashMap https://github.com/micrometer-metrics/micrometer/blob/c599c23ef02cd2b2d7e7eb7c05b2dbd1224d8b5e/micrometer-core/src/main/java/io/micrometer/core/instrument/composite/CompositeMeterRegistry.java#L237-L241

[jonatan-ivanov]

Thank you for the issue!

What is your use-case? What are you trying to do so that you need to call getRegistries() concurrently?

I'm especially curious about the concurrent usage of close() (most of the users should not call it but their framework of choice should). close() should be called when the registry is not needed anymore, at which point no other calls should be made to the registry (typically at shutdown).

[ItsFlare]

Thanks for the swift reply, Jonatan!

Our company provides a messenger service to thousands of concurrent users in a Java monolith. To minimize downtime, we're running a module system comprised of multiple spring applications, which can be dynamically updated/restarted. When analyzing classloader leaks we found that micrometer registries belonging to modules were not entirely deregistered by Spring. To manually deregister leaked instances during module shutdown, we do a transitive cleanup:

//In module shutdown
ConfigurableApplicationContext module = ...

module.getBeansOfType(MeterRegistry.class).forEach((s, meterRegistry) -> {
	meterRegistry.close();
	Metrics.removeRegistry(meterRegistry);

	// Throws ConcurrentModificationException (regardless of loop content)
	for (final MeterRegistry registry : Metrics.globalRegistry.getRegistries()) {
		if (registry instanceof CompositeMeterRegistry composite) {
			composite.remove(meterRegistry);
		}
	}
});

I haven't fully examined the nature of the concurrent access in our specific case, but I suspect modules interfere with each other when they shutdown concurrently. Regardless of the cause, I think thread-safe access would be desirable for anyone with similar setups.

Alternatively, micrometer could solve the root cause by:

• Weakly referencing registries
• Weakly referencing foreign objects (e.g. lambdas)
• Ensuring proper transitive deregistration internally

[jonatan-ivanov]

I think the root of these issues is using the global static Metrics registry. We really not recommend using it especially with Frameworks like Spring where dependency injection is what you should do instead of using static globals.

I'm thinking if it would make sense to open an issue in Spring Boot so that it will de-register instances from the global registry for you (but this would not change the above, you should not use it).

Also, in the code snippet in your comment, I would remove the registry first and close it after so that it is less likely to be used when closed.

I'm also thinking if Collections.synchronizedSet(Metrics.globalRegistry.getRegistries()) would help as a workaround.

I think the cause/use-case is somewhat important here since those methods are not meant to be called concurrently, that's why they are not thread-safe while what is meant to be called concurrently is.

[shakuzen]

• Ensuring proper transitive deregistration internally

I'm not sure the context of "internally" here but generally Spring will handle the order of creating and destroying beans as long as it properly knows about them and their dependencies (i.e. they and their dependencies are registered as beans and are not unmanaged objects used by Spring beans via static calls).

When analyzing classloader leaks we found that micrometer registries belonging to modules were not entirely deregistered by Spring.

I'd focus on resolving this and then the whole problem seems likely to go away. As Jonatan says, Spring modules should use dependency injection instead of global statics. Is there some problem preventing using dependency injection instead?

We can consider making things more defensive where it makes sense, but in this case it does sound more like a workaround to an underlying different issue.

[github-actions[bot]]

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

[shakuzen]

@ItsFlare we would still consider changing something here, but we'd appreciate more information. Let us know if you have a chance to provide more info.

On the one hand, this probably should be synchronized, and since we don't expect registries to be added and removed very frequently, the performance hit is probably not a big concern. That said, it isn't really expected that calls to add/remove would happen while close is being called on a CompositeMeterRegistry either, and we'd probably want to throw an exception when that happens anyway. Is that what's happening in your application?