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

Adhere to expected Exception Handler invocation order #2483

Merged
merged 6 commits into from Dec 2, 2022

Conversation

smcvb
Copy link
Member

@smcvb smcvb commented Nov 14, 2022

eBy using the @ExceptionHandler, a method becomes a special type of MessageHandlerInterceptor, specifically to catch thrown exceptions from message handlers.
As such, an exception handler is "just another message handler" from Axon Framework's perspective.

A part of being a regular message handler is the ordering implied on them through the HandlerComparator.
This comparator gives precedence on MessageHandlingMembers based on (1) priority, (2) message payloadType hierarchy, (3) parameter count, and (4) method name.

For any other interceptor, this means the defined priority by the HandlerComporator dictates the interceptor invocation order.
When we're dealing with an exception handler, the method is registered based on this order.
When then an exception is thrown, the last registered handler is invoked first.

This thus causes @ExceptionHandler annotated methods to be invoked in the inverse order.

To resolve this predicament, this pull request introduces the ReversedOrder marker interface.
Components can implement this interface, so that a Comparator or Comparable implementation can impose a reverse ordering on these components.
Furthermore, the HandlerComparator checks whether the objects compared on both implement this interface.
If they do, steps 2 through 4 are inverted.

Through this, the @ExceptionHandler order can be adjusted by letting the ResultHandlingInterceptorMember implement the ReversedOrder interface.

Due to the regular message handler ordering, @ExceptionHandler annotated
 intercepting methods are invoked in reverse ordering when an exception
 is thrown. This happens, as the exception handler interceptors are
 added to the InterceptorChain in the regular message handler ordering.
To resolve this, we need to specify exception handling members to have a
 reversed ordering instead.

#exception-handler-ordering
@smcvb smcvb added Type: Bug Use to signal issues that describe a bug within the system. Priority 2: Should High priority. Ideally, these issues are part of the release they’re assigned to. Status: In Progress Use to signal this issue is actively worked on. labels Nov 14, 2022
@smcvb smcvb added this to the Release 4.6.3 milestone Nov 14, 2022
@smcvb smcvb requested review from abuijze and a team November 14, 2022 16:00
@smcvb smcvb self-assigned this Nov 14, 2022
@smcvb smcvb requested review from gklijs and CodeDrivenMitch and removed request for a team November 14, 2022 16:00
Copy link
Member

@abuijze abuijze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote a little test to verify whether the outcome is consistent. Since the "expected" value here isn't exactly what we'd expect, I didn't commit the test as part of the PR.

    @RepeatedTest(100)
    void handlerOrderIsDeterministic() {
        List<MessageHandlingMember<?>> handlers = new ArrayList<>(Arrays.asList(stringHandler, objectHandler, longHandler, numberHandler, priorityHandler, stringHandlerReversedOrder, objectHandlerReversedOrder));
        Collections.shuffle(handlers, ThreadLocalRandom.current());

        handlers.sort(testSubject);

        List<MessageHandlingMember<?>> expected = Arrays.asList(priorityHandler, objectHandlerReversedOrder, longHandler, numberHandler, stringHandler, objectHandler, stringHandlerReversedOrder);
        assertEquals(expected, handlers);
    }

The actual order I would recommend for this is:
List<MessageHandlingMember<?>> expected = Arrays.asList(objectHandlerReversedOrder, stringHandlerReversedOrder, priorityHandler, longHandler, numberHandler, stringHandler, objectHandler);
Although the numberHandler and stringHandler have equal precedence rules, since they are both direct descendants of Object. Either one first works, as long as it is deterministic.

@smcvb smcvb requested a review from abuijze November 15, 2022 09:22
There's a window of opportunity, when a MessageHandlingMember is
implemented by an end-user, that the ReversedOrder implementation result
 in a non-deterministic outcome depending on the insert order. It may
 result in a > b, b > c but then a < c. Hence, the order of the sorted
 elements highly depends on the order they were detected. Although
 setting the priority to MIN_VALUE does not resolve this in theory, it
 will in practice, as the chances are extremely slim that (1) somebody
 has implemented their own MessageHandlingMember using the same
 priority, (2) these are not result handling members (that require the
 reverse order, and (3) that several of these custom handlers *and* the
 regular ResultHandlingInterceptorMember are combined within a single
 component.

#2483
There's a window of opportunity, when a MessageHandlingMember is
implemented by an end-user, that the ReversedOrder implementation result
 in a non-deterministic outcome depending on the insert order. It may
 result in a > b, b > c but then a < c. Hence, the order of the sorted
 elements highly depends on the order they were detected. Although
 setting the priority to MAX_VALUE does not resolve this in theory, it
 will in practice, as the chances are extremely slim that (1) somebody
 has implemented their own MessageHandlingMember using the same
 priority, (2) these are not result handling members (that require the
 reverse order, and (3) that several of these custom handlers *and* the
 regular ResultHandlingInterceptorMember are combined within a single
 component.

#2483
Correct method name to executableSignature, as the entire signature's
is used i.o. just the method name.

#2483
Copy link
Contributor

@gklijs gklijs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍

Copy link
Member

@CodeDrivenMitch CodeDrivenMitch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took me a while to wrap my head around the changes, but it looks good. I'm approving.

abuijze and others added 2 commits December 1, 2022 09:39
Removed the ReversedOrder marker interface in favor of detecting result handler by their attributes. Also made the ordering of handlers fully deterministic.
Minor clean up

#2483
Copy link
Member Author

@smcvb smcvb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the adjustments you've made, @abuijze 👍
Thanks for the effort!

@smcvb smcvb dismissed abuijze’s stale review December 2, 2022 14:34

Allard made adjustments to accommodate for his review remarks.

@sonarcloud
Copy link

sonarcloud bot commented Dec 2, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

96.0% 96.0% Coverage
0.0% 0.0% Duplication

@smcvb smcvb merged commit 4af458c into axon-4.6.x Dec 2, 2022
@smcvb smcvb deleted the bug/exception-handler-ordering branch December 2, 2022 14:57
@smcvb smcvb added Status: Resolved Use to signal that work on this issue is done. and removed Status: In Progress Use to signal this issue is actively worked on. labels Dec 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority 2: Should High priority. Ideally, these issues are part of the release they’re assigned to. Status: Resolved Use to signal that work on this issue is done. Type: Bug Use to signal issues that describe a bug within the system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants