Check hasNext when looking for ordered UserDestinationResult sessionIds

[brandenclark]

Found a bug in UserDestinationMessageHandler.SendHelper#send that occurs when the UserDestinationResult was created by the pre-6.1.x constructor. Bug was introduced with 3277b0d

Ideally this fix will target 6.1+ and should be compatible. Not sure if there's a special way to call that out as a first-time contributor.

Changes

• Add a test that reproduces the error and subsequently covers the fix
• Check if the iterator has a next value before accessing it and use the default condition if it does not
  • Using the default seems to be the original intent and was just missing this initial check

Bug Details

The UserDestinationMessageHandler unsafely retrieves values from an iterator of sessionId strings to lookup ordered messages.

This happens when the UserDestinationResult used to route the message was created with the pre-6.1.x constructor this retrieval throws a NoSuchElementException.

This is because the older constructor passes a null to the newer constructor for the sessionIds constructor arg. The class attempts to safe handle this null arg by creating an EmptySet that eventually provides an [EmptyIterator],(https://github.com/openjdk/jdk/blob/98a93e115137a305aed6b7dbf1d4a7d5906fe77c/src/java.base/share/classes/java/util/Collections.java#L4559) which then throws for Set#next.

This bug seems to only happen for custom implementations of UserDestinationResolver as the DefaultUserDestinationResolver always provides a populated set to the new constructor.

[mdhirsch]

Interestingly, sessionIds coming from destinationResult.getSessionIds() is documented to be nullable, but actually the sessionIds are final and instantiated to always be non-null. That method could be annotated @nonnull and the logic here would be simpler. sessionIds doesn't have to be checked for null in this line, and therefore itr is never null and doesn't need to be check for null in line 283.

[brandenclark]

Good point! I'll make that change just so it's up for viewing and we can get some feedback from @rstoyanchev.

[rstoyanchev]

The proposed updates make sense. Thanks for the pull request.

As UserDestinationResult is typically created by the framework, and we no longer use the pre-6.1 constructor, we should probably deprecate it. Could you clarify what or who is calling that constructor?

[brandenclark]

We have a custom UserDestinationResolver instead of using the framework provided one, which should be the only way to trigger this.

I'm pretty novice in this space, but it seems like the pre-6.1 constructor is still useful when needing to customize the UserDestinationResolver and explicitly not requiring ordered messaging templates. Also feels strange to deprecate what was the default behavior before 6.1 and require deep knowledge of the subsystem to safely continuing to do that. Especially since the behavior is still supported in the UserDestinationMessageHandler and not being removed entirely.