HandlerDefinitions and HandlerEnhancerDefinitions are shared between multiple Spring Application Contexts

[nils-christian]

Hi,

We noticed that the HandlerDefinitions and HandlerEnhancerDefinitions are shared between otherwise separated Spring Application Contexts. The reason for this is that the HandlerDefinitionFactoryBean uses ClasspathHandlerDefinition and ClasspathHandlerEnhancerDefinition to load some definitions. Those classes use static fields to store the loaded definitions. This breaks with the concept of Spring beans.

Basic information

• Axon Framework version: 4.7.1
• JDK version: 17
• Complete executable reproducer if available (e.g. GitHub Repo): https://github.com/nils-christian/axon-issue-shared-definitions

Steps to reproduce

Please take a look at the provided example project. Please note that we do not use the auto configuration of Axon. Instead we use a hierarchical Spring Context with a configuration class which basically contains Axon's auto configuration. The example application retrieves the HandlerEnhancerDefinitions from both child contexts and calculates the intersection of them.

Expected behaviour

The intersection should be empty.

Actual behaviour

The intersection contains various definitions.

Workaround

Build a custom HandlerDefinitionFactoryBean which doesn't store the objects in a static map. This is difficult though, because the HandlerDefinitionFactoryBean is not marked with ConditionalOnMissingBean.

[smcvb]

Thanks once more for filing an issue with us, @nils-christian!

First and foremost, did this predicament start occurring with Axon Framework 4.7.1?
Or did you face the same concern before 4.7.0?

The reason for this is that the HandlerDefinitionFactoryBean uses ClasspathHandlerDefinition and ClasspathHandlerEnhancerDefinition to load some definitions.
Those classes use static fields to store the loaded definitions. This breaks with the concept of Spring beans.

I fully agree that the HandlerDefinition and HandlerEnhancerDefinition do not align with regular Spring bean processing.
That's because they follow the Service Loader paradigm, regardless of whether a user is in a Spring Context, yes or no.
Although this decision is debatable, I would not want to make an exception just for the HandlerDefinition and HandlerEnhancerDefinition classes.

This approach is followed for numerous infrastructure components within the Framework.
I'd much rather align the approaches for the lifespan of Axon Framework 4.
For Axon Framework 5, we are already discussing whether this flow should stick as it is, making it better suited for adjustment.

Granted, the setup causes issues in your environment, I assume?
If making the HandlerDefinitionFactoryBean conditional is sufficient for your scenario, I think that is a fair adjustment.

[nils-christian]

Hi @smcvb,

The issue is not exclusive to the 4.7.0 release but exists in 4.6.3 as well.

I would like to explain our issue with the shared definitions. As you might know, we use multiple instances of Axon in the same application using Spring's hierarchical context. Furthermore, we start some contexts in parallel, which leads to a ConcurrentModificationException in the PayloadAssociationResolver. This is how we detected the issue.

I know that this particular issue has been adresses in #2591, but I wonder if there could be any other issues regarding the shared definitions. Could something unexpected break or should the definitions be otherwise threadsafe and everything?

Unfortunately, making the bean conditional is not really sufficient. Replacing the static code in all involved classes is rather cumbersome, to be honest.

[smcvb]

The issue is not exclusive to the 4.7.0 release but exists in 4.6.3 as well.

Good to know. I assumed as much, but wanted to make sure.

Furthermore, we start some contexts in parallel, which leads to a ConcurrentModificationException in the PayloadAssociationResolver.

I similarly assume this also occurred in 4.6.3 and before?
Or did you start seeing this in 4.7.x?

I know that this particular issue has been adresses in #2591, but I wonder if there could be any other issues regarding the shared definitions.
Could something unexpected break or should the definitions be otherwise threadsafe and everything?

A valid concern, @nils-christian, and one I was jumping on right after reading your reply.
I've validated the HandlerDefinitions (with only one real implementation, the AnnotatedMessageHandlingMemberDefinition) and HandlerEnhancerDefinitions for any other predicaments that may cause concurrency issues.
As it stands, the only feasible one for the problematic behavior is the SagaMethodMessageHandlerDefinition.
With the exception of custom variants, of course.

However, that's not something that's changed recently. At all.
We did adjust the etrieval of the HandlerDefinition in pull request #2545, to allow HandlerEnhancerDefinition registration through the Configurer.
Nonetheless, I do not (yet) see how this may cause issues around the SagaMethodMessageHandlerDefinition.

Long story short, I'm running some internal errands on the subject right now.
If something worthwhile comes up, I'll share it with you.

Replacing the static code in all involved classes is rather cumbersome, to be honest.

Would you mind specifying what you think should be replaced for your scenario?

[nils-christian]

I similarly assume this also occurred in 4.6.3 and before? Or did you start seeing this in 4.7.x?

We have seen it after a (seemingly) simple refactoring of a test case on 4.7.X, but the same test case crashes with 4.6.3. So it is not a regression in Axon.

A valid concern, @nils-christian, and one I was jumping on right after reading your reply. I've validated the HandlerDefinitions (with only one real implementation, the AnnotatedMessageHandlingMemberDefinition) and HandlerEnhancerDefinitions for any other predicaments that may cause concurrency issues. As it stands, the only feasible one for the problematic behavior is the SagaMethodMessageHandlerDefinition. With the exception of custom variants, of course.

Good to know, thank you.

However, that's not something that's changed recently. At all.

Agreed. See above. I don't think of this as regression, but rather an already existing behaviour that now concern us.

Would you mind specifying what you think should be replaced for your scenario?

We tried to replace it and ended up with the following (not yet working) modifications:

• Obviously HandlerDefinitionFactoryBean has to be replaced so that we can introduce Spring managed beans for ClasspathHandlerEnhancerDefinition and ClasspathHandlerDefinition.
• Said classes would be modified versions of the originals which would no longer use static fields.
• Now we would also override the regular MultiHandlerDefinition and provide new ordered-methods, so that we can introduce HandlerEnhancerDefinition into the parameters as well.

Doing that we still get some missing handler (enhancer) definitions, I think. So we might have overlooked something.

That said: We are currently only encountering the ConcurrentModificationException in our specific test. So if it is "just" a concurrency issue during startup, I think we can also wait for a version of Axon containing #2591.

Also, I just executed our test case in question against 4.7.2-SNAPSHOT of Axon. I can confirm that the original ConcurrentModificationException no longer occurs. However, it seems that the event handler registration does not work reliably when executed in parallel. Interestingly enough, this is the same error I get after rewriting the definitions as described above. I will investigate this further and check if this is an issue on our side or in Axon.

[nils-christian]

We double checked and there was indeed another minor issue in our code. It works with 4.7.2-SNAPSHOT. So from our side, #2591 reduces the severity of the issue with the shared definitions.

[smcvb]

Thanks so much for all the feedback here, @nils-christian.
I have had some chats internally to discuss this occurrence on the HandlerDefinition and HandlerEnhancerDefinition.

Both are acted on during the application's start-up to build the message-handling components Axon Framework can deal with.
In the majority of scenarios, this is a single-threaded process.
Hence for most use cases, the components don't have to be thread-safe.

However, it's such an easy addition to keeping in mind when, for example, you're constructing a HandlerEnhancerDefinition, that we think it's important enough to take into account.
We will add this pointer to the JavaDoc, as this is not directly clear at the moment.
The Reference Guide will follow this pointer somewhere in the future too.

We tried to replace it and ended up with the following (not yet working) modifications:

That's a bit too much, indeed.
Doable but not overly user-friendly.

However, I feel this predicament stems from using a hierarchical Spring context, correct?
Note that better supporting hierarchical Spring contexts are on the backlog to traverse.
As such, I feel this is more a kind of enhancement instead of a bug.
What do you think?

[nils-christian]

However, I feel this predicament stems from using a hierarchical Spring context, correct? Note that better supporting hierarchical Spring contexts are on the backlog to traverse.

Well, yes, but even when using Spring's hierarchical contexts, one wouldn't normally start them in parallel. So I have to admit that our scenario is pretty unique in this regard.

As such, I feel this is more a kind of enhancement instead of a bug. What do you think?

I agree. As mentioned earlier, the bugfix in the upcoming version solves the issue for us.