ARTEMIS-5155 Send of AMQP large messages should close the LargeMessageReader before proceeding with the message delivery

[clebertsuconic]

This is fixing LargeMessageInterruptTest

[clebertsuconic]

this change is reverting the offending change from c83ed89

As I said before, marking the LargeMessageReader as complete after the delivery of the message of the queue could cause the message body to go truncated, if a network failure happened between the delivery and the actual close of the message (aka race).

I'm merging this PR now as this is clearly an issue and this fixes that issue.

[gemmellr]

I changed it to draft, as we have been looking into why this change might have any effect, as it didnt seem to either myself or Tim that it should have the desired impact, and possibly pointed to a potential concurrency problem. Whilst moving this might indeed help adjust the behaviour to stop a test failing, it still isnt clear to either of us it is actually the correct change given the point the close is done. Tim may have found a related issue around the delivery handling offloading large messages to another thread.

In short, we still need to look at this a bit closer before merging.

[clebertsuconic]

@gemmellr I don't think this is a draft.. it's definitely fixing the issue.. it's a small issue..

last packet arrived for the message
deliverMessage
close reader

another thread detects a failure... removed the file

consequence is an empty / truncated file

[clebertsuconic]

@gemmellr all I'm fixing now is.. when closeCurrentReader is called, it's deleting the file...

which is what I'm fixing.. if you want to improve other things in large message.. it's totally orthogonal to this.. this change is fixing the issue.

[gemmellr]

We are saying it may have been wrong the way it was as well, so just putting it back that way is not necessarily correct. Still looking at it further. In the meantime, this has been this way for 8 months, lets look at it before merging rather than after.

[tabish121]

There is at least one race in the AMQPLargeMessageReader where the bytes being written to the large message file is done on a session thread and only when the message is complete is it submitted to the receiver via the connection thread. If a network error happens that causes the receiver to close its active reader (which deletes the large message file) and at the same time the processing of a large message is about to complete and schedule the 'onMessageComplete' call to process the read message the message will get processed after its file is deleted and be put onto the Queue as if it was valid resulting in a corrupted message.

We need to reevaluate the logic and how we process AMQP large messages off the netty thread to better account for the chasing scenarios to ensure files are deleted but completed message still get processed as intended.

[clebertsuconic]

I think the close should happen before onMessageComplete, on the caller thread.

[tabish121]

This null assignment should remain in a finally block as it ensures that the reader is always nulled when onMessageComplete gets called.

[clebertsuconic]

I have closed this in favor of Tim's PR.