Skip to content

Comments

Make NIO client initialization thread safe#106

Merged
ibauersachs merged 1 commit into3.0.xfrom
bugfix/startup
May 15, 2020
Merged

Make NIO client initialization thread safe#106
ibauersachs merged 1 commit into3.0.xfrom
bugfix/startup

Conversation

@ibauersachs
Copy link
Member

Close #104

@ibauersachs ibauersachs changed the base branch from master to 3.0.x May 6, 2020 19:27
Copy link

@ansell ansell left a comment

Choose a reason for hiding this comment

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

Comment on the AtomicBoolean pattern usage here.


private static void startTcp() throws IOException {
if (run) {
if (!run.compareAndSet(false, true)) {
Copy link

Choose a reason for hiding this comment

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

This type of AtomicBoolean check is only threadsafe if there is nowhere to set it back to true. By setting it back to true in closeTcp the atomicity guarantees are removed as someone could call closeTcp while this method is still executing (mutating is variables), and that would also allow someone else to get past this point in startTcp again.

Copy link
Member Author

Choose a reason for hiding this comment

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

closeTcp isn't actually called anywhere, but yes you're right. The issue that is still left is that another thread would assume the initialization is finished and could access not initialized variables. I'll work on that during the weekend.

@ibauersachs
Copy link
Member Author

@ansell @matsli @MichaelKunze any further comments?

Copy link

@ansell ansell left a comment

Choose a reason for hiding this comment

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

This version potentially has side-effects still.

} catch (IOException e) {
log.error("A selection operation failed", e);
} catch (ClosedSelectorException e) {
// ignore
Copy link

Choose a reason for hiding this comment

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

If the Selector is closed, there doesn't seem to be any reason to try the loop again. This should be a signal to close this daemon thread, and close the Client to also set run to false. The reasoning for that is if the client isn't closed it will go straight back to selector.select(1000), receive another ClosedSelectorException and come back here again, with no other action possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

The only possible way to get a ClosedSelectorException is when selector.close() has been called. The only time this is done is in close() in the shutdown hook, which by this time has run already set to false and the loop is never retried.
The only reason to catch this at all is to prevent throwing when a key is still being processed during shutdown.

selectorThread.join();
try {
selector.close();
selectorThread.join();
Copy link

Choose a reason for hiding this comment

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

If selector.close() returns an IOException (as its documentation says it could do), selectorThread.join() will not execute. It may be more ideal to use try-finally here to ensure both operations run.

In my code, I am typically very pedantic about close methods and use try-finally nested blocks around each statement that could possibly throw an exception, regardless of how rare the exception may be. Ie. something like,

  private static void close() {
    run = false;
    initialisationCompleted.await();
    try {
      try {
        for(Runnable nextCloseTask : closeTasks) {
          try {
            nextCloseTask.run();
          } catch (Exception closeTaskException) {
            log.warn("Close task failed: ", closeTaskException);
          }
        }
      } finally {
        try {
          timeoutTasks.clear();
        } finally {
          try {
            Selector selectorToClose = selector;
            if(selectorToClose != null) {
              selectorToClose.wakeup();
              selectorToClose.close();
            }
          } finally {
            Thread selectorThreadToJoin = selectorThread;
            if(selectorThreadToJoin != null) {
              selectorThreadToJoin.join();
            }
          }
        }
    } catch (InterruptedException | IOException e) {
      log.warn("Failed to properly shutdown", e);
    }
}

Copy link
Member Author

Choose a reason for hiding this comment

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

If selector.close() returns an IOException (as its documentation says it could do), selectorThread.join() will not execute. It may be more ideal to use try-finally here to ensure both operations run.

This is just intended to be nice during a normal all-is-nice shutdown. close is only called when the program ends and is not guaranteed to run at all. If closing the selector fails, okay, no matter. It is a daemon thread not holding any resources which will end anyway.

In my code, I am typically very pedantic about close methods and use try-finally nested blocks around each statement that could possibly throw an exception, regardless of how rare the exception may be. Ie. something like,

  private static void close() {
    run = false;
    initialisationCompleted.await();

The shutdown hook definitely should not wait for an initialization. But it is pointless anyway, as the initialization is definitely complete by the time this can be called (by the shutdown hook).

try {
  try {
    for(Runnable nextCloseTask : closeTasks) {
      try {
        nextCloseTask.run();
      } catch (Exception closeTaskException) {
        log.warn("Close task failed: ", closeTaskException);

The only possible exceptions to catch here are RuntimeExceptions, which I wouldn't really want to catch.

      }
    }
  } finally {
    try {
      timeoutTasks.clear();
    } finally {
      try {
        Selector selectorToClose = selector;
        if(selectorToClose != null) {

This is impossible. The selector is never null by the time close is executed (unless Selector.open() would return null, but then everything is broken).

          selectorToClose.wakeup();
          selectorToClose.close();
        }
      } finally {
        Thread selectorThreadToJoin = selectorThread;
        if(selectorThreadToJoin != null) {

This is impossible. The selectorThread is never null by the time close is executed.

          selectorThreadToJoin.join();
        }
      }
    }
} catch (InterruptedException | IOException e) {
  log.warn("Failed to properly shutdown", e);
}

}

if (selector == null) {
synchronized (Client.class) {
if (selector == null) {
selector = Selector.open();
Copy link

@ansell ansell May 9, 2020

Choose a reason for hiding this comment

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

This isn't the usual pattern for if-null/synchronized/if-null, as it sets the visible static field before the synchronised section is complete. The variable that is used as the check should not be set until the last instruction in the if block in this pattern. In this case, it could cause a very rare null condition for the selectorThread static field which is referenced in the close method.

A method I have used in the past for static initialisation is to use your previous AtomicBoolean approach to restrict entries (without global synchronisation in this step), and use a CountDownLatch with 1 token to prevent exits before the initialisation is complete. An example of that here could be:

private static final AtomicBoolean initialisationStarted - new AtomicBoolean(false);
private static final CountDownLatch initialisationComplete = new CountDownLatch(1);

static Selector selector() throws IOException {
  if(initialisationStarted.compareAndSet(false, true)) {
    try {
      selector = Selector.open();
      log.debug("Starting dnsjava NIO selector thread");
      selectorThread = new Thread(Client::runSelector);
      selectorThread.setDaemon(true);
      selectorThread.setName("dnsjava NIO selector");
      selectorThread.start();
      Thread closeThread = new Thread(Client::close);
      closeThread.setName("dnsjava NIO shutdown hook");
      Runtime.getRuntime().addShutdownHook(closeThread);
    } finally {
      initialisationCompleted.countDown();
    }
  }
  initialisationCompleted.await();
  return selector;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

This isn't the usual pattern for if-null/synchronized/if-null, as it sets the visible static field before the synchronised section is complete. The variable that is used as the check should not be set until the last instruction in the if block in this pattern. In this case, it could cause a very rare null condition for the selectorThread static field which is referenced in the close method.

I might be missing something, but how? close can only be called after selector is initialized. The worst that could happen here is an IOException when calling open(). But in this case neither the selector thread nor the shutdown hook will be registered, and everything is retried when either of the Nio[Tcp/Udp]Clients attempt to get the selector.
Also, having access to the selector before the selector thread is started or the shutdown hook registered does not matter, but is in fact this is wanted. Queuing of commands to process can and shall be done while the selector thread starts up.

A method I have used in the past for static initialisation is to use your previous AtomicBoolean approach to restrict entries (without global synchronisation in this step), and use a CountDownLatch with 1 token to prevent exits before the initialisation is complete. An example of that here could be:

private static final AtomicBoolean initialisationStarted - new AtomicBoolean(false);
private static final CountDownLatch initialisationComplete = new CountDownLatch(1);

static Selector selector() throws IOException {
  if(initialisationStarted.compareAndSet(false, true)) {
    try {
      selector = Selector.open();
      log.debug("Starting dnsjava NIO selector thread");
      selectorThread = new Thread(Client::runSelector);
      selectorThread.setDaemon(true);
      selectorThread.setName("dnsjava NIO selector");
      selectorThread.start();
      Thread closeThread = new Thread(Client::close);
      closeThread.setName("dnsjava NIO shutdown hook");
      Runtime.getRuntime().addShutdownHook(closeThread);
    } finally {
      initialisationCompleted.countDown();
    }
  }
  initialisationCompleted.await();
  return selector;
}

If selector.open() fails with an IOException, there will never be a retry and further calls to the method will return null.

@ibauersachs ibauersachs merged commit 109ca7d into 3.0.x May 15, 2020
@ibauersachs ibauersachs deleted the bugfix/startup branch May 15, 2020 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Race condition in NioUdpClient.startUdp()

2 participants