Preserve thread context when calling getNextPart()#109519
Preserve thread context when calling getNextPart()#109519DaveCTurner merged 3 commits intoelastic:mainfrom
getNextPart()#109519Conversation
The listener supplied to `getNextPart()` may be completed in a non-default thread context, and this trips assertions about thread context pollution in the transport layer, so this commit adds the missing protection against that. Also the listener supplied to `getNextPart()` may be completed immediately on the transport thread, which could lead to a stack overflow, so this commit adds another `execute()` call to unconditionally fork a fresh task. Relates elastic#104851
|
Pinging @elastic/es-distributed (Team:Distributed) |
Which assertion is this? Have you seen such a test failure that can be linked to this PR? |
|
Any of the various existing calls to |
|
Also FWIW this is the same as what we do in |
| }), finishingWriteBodyPart::getNextPart); | ||
| ActionListener.run( | ||
| new ContextPreservingActionListener<>( | ||
| threadContext.newRestorableContext(false), |
There was a problem hiding this comment.
If the response has multiple continuations, it seems to me that we are technically restoring the threadContext from the previous iteration instead of the threadContext when we send the first part? But maybe it is effectively the same since (1) the capture and restore are chained and (2) there is nothing in writeAndFlush could change the threadContext?
There was a problem hiding this comment.
Right, but we're in the Netty event loop here so we are indeed not changing the thread context (which is why we call assertDefaultThreadContext to check that)
There was a problem hiding this comment.
assertDefaultThreadContext does not check response header. But I see what you mean.
With the changes in elastic#109519 we now do one more async step while serving the response, so we need to acquire another ref to track the new step. Relates elastic#109866 Relates elastic#110118 Relates elastic#110175 Relates elastic#110249
The listener supplied to
getNextPart()may be completed in anon-default thread context, and this trips assertions about thread
context pollution in the transport layer, so this commit adds the
missing protection against that.
Also the listener supplied to
getNextPart()may be completedimmediately on the transport thread, which could lead to a stack
overflow, so this commit adds another
execute()call tounconditionally fork a fresh task.
Relates #104851