Skip to content

Run NIO close in the selector thread to prevent CME #381

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

Merged
ibauersachs merged 1 commit into from
Jun 28, 2025
Merged

Run NIO close in the selector thread to prevent CME #381

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions sonar-project.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ sonar.organization=dnsjava
sonar.host.url=https://sonarcloud.io
sonar.java.source=8
sonar.coverage.jacoco.xmlReportPaths=target/site/jacoco/jacoco.xml
sonar.scanner.skipJreProvisioning=true

sonar.issue.ignore.multicriteria=S106,S107,S120,S1948,S2160

Expand Down
139 changes: 99 additions & 40 deletions src/main/java/org/xbill/DNS/NioClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import java.nio.channels.SelectionKey;
import java.nio.channels.Selector;
import java.util.Iterator;
import java.util.function.Consumer;
import lombok.AccessLevel;
import lombok.NoArgsConstructor;
import lombok.extern.slf4j.Slf4j;
Expand All @@ -24,9 +25,9 @@
* <p>The following configuration parameter is available:
*
* <dl>
* <dt>dnsjava.nio.selector_timeout
* <dt>{@value SELECTOR_TIMEOUT_PROPERTY}
* <dd>Set selector timeout in milliseconds. Default/Max 1000, Min 1.
* <dt>dnsjava.nio.register_shutdown_hook
* <dt>{@value REGISTER_SHUTDOWN_HOOK_PROPERTY}
* <dd>Register Shutdown Hook termination of NIO. Default True.
* </dl>
*
Expand All @@ -35,24 +36,32 @@
@Slf4j
@NoArgsConstructor(access = AccessLevel.NONE)
public abstract class NioClient {
static final String SELECTOR_TIMEOUT_PROPERTY = "dnsjava.nio.selector_timeout";
static final String REGISTER_SHUTDOWN_HOOK_PROPERTY = "dnsjava.nio.register_shutdown_hook";
private static final Object NIO_CLIENT_LOCK = new Object();

/** Packet logger, if available. */
private static PacketLogger packetLogger = null;

private static final Runnable[] TIMEOUT_TASKS = new Runnable[2];
private static final Runnable[] REGISTRATIONS_TASKS = new Runnable[2];

@SuppressWarnings("unchecked")
private static final Consumer<Selector>[] REGISTRATIONS_TASKS = new Consumer[2];

private static final Runnable[] CLOSE_TASKS = new Runnable[2];
private static Thread selectorThread;
private static Thread closeThread;
private static volatile Selector selector;
private static volatile boolean run;
private static volatile boolean closeDone;

interface KeyProcessor {
void processReadyKey(SelectionKey key);
}

static Selector selector() throws IOException {
if (selector == null) {
synchronized (NioClient.class) {
synchronized (NIO_CLIENT_LOCK) {
if (selector == null) {
selector = Selector.open();
log.debug("Starting dnsjava NIO selector thread");
Expand All @@ -63,8 +72,7 @@
selectorThread.start();
closeThread = new Thread(() -> close(true));
closeThread.setName("dnsjava NIO shutdown hook");
if (Boolean.parseBoolean(
System.getProperty("dnsjava.nio.register_shutdown_hook", "true"))) {
if (Boolean.parseBoolean(System.getProperty(REGISTER_SHUTDOWN_HOOK_PROPERTY, "true"))) {
Runtime.getRuntime().addShutdownHook(closeThread);
}
}
Expand All @@ -74,13 +82,23 @@
return selector;
}

/** Shutdown the network I/O used by the {@link SimpleResolver}. */
/**
* Shutdown the network I/O used by the {@link SimpleResolver}.
*
* @implNote Does not wait until the selector thread has stopped. But users may immediately start
* using the {@link NioClient} again.
* @since 3.4.0
*/
public static void close() {
close(false);
}

private static void close(boolean fromHook) {
Copy link
Contributor

@bhaveshthakker bhaveshthakker Jun 14, 2025

Choose a reason for hiding this comment

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

Now, since the close() function is not actually closing the selector, can we rename this to either of markForClose(), markForShutdown(), scheduleClose(), etc. Close() is misleading!

Also, now since the logic for close is not very sequentially straightforward, possibly we can add a comment over the function what this function does and how selector will read the flag later to actually close it

Copy link
Member Author

@ibauersachs ibauersachs Jun 15, 2025

Choose a reason for hiding this comment

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

Now, since the close() function is not actually closing the selector, can we rename this to either of markForClose(), markForShutdown(), scheduleClose(), etc. Close() is misleading!

No, because close() is part of the public API and dnsjava follows SemVer.

What we could do is to remove the local* variables and close directly inside the lock. I don't quite see the benefit of this though, since calling selector() is possible again once close() has returned.

Copy link
Contributor

@bhaveshthakker bhaveshthakker Jun 15, 2025

Choose a reason for hiding this comment

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

Ahh yes! closer() is public!! Well then adding a javadoc would suffice!

run = false;
Selector localSelector = selector;
if (localSelector != null) {
selector.wakeup();
}

if (!fromHook) {
try {
Expand All @@ -90,40 +108,26 @@
}
}

try {
runTasks(CLOSE_TASKS);
} catch (Exception e) {
log.warn("Failed to execute shutdown task, ignoring and continuing close", e);
}

Selector localSelector = selector;
Thread localSelectorThread = selectorThread;
synchronized (NioClient.class) {
selector = null;
selectorThread = null;
closeThread = null;
if (localSelector == null) {
// Prevent hanging when close() was called without starting
return;
}

if (localSelector != null) {
localSelector.wakeup();
synchronized (NIO_CLIENT_LOCK) {
try {
localSelector.close();
} catch (IOException e) {
log.warn("Failed to properly close selector, ignoring and continuing close", e);
}
}

if (localSelectorThread != null) {
try {
localSelectorThread.join();
while (!closeDone) {
NIO_CLIENT_LOCK.wait();
}
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
} finally {
closeDone = false;
}
}
}

static void runSelector() {
int timeout = Integer.getInteger("dnsjava.nio.selector_timeout", 1000);
int timeout = Integer.getInteger(SELECTOR_TIMEOUT_PROPERTY, 1000);

if (timeout <= 0 || timeout > 1000) {
throw new IllegalArgumentException("Invalid selector_timeout, must be between 1 and 1000");
Expand All @@ -136,7 +140,7 @@
}

if (run) {
runTasks(REGISTRATIONS_TASKS);
runRegistrationTasks();
processReadyKeys();
}
} catch (IOException e) {
Expand All @@ -145,30 +149,74 @@
// ignore
}
}

runClose();

Check warning on line 153 in src/main/java/org/xbill/DNS/NioClient.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/xbill/DNS/NioClient.java#L153 

Added line #L153 was not covered by tests
log.debug("dnsjava NIO selector thread stopped");
}

static synchronized void setTimeoutTask(Runnable r, boolean isTcpClient) {
private static void runClose() {
try {
runTasks(CLOSE_TASKS);
} catch (Exception e) {
log.warn("Failed to execute shutdown task, ignoring and continuing close", e);

Check warning on line 161 in src/main/java/org/xbill/DNS/NioClient.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/xbill/DNS/NioClient.java#L160-L161 

Added lines #L160 - L161 were not covered by tests
}

Selector localSelector = selector;
Thread localSelectorThread = selectorThread;
synchronized (NIO_CLIENT_LOCK) {
selector = null;
selectorThread = null;
closeThread = null;
closeDone = true;
NIO_CLIENT_LOCK.notifyAll();
}

if (localSelector != null) {
try {
localSelector.close();
} catch (IOException e) {
log.warn("Failed to properly close selector, ignoring and continuing close", e);

Check warning on line 178 in src/main/java/org/xbill/DNS/NioClient.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/xbill/DNS/NioClient.java#L177-L178 

Added lines #L177 - L178 were not covered by tests
}
}

if (localSelectorThread != null) {
try {
localSelectorThread.join();
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
}

Check warning on line 187 in src/main/java/org/xbill/DNS/NioClient.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/xbill/DNS/NioClient.java#L184-L187 

Added lines #L184 - L187 were not covered by tests
}
}

Check warning on line 189 in src/main/java/org/xbill/DNS/NioClient.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/xbill/DNS/NioClient.java#L189 

Added line #L189 was not covered by tests

static void setTimeoutTask(Runnable r, boolean isTcpClient) {
addTask(TIMEOUT_TASKS, r, isTcpClient);
}

static synchronized void setRegistrationsTask(Runnable r, boolean isTcpClient) {
addTask(REGISTRATIONS_TASKS, r, isTcpClient);
static void setRegistrationsTask(Consumer<Selector> r, boolean isTcpClient) {
addRegistrationTask(r, isTcpClient);
}

static synchronized void setCloseTask(Runnable r, boolean isTcpClient) {
static void setCloseTask(Runnable r, boolean isTcpClient) {
addTask(CLOSE_TASKS, r, isTcpClient);
}

private static void addTask(Runnable[] closeTasks, Runnable r, boolean isTcpClient) {
private static void addTask(Runnable[] tasks, Runnable r, boolean isTcpClient) {
if (isTcpClient) {
closeTasks[0] = r;
tasks[0] = r;
} else {
closeTasks[1] = r;
tasks[1] = r;
}
}

private static synchronized void runTasks(Runnable[] runnables) {
private static void addRegistrationTask(Consumer<Selector> r, boolean isTcpClient) {
if (isTcpClient) {
REGISTRATIONS_TASKS[0] = r;
} else {
REGISTRATIONS_TASKS[1] = r;
}
}

private static void runTasks(Runnable[] runnables) {
Runnable r0 = runnables[0];
if (r0 != null) {
r0.run();
Expand All @@ -179,6 +227,17 @@
}
}

private static void runRegistrationTasks() {
Consumer<Selector> r0 = REGISTRATIONS_TASKS[0];
if (r0 != null) {
r0.accept(selector);
}
Consumer<Selector> r1 = REGISTRATIONS_TASKS[1];
if (r1 != null) {
r1.accept(selector);
}
}

private static void processReadyKeys() {
Iterator<SelectionKey> it = selector.selectedKeys().iterator();
while (it.hasNext()) {
Expand Down
3 changes: 1 addition & 2 deletions src/main/java/org/xbill/DNS/NioTcpClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,15 +32,14 @@ final class NioTcpClient extends NioClient implements TcpIoClient {
setCloseTask(this::closeTcp, true);
}

private void processPendingRegistrations() {
private void processPendingRegistrations(Selector selector) {
while (!registrationQueue.isEmpty()) {
ChannelState state = registrationQueue.poll();
if (state == null) {
continue;
}

try {
final Selector selector = selector();
if (!state.channel.isConnected()) {
state.channel.register(selector, SelectionKey.OP_CONNECT, state);
} else {
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/org/xbill/DNS/NioUdpClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@
setCloseTask(this::closeUdp, false);
}

private void processPendingRegistrations() {
private void processPendingRegistrations(Selector selector) {
while (!registrationQueue.isEmpty()) {
Transaction t = registrationQueue.poll();
if (t == null) {
Expand All @@ -63,7 +63,7 @@

try {
log.trace("Registering OP_READ for transaction with id {}", t.id);
t.channel.register(selector(), SelectionKey.OP_READ, t);
t.channel.register(selector, SelectionKey.OP_READ, t);

Check warning on line 66 in src/main/java/org/xbill/DNS/NioUdpClient.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/xbill/DNS/NioUdpClient.java#L66 

Added line #L66 was not covered by tests
t.send();
} catch (IOException e) {
t.completeExceptionally(e);
Expand Down
Loading
Loading