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

[AMQ-9532] Convert DestinationMapNode from recursion to loops. #1254

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Nikita-Shupletsov
Copy link

Because destinations are parsed recursively, it's possible to trigger java.lang.StackOverflowError if a destination has too many nodes.
the PR is converting the recursion to loops.

Ran DestinationMapTest.java to make sure the behavior of the class stays the same. There is no need for additional testing in my opinion, because the behavior is supposed to stay the same as it was before the change.

Copy link
Contributor

@cshannon cshannon left a comment

Choose a reason for hiding this comment

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

This looks like a nice change to me and should certainly improve things if there are a lot of paths.

I looked over everything and I couldn't spot any issues but it's always a little tricky looking at recursive code. I agree that there should be enough tests already. Besides the DestinationMapTest which exercises the map very well, the map is used all over the code base and is extended by the PolicyMap plus others, so I would expect some test failures if there was an issue. I looked at the failures for this PR and it seems like it's just timeout stuff and un-related but it would be nice to see a clean jenkins run.

@mattrpav and @jbonofre , can you take a quick look at this as well and see if you spot anything? We could target it for a 6.1.x release but not sure if we should just keep it for 6.2.0

@mattrpav mattrpav self-requested a review July 16, 2024 13:14
@mattrpav
Copy link
Contributor

@Nikita-Shupletsov how many is too-many nodes to trigger a stack overflow? What was the -Xss setting when the SO occurred?

@Nikita-Shupletsov
Copy link
Author

@Nikita-Shupletsov how many is too-many nodes to trigger a stack overflow? What was the -Xss setting when the SO occurred?

with -Xss256k it's around 1300

i've encountered the issue due to this: https://activemq.apache.org/components/classic/documentation/message-redelivery-and-dlq-handling#setting-expiration-on-messages-in-the-dlq
there were DLQs for DlQs and so on

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants