jonmv commented on code in PR #2154:
URL: https://github.com/apache/zookeeper/pull/2154#discussion_r1766348763
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Learner.java:
##########
@@ -900,26 +900,26 @@ boolean isRunning() {
}
void closeSocket() {
- if (sock != null) {
- if (sockBeingClosed.compareAndSet(false, true)) {
- if (closeSocketAsync) {
- final Thread closingThread = new Thread(() ->
closeSockSync(), "CloseSocketThread(sid:" + zk.getServerId());
- closingThread.setDaemon(true);
- closingThread.start();
- } else {
- closeSockSync();
- }
+ if (sockBeingClosed.compareAndSet(false, true)) {
+ if (sock == null) { // Closing before establishing the connection
is a noop
+ return;
+ }
Review Comment:
Close could be called from different threads, and `sockBeingClosed` ensures
memory visibility for `sock`, as it's set _after_ `sock` is assigned in
`connectToLeader`.
The only other thread I can see that closes the learner, right now, is the
sync processor, which is initialised _after_ `sock` is assigned, so it works
as-was, but I still prefer to be explicit about this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
