[fix][broker] Copy command fields and fix potential thread-safety in ServerCnx#19517
Conversation
| return CompletableFuture.completedFuture(false); | ||
| } | ||
| }); | ||
| }, ctx.executor()); |
There was a problem hiding this comment.
I requested this change last week because getAuthenticationData() was getting a non-volatile variable. However, before this PR was opened, I contributed #19507. @nicoloboschi - we can probably remove this part of the diff since reading the authenticationData is now thread safe.
In the long term, it might be worth exploring if we should only read this metadata from the event loop, but for now, these variables are volatile.
michaeljmarshall
left a comment
michaeljmarshall
left a comment
There was a problem hiding this comment.
Thanks for following up on this @nicoloboschi!
| return CompletableFuture.completedFuture(false); | ||
| } | ||
| }); | ||
| }, ctx.executor()); |
There was a problem hiding this comment.
I requested this change last week because getAuthenticationData() was getting a non-volatile variable. However, before this PR was opened, I contributed #19507. @nicoloboschi - we can probably remove this part of the diff since reading the authenticationData is now thread safe.
In the long term, it might be worth exploring if we should only read this metadata from the event loop, but for now, these variables are volatile.
…ServerCnx (apache#19517) (cherry picked from commit 0bb0f6b) (cherry picked from commit 36582d8)
4 participants
Motivation
In #19467 we introduced a couple of possible issues about thread-safety of the
BaseCommandandServerCnxinstances.Original comments from @michaeljmarshall :
Modifications
Documentation
docdoc-requireddoc-not-neededdoc-complete