[
https://issues.apache.org/jira/browse/ZOOKEEPER-2366?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15346833#comment-15346833
]
Raul Gutierrez Segales commented on ZOOKEEPER-2366:
---------------------------------------------------
[~fpj]: what I am saying is that closing the (old) socket should always be best
effort, the outer try/catch means that acceptThread.wakeupSelector() (and what
comes after) would be skipped, which doesn't sound right.
I think it should be like this:
{code}
private void tryClose(ServerSocketChannel s) {
try {
s.close();
} catch (IOException sse) {
LOG.error("Error while closing server socket.", sse);
}
}
public void reconfigure(InetSocketAddress addr) {
ServerSocketChannel oldSS = ss;
try {
this.ss = ServerSocketChannel.open();
ss.socket().setReuseAddress(true);
LOG.info("binding to port " + addr);
ss.socket().bind(addr);
ss.configureBlocking(false);
acceptThread.setReconfiguring();
tryClose(oldSS);
acceptThread.wakeupSelector();
try {
acceptThread.join();
} catch (InterruptedException e) {
LOG.error("Error joining old acceptThread when reconfiguring
client port {}",
e.getMessage());
Thread.currentThread().interrupt();
}
acceptThread = new AcceptThread(ss, addr, selectorThreads);
acceptThread.start();
} catch(IOException e) {
LOG.error("Error reconfiguring client port to {} {}", addr,
e.getMessage());
tryClose(oldSS);
}
{code}
Thus, the outer try/catch is about the rest of the stuff we are doing and
closing the old socket is only a best effort thing.
> Reconfiguration of client port causes a socket leak
> ---------------------------------------------------
>
> Key: ZOOKEEPER-2366
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2366
> Project: ZooKeeper
> Issue Type: Bug
> Components: quorum
> Affects Versions: 3.5.0
> Reporter: Timothy Ward
> Assignee: Flavio Junqueira
> Priority: Blocker
> Fix For: 3.5.2
>
> Attachments: ZOOKEEPER-2366.patch, ZOOKEEPER-2366.patch,
> ZOOKEEPER-2366.patch, ZOOKEEPER-2366.patch, ZOOKEEPER-2366.patch,
> ZOOKEEPER-2366.patch, zookeeper.patch
>
>
> The NIOServerCnxnFactory reconfigure method can leak server sockets, and
> hence make ports unusable until the JVM restarts:
> The first line of the method takes a reference to the current
> ServerSocketChannel and then the next line replaces it. The subsequent
> interactions with the server socket can fail (for example if the
> reconfiguration tries to bind to an in-use port). If they fail *before* the
> call to oldSS.close() then oldSS is *never* closed. This holds that port open
> forever, and prevents the user from rolling back to the previous port!
> The code from reconfigure is shown below:
> ServerSocketChannel oldSS = ss;
> try {
> this.ss = ServerSocketChannel.open();
> ss.socket().setReuseAddress(true);
> LOG.info("binding to port " + addr);
> ss.socket().bind(addr);
> ss.configureBlocking(false);
> acceptThread.setReconfiguring();
> oldSS.close();
> acceptThread.wakeupSelector();
> try {
> acceptThread.join();
> } catch (InterruptedException e) {
> LOG.error("Error joining old acceptThread when
> reconfiguring client port " + e.getMessage());
> }
> acceptThread = new AcceptThread(ss, addr, selectorThreads);
> acceptThread.start();
> } catch(IOException e) {
> LOG.error("Error reconfiguring client port to " + addr + " " +
> e.getMessage());
> }
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
