-
Notifications
You must be signed in to change notification settings - Fork 948
ARTEMIS-5890 AMQP LargeMessage writer should check connection state on the WriterThread #6218
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
base: main
Are you sure you want to change the base?
Conversation
593bd21 to
72a1674
Compare
| logger.trace("AMQP Large Message Writer was closed before queued write attempt was executed"); | ||
| return; | ||
| } | ||
| if (protonSender.getSession().getConnection().getRemoteState() != EndpointState.ACTIVE || protonSender.getSession().getConnection().getLocalState() != EndpointState.ACTIVE) { |
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 code actually already has several checks for closed state related to the sender but in reality even this is not a guarantee that you won't see this error as the proton code was not meant to be used outside the thread that manages the engine so the visibility of these values is not ensured to be updated in this delivering thread.
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.
What happens is this:
- there's a pending task in the executor to deliver the large message.
- before the deliver happens, an onRemoteClose is called:
**Lines 839 to 850 in db04425
handler.requireHandler(); connection.close(); connection.free(); for (AMQPSessionContext protonSession : sessions.values()) { protonSession.close(); } sessions.clear(); // We must force write the channel before we actually destroy the connection handler.flushBytes(); destroy(); - The Writer is not closed, when the task reaches the execution point the Link will have some invalid state.
We need to check the isClosed from the session / connection / link somehow. or we need to make the onRemoteClose to also close the Writer.
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 just amended to another possible fix..
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.
BTW: The thread being used here is the netty thread, as this is started with connection.runLater calls. so this is in the same thread.
ba54e76 to
c9adbf4
Compare
…n the WriterThread
c9adbf4 to
3cb3c7b
Compare
|
there are a few important comments added on https://issues.apache.org/jira/browse/ARTEMIS-5890 |
|
I've pushed a branch that simplifies the changes and accounts for the various paths that are triggering WARN logging which reduces the amount of change here. While the checks for the connection state were actually thread safe they don't account for all the ways this could hit closed states. https://github.com/tabish121/artemis/pull/new/ARTEMIS-5890 Have a look |
…