ARTEMIS-2513 Large message's copy may be interfered by other threads #2859
Conversation
|
pls hold this, it's getting test failures in jenkins. |
| file.close(false); | ||
| newMessage.getFile().close(); | ||
| } finally { | ||
| cloneFile.close(); | ||
| } |
There was a problem hiding this comment.
Do we need to close underlying file of new large message after copy?
There was a problem hiding this comment.
yes it should be closed as original code. That'll cause file leak.
There was a problem hiding this comment.
@clebertsuconic @gaohoward I don't understand why we only close file when old file is not originally opened? In your case where other consumers open file and deliver, the file of new large message might not be closed. This will result in file leak and maybe data corrupt if broker crashes, wdyt?
There was a problem hiding this comment.
tbh I don't know either. I just keep the old behavior.
There was a problem hiding this comment.
@wy96f I'm doing some work with large messages, and I want to double check that as well.
There was a problem hiding this comment.
@wy96f / @gaohoward actually.. I will merge this right away.. as whatever I do would clash here.
and I will make sure I address how we open and close files.
|
�There are files leaking in your test. |
|
@clebertsuconic Yes I'm fixing it. Also same issue with hornetq. I'll fix that too. |
In LargeMessageImpl.copy(long) it need to open the underlying file in order to read and copy bytes into the new copied message. However there is a chance that another thread can come in and close the file in the middle, making the copy failed with "channel is null" error. This is happening in cases where a large message is sent to a jms topic (multicast address). During delivery it to multiple subscribers, some consumer is doing delivery and closed the underlying file after. Some other consumer is rolling back the messages and eventually move it to DLQ (which will call the above copy method). So there is a chance this bug being hit on.
|
I'll run jenkins again. |
|
@clebertsuconic Jenkins has 5 failures, not likely relevant and those failed tests are passing on my local. So it's safe now. |
|
@gaohoward can I keep this open for 1 or 2 days? I'm doing some work on large messages and I want to make sure why we have that code to close or keep files open. |
In LargeMessageImpl.copy(long) it need to open the underlying
file in order to read and copy bytes into the new copied message.
However there is a chance that another thread can come in and close
the file in the middle, making the copy failed
with "channel is null" error.
This is happening in cases where a large message is sent to a jms
topic (multicast address). During delivery it to multiple
subscribers, some consumer is doing delivery and closed the
underlying file after. Some other consumer is rolling back
the messages and eventually move it to DLQ (which will call
the above copy method). So there is a chance this bug being hit on.