ARTEMIS-5530 Some handling of compressed messages can throw NegativeA…

[AntonRoskvist]

…rraySizeException

Opening this as a draft as I'd like feedback regarding some details.

The new test "testUpstreamFederatedAddressWithCompressedMessage" demonstrates the issue and should be fixed with the rest of the changes here.

While doing additional testing I stumbled on some (to me at least) unexpected behavior, demonstrated in the "testCompressedMessageRouting" where resending the same message multiple times exhibited the NegativeArraySizeException as well, though to be honest I am not sure if this is a legitimate use case or not. That test has some comments and logging not intended for a finalized PR, their just meant to highlight some points of interest.

[clebertsuconic]

@AntonRoskvist I can work on this early next week

[clebertsuconic]

I'm kind of confused on what is the real issue here..

is it about the compressed being set while the message is still pending?

what is the issue with rollback?

Or is it the message being used between multiple producers and different session throwing an error?

I'm kind of confused on what is the actual issue you saw.

[AntonRoskvist]

Hi,

Thanks for looking into this. Do note that I have two PRs pertaining to handling of compressed messages. This one and #5574.

In this case (reported by another user), some handling of a compressed message can throw a NegativeArraySizeException which is demonstrated by the added test "testUpstreamFederatedAddressWithCompressedMessage". This is a fairly "straight forward" setup where the federation of a compressed message triggers the issue. Running that with no other changes will demonstrate the issue.

While working on a test for this i also stumbled on some similar issue but that's triggered by a questionable use-case, where sending the same compressed message multiple times also gives the NegativeArraySizeException after a few sends. The included test "testCompressedMessageRouting" (which is not meant to be in the final PR) demonstrates the behavior with some added logging and code comments.

#5574 on the other hand is about consumer credits getting out of balance when receiving and rollbacking a compressed "regular" message. It will give too many credits back so the consumerWindowSize will get larger and larger until an integer overflow happens (leaving the consumer "busy" until restarted).

[clebertsuconic]

OH.. thanks.. I was confused :)

[clebertsuconic]

@AntonRoskvist one feature of github is to allow us to push on your branch when you send a pull request.

I used that feature to push -f on your branch for the rebase I performed here.

(I thought it would be better to just push -f instead of asking you to rebase it again... so if you look at your branch please rebase it against your fork).

[AntonRoskvist]

@clebertsuconic thanks, rebase worked great.

There is still some issue here that I cannot quite figure out though... So even with the recent changes I see the same behavior on the test "testCompressedMessageRouting".

I also went ahead and made a small change to the federation test ("testUpstreamFederatedAddressWithCompressedMessage") where it now sends three messages, one regular, one large compressed and one compressed into a regular message. The last case fails because (from what I can tell at least) LargeMessageControllerImpl.LargeData is empty for the "regular compressed" message going through federation.

Still trying to understand the behavior of both these tests but pushing the change I made to the federation test.

[clebertsuconic]

From what I looked, ClientProducerImpl should copy the message if it's compressed. The compression send is changing the body buffer and properties what would affect subsequent sends.

copy would make it work.. there will be some impact on the client's GC.. but I think that would be the best approach.

[clebertsuconic]

replacing this PR by #5867