Make NIO client initialization thread safe#106
Conversation
88ff3e2 to
6cf2245
Compare
ansell
left a comment
There was a problem hiding this comment.
Comment on the AtomicBoolean pattern usage here.
|
|
||
| private static void startTcp() throws IOException { | ||
| if (run) { | ||
| if (!run.compareAndSet(false, true)) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
6cf2245 to
33944cd
Compare
33944cd to
ff6fb9a
Compare
|
@ansell @matsli @MichaelKunze any further comments? |
ansell
left a comment
There was a problem hiding this comment.
This version potentially has side-effects still.
| } catch (IOException e) { | ||
| log.error("A selection operation failed", e); | ||
| } catch (ClosedSelectorException e) { | ||
| // ignore |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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);
}
}
There was a problem hiding this comment.
If
selector.close()returns anIOException(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(); |
There was a problem hiding this comment.
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;
}
There was a problem hiding this comment.
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
selectorThreadstatic field which is referenced in theclosemethod.
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
AtomicBooleanapproach to restrict entries (without global synchronisation in this step), and use aCountDownLatchwith 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.
Close #104