Skip to content

ZOOKEEPER-4419: The potential exception in Learner$LeaderConnector.connectToLeader may cause unnecessary re-election or service delay #1782

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.

Sign up for GitHub

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

Open
functioner wants to merge 2 commits into
base: master
Choose a base branch
from
+19 −23
Open

ZOOKEEPER-4419: The potential exception in Learner$LeaderConnector.connectToLeader may cause unnecessary re-election or service delay #1782

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f0fab48
ZOOKEEPER-4419: The potential exception in Learner.connectToLeader ma…
functioner Nov 27, 2021
8d130a5
update
functioner Jun 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Learner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,7 +396,7 @@ public void run() {
}

private Socket connectToLeader() throws IOException, X509Exception, InterruptedException {
Socket sock = createSocket();
Socket sock = null;

// leader connection timeout defaults to tickTime * initLimit
int connectTimeout = self.tickTime * self.initLimit;
Expand All @@ -410,8 +410,12 @@ private Socket connectToLeader() throws IOException, X509Exception, InterruptedE
int remainingTimeout;
long startNanoTime = nanoTime();

for (int tries = 0; tries < 5 && socket.get() == null; tries++) {
for (int tries = 0; tries < 5; tries++) {

@kezhuw kezhuw Jul 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

connectToLeader could be called concurrently for different addresses, so it is good to break eagerly when there is a success connection.

try {
sock = createSocket();
if (socket.get() == null) {
continue;

@kezhuw kezhuw Mar 29, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

What does this mean ?

I think all we have to do is moving createSocket under try-catach so the for-loop can continue retry.

Besides, shall we close sock on execeptional ?

@anmolnar anmolnar Mar 31, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this either. Wouldn't be better to use the try block for creating and closing?

try (Socket sock = createSocket())  {
...
}

@functioner functioner Mar 31, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anmolnar oh you're right. try block for creating and closing is better.

@kezhuw what's the expected behavior when closing sock on exception? catch and throw out of the function?

@kezhuw kezhuw Apr 1, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try block for creating and closing is better.

We need the created socket on return, so "try-with-resources" statement is not going to work here.

what's the expected behavior when closing sock on exception? catch and throw out of the function?

I think it should be catch-and-swallow. Socket::connect failure is the reason to retry.

@PAC-MAJ PAC-MAJ Dec 30, 2025 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a small question if I may.
Do we really want to "continue;" and skip the backoff delay leaderConnectDelayDuringRetryMs ?
For the exceptions, it wasn't previously done neither but .close() with catch and swallow would do the trick in my opinion.

@functioner functioner Jan 13, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PAC-MAJ

The "continue;" you mentioned handles the case that sock = createSocket(); gives null value. In this case, the abnormal null value comes from some internal issue of the zk learner, not from leader's connection issue. Hence introducing the backoff delay leaderConnectDelayDuringRetryMs will not make sense, because that delay should focus on coordinating learn-leader connection frequency.

In a broader sense, the loop for (int tries = 0; tries < 5; tries++) is for the retry of creating socket AND connecting leader. Maybe this logic could be decoupled as two phases? For example, write a retry for loop for the leaderConnectDelayDuringRetryMs backoff delay, after successfully creating the socket.

@PAC-MAJ PAC-MAJ Jun 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hello,

Key comments are:

  1. Move createSocket() into the try block so socket creation failures are retried.
  2. Close the socket in the catch block before retrying.
  3. Remove the socket recreation from the catch block since the next iteration creates a fresh socket.

Then I propose the following outline.

Socket sock = null;

for (...) {
    try {
        sock = createSocket();

        remainingTimeout = ...;

        sockConnect(sock, ...);

        if (sslEnabled) {
            handshake(...);
        }

        sock.setTcpNoDelay(...);

        break; // success

    } catch (IOException e) {

        // close failed socket before retrying
        if (sock != null) {
            try {
                sock.close();
            } catch (IOException closeException) {
                LOG.debug("Failed to close socket after connection failure", closeException);
            }
            sock = null;
        }

        // keep existing timeout calculation and logging unchanged
        remainingTimeout = ...;

        if (remainingTimeout <= leaderConnectDelayDuringRetryMs) {
            LOG.error(...);
            throw e;
        } else if (tries >= 4) {
            LOG.error(...);
            throw e;
        } else {
            LOG.warn(...);
        }
    }

    Thread.sleep(leaderConnectDelayDuringRetryMs);
}

return sock;

}
// recalculate the init limit time because retries sleep for 1000 milliseconds
remainingTimeout = connectTimeout - (int) ((nanoTime() - startNanoTime) / 1_000_000);
if (remainingTimeout <= 0) {
Expand Down Expand Up @@ -451,7 +455,6 @@ private Socket connectToLeader() throws IOException, X509Exception, InterruptedE
remainingTimeout,
address,
e);
sock = createSocket();
}
}
Thread.sleep(leaderConnectDelayDuringRetryMs);
Expand Down