-
Notifications
You must be signed in to change notification settings - Fork 914
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
ARTEMIS-4771 Avoid NPE while delivering amqp large message after a close #4932
Conversation
final Delivery localDelivery = delivery; | ||
final MessageReference localReference = reference; | ||
final LargeBodyReader localBodyReader = largeBodyReader; | ||
|
||
if (localDelivery == null || localReference == null || localBodyReader == null) { | ||
logger.debug("Write got closed before tryDelivering was called"); | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cant we just check 'closed' here instead of all these changes and checks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would make more sense as the closed indicator is the only volatile in the mix and would generally convey the state in a timely manner. Copying the other resources local is fine but doesn't pass a barrier so they still aren't the best indicator of closedness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I checked just closed, I guess you could still hit the NPE. You could check the closed variable.. but I don't know how to get rid of the possible NPE here without adding synchronization.
The NPE will be just an annoyance since it's closing anyway.. but I was asked to mitigate the NPE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lines 126 to 134 in 14e83fa
private void resetClosed() { | |
message = null; | |
reference = null; | |
delivery = null; | |
largeBodyReader = null; | |
position = 0; | |
initialPacketHandled = false; | |
closed = true; | |
} |
so.. one thread is in resetClose.. another thread is on the tryDeliver...
closed is only set to true at the end of the statement...
as a result you could the NPE.
you may say set closed first...but still I don't feel like this is completely atomic without added synchronization... however I don't want synchrnoization here.. just a local copy and checking it should be enough for me.
I plan to add a test on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So why not just add synchronization if your goal is to make the code thread safe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the code is being used as it was originally intended and only accessed from one thread the check on closed state should be all that's needed yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok.. I will make the change and add a test.
although the test is somewhat difficult as I think you need flowControl / resume to make it happen.
although I could make a mockTest. If I can't reproduce it with a simple consumer being closed I will create a mock test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gemmellr the exception is a network disconnect. Client disconnects and a network failure is transmitted. Close is called.
For that we need to either use a cached local variable or add synchronization.
I suggest we add a local cache as it doesn't really matter if we just send stuff on the already closed session. Adding synchronization on the write may risk deadlocks.
I'm not able to fix it this week as I'm going out for a week. if you desire to pick up this issue please close my PR and open a new one.
I will keep my as draft until I can come back into this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The close method is only safe to call on the connection thread, which is where it appears to be being called. Are you seeing otherwise? If not there is no benefit in introducing the 3 additional variables and complicating the code with a huge if statement (might even mislead folks into thinking there is some safety there isnt). Should be no need for synchronization as the writing code is not called concurrently. It would seem its simply running from a previous scheduling, but no longer needs to. If so that can be addressed by checking 'closed' at the start of the scheduled task if it was closed already before proceeding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed the existing code again and the Writer is still confined to the connection thread unlike the modifications that moved some of the large message reader off that thread. This would imply that a simple check on closed is sufficient here as this case would just be an already scheduled run of this writer that was behind the remote close handling.
c1c8947
to
350b925
Compare
Replaced by #4942 |
No description provided.