Skip to content

Commit 6e700d8

Browse files
ibauersachsbhaveshthakker
ibauersachs
and
bhaveshthakker
committed
Run NIO close in the selector thread to prevent CME
Closes #379 Closes #380 Co-authored-by: bhaveshthakker <bhaveshthakker111@gmail.com>
1 parent 59eb83b commit 6e700d8

File tree

5 files changed

+245
-48
lines changed

5 files changed

+245
-48
lines changed

‎sonar-project.properties‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ sonar.organization=dnsjava
33
sonar.host.url=https://sonarcloud.io
44
sonar.java.source=8
55
sonar.coverage.jacoco.xmlReportPaths=target/site/jacoco/jacoco.xml
6+
sonar.scanner.skipJreProvisioning=true
67

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

‎src/main/java/org/xbill/DNS/NioClient.java‎

Lines changed: 99 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
import java.nio.channels.SelectionKey;
1111
import java.nio.channels.Selector;
1212
import java.util.Iterator;
13+
import java.util.function.Consumer;
1314
import lombok.AccessLevel;
1415
import lombok.NoArgsConstructor;
1516
import lombok.extern.slf4j.Slf4j;
@@ -24,9 +25,9 @@
2425
* <p>The following configuration parameter is available:
2526
*
2627
* <dl>
27-
* <dt>dnsjava.nio.selector_timeout
28+
* <dt>{@value SELECTOR_TIMEOUT_PROPERTY}
2829
* <dd>Set selector timeout in milliseconds. Default/Max 1000, Min 1.
29-
* <dt>dnsjava.nio.register_shutdown_hook
30+
* <dt>{@value REGISTER_SHUTDOWN_HOOK_PROPERTY}
3031
* <dd>Register Shutdown Hook termination of NIO. Default True.
3132
* </dl>
3233
*
@@ -35,24 +36,32 @@
3536
@Slf4j
3637
@NoArgsConstructor(access = AccessLevel.NONE)
3738
public abstract class NioClient {
39+
static final String SELECTOR_TIMEOUT_PROPERTY = "dnsjava.nio.selector_timeout";
40+
static final String REGISTER_SHUTDOWN_HOOK_PROPERTY = "dnsjava.nio.register_shutdown_hook";
41+
private static final Object NIO_CLIENT_LOCK = new Object();
42+
3843
/** Packet logger, if available. */
3944
private static PacketLogger packetLogger = null;
4045

4146
private static final Runnable[] TIMEOUT_TASKS = new Runnable[2];
42-
private static final Runnable[] REGISTRATIONS_TASKS = new Runnable[2];
47+
48+
@SuppressWarnings("unchecked")
49+
private static final Consumer<Selector>[] REGISTRATIONS_TASKS = new Consumer[2];
50+
4351
private static final Runnable[] CLOSE_TASKS = new Runnable[2];
4452
private static Thread selectorThread;
4553
private static Thread closeThread;
4654
private static volatile Selector selector;
4755
private static volatile boolean run;
56+
private static volatile boolean closeDone;
4857

4958
interface KeyProcessor {
5059
void processReadyKey(SelectionKey key);
5160
}
5261

5362
static Selector selector() throws IOException {
5463
if (selector == null) {
55-
synchronized (NioClient.class) {
64+
synchronized (NIO_CLIENT_LOCK) {
5665
if (selector == null) {
5766
selector = Selector.open();
5867
log.debug("Starting dnsjava NIO selector thread");
@@ -63,8 +72,7 @@ static Selector selector() throws IOException {
6372
selectorThread.start();
6473
closeThread = new Thread(() -> close(true));
6574
closeThread.setName("dnsjava NIO shutdown hook");
66-
if (Boolean.parseBoolean(
67-
System.getProperty("dnsjava.nio.register_shutdown_hook", "true"))) {
75+
if (Boolean.parseBoolean(System.getProperty(REGISTER_SHUTDOWN_HOOK_PROPERTY, "true"))) {
6876
Runtime.getRuntime().addShutdownHook(closeThread);
6977
}
7078
}
@@ -74,13 +82,23 @@ static Selector selector() throws IOException {
7482
return selector;
7583
}
7684

77-
/** Shutdown the network I/O used by the {@link SimpleResolver}. */
85+
/**
86+
* Shutdown the network I/O used by the {@link SimpleResolver}.
87+
*
88+
* @implNote Does not wait until the selector thread has stopped. But users may immediately start
89+
* using the {@link NioClient} again.
90+
* @since 3.4.0
91+
*/
7892
public static void close() {
7993
close(false);
8094
}
8195

8296
private static void close(boolean fromHook) {
8397
run = false;
98+
Selector localSelector = selector;
99+
if (localSelector != null) {
100+
selector.wakeup();
101+
}
84102

85103
if (!fromHook) {
86104
try {
@@ -90,40 +108,26 @@ private static void close(boolean fromHook) {
90108
}
91109
}
92110

93-
try {
94-
runTasks(CLOSE_TASKS);
95-
} catch (Exception e) {
96-
log.warn("Failed to execute shutdown task, ignoring and continuing close", e);
97-
}
98-
99-
Selector localSelector = selector;
100-
Thread localSelectorThread = selectorThread;
101-
synchronized (NioClient.class) {
102-
selector = null;
103-
selectorThread = null;
104-
closeThread = null;
111+
if (localSelector == null) {
112+
// Prevent hanging when close() was called without starting
113+
return;
105114
}
106115

107-
if (localSelector != null) {
108-
localSelector.wakeup();
116+
synchronized (NIO_CLIENT_LOCK) {
109117
try {
110-
localSelector.close();
111-
} catch (IOException e) {
112-
log.warn("Failed to properly close selector, ignoring and continuing close", e);
113-
}
114-
}
115-
116-
if (localSelectorThread != null) {
117-
try {
118-
localSelectorThread.join();
118+
while (!closeDone) {
119+
NIO_CLIENT_LOCK.wait();
120+
}
119121
} catch (InterruptedException e) {
120122
Thread.currentThread().interrupt();
123+
} finally {
124+
closeDone = false;
121125
}
122126
}
123127
}
124128

125129
static void runSelector() {
126-
int timeout = Integer.getInteger("dnsjava.nio.selector_timeout", 1000);
130+
int timeout = Integer.getInteger(SELECTOR_TIMEOUT_PROPERTY, 1000);
127131

128132
if (timeout <= 0 || timeout > 1000) {
129133
throw new IllegalArgumentException("Invalid selector_timeout, must be between 1 and 1000");
@@ -136,7 +140,7 @@ static void runSelector() {
136140
}
137141

138142
if (run) {
139-
runTasks(REGISTRATIONS_TASKS);
143+
runRegistrationTasks();
140144
processReadyKeys();
141145
}
142146
} catch (IOException e) {
@@ -145,30 +149,74 @@ static void runSelector() {
145149
// ignore
146150
}
147151
}
152+
153+
runClose();
148154
log.debug("dnsjava NIO selector thread stopped");
149155
}
150156

151-
static synchronized void setTimeoutTask(Runnable r, boolean isTcpClient) {
157+
private static void runClose() {
158+
try {
159+
runTasks(CLOSE_TASKS);
160+
} catch (Exception e) {
161+
log.warn("Failed to execute shutdown task, ignoring and continuing close", e);
162+
}
163+
164+
Selector localSelector = selector;
165+
Thread localSelectorThread = selectorThread;
166+
synchronized (NIO_CLIENT_LOCK) {
167+
selector = null;
168+
selectorThread = null;
169+
closeThread = null;
170+
closeDone = true;
171+
NIO_CLIENT_LOCK.notifyAll();
172+
}
173+
174+
if (localSelector != null) {
175+
try {
176+
localSelector.close();
177+
} catch (IOException e) {
178+
log.warn("Failed to properly close selector, ignoring and continuing close", e);
179+
}
180+
}
181+
182+
if (localSelectorThread != null) {
183+
try {
184+
localSelectorThread.join();
185+
} catch (InterruptedException e) {
186+
Thread.currentThread().interrupt();
187+
}
188+
}
189+
}
190+
191+
static void setTimeoutTask(Runnable r, boolean isTcpClient) {
152192
addTask(TIMEOUT_TASKS, r, isTcpClient);
153193
}
154194

155-
static synchronized void setRegistrationsTask(Runnable r, boolean isTcpClient) {
156-
addTask(REGISTRATIONS_TASKS, r, isTcpClient);
195+
static void setRegistrationsTask(Consumer<Selector> r, boolean isTcpClient) {
196+
addRegistrationTask(r, isTcpClient);
157197
}
158198

159-
static synchronized void setCloseTask(Runnable r, boolean isTcpClient) {
199+
static void setCloseTask(Runnable r, boolean isTcpClient) {
160200
addTask(CLOSE_TASKS, r, isTcpClient);
161201
}
162202

163-
private static void addTask(Runnable[] closeTasks, Runnable r, boolean isTcpClient) {
203+
private static void addTask(Runnable[] tasks, Runnable r, boolean isTcpClient) {
164204
if (isTcpClient) {
165-
closeTasks[0] = r;
205+
tasks[0] = r;
166206
} else {
167-
closeTasks[1] = r;
207+
tasks[1] = r;
168208
}
169209
}
170210

171-
private static synchronized void runTasks(Runnable[] runnables) {
211+
private static void addRegistrationTask(Consumer<Selector> r, boolean isTcpClient) {
212+
if (isTcpClient) {
213+
REGISTRATIONS_TASKS[0] = r;
214+
} else {
215+
REGISTRATIONS_TASKS[1] = r;
216+
}
217+
}
218+
219+
private static void runTasks(Runnable[] runnables) {
172220
Runnable r0 = runnables[0];
173221
if (r0 != null) {
174222
r0.run();
@@ -179,6 +227,17 @@ private static synchronized void runTasks(Runnable[] runnables) {
179227
}
180228
}
181229

230+
private static void runRegistrationTasks() {
231+
Consumer<Selector> r0 = REGISTRATIONS_TASKS[0];
232+
if (r0 != null) {
233+
r0.accept(selector);
234+
}
235+
Consumer<Selector> r1 = REGISTRATIONS_TASKS[1];
236+
if (r1 != null) {
237+
r1.accept(selector);
238+
}
239+
}
240+
182241
private static void processReadyKeys() {
183242
Iterator<SelectionKey> it = selector.selectedKeys().iterator();
184243
while (it.hasNext()) {

‎src/main/java/org/xbill/DNS/NioTcpClient.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,15 +32,14 @@ final class NioTcpClient extends NioClient implements TcpIoClient {
3232
setCloseTask(this::closeTcp, true);
3333
}
3434

35-
private void processPendingRegistrations() {
35+
private void processPendingRegistrations(Selector selector) {
3636
while (!registrationQueue.isEmpty()) {
3737
ChannelState state = registrationQueue.poll();
3838
if (state == null) {
3939
continue;
4040
}
4141

4242
try {
43-
final Selector selector = selector();
4443
if (!state.channel.isConnected()) {
4544
state.channel.register(selector, SelectionKey.OP_CONNECT, state);
4645
} else {

‎src/main/java/org/xbill/DNS/NioUdpClient.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ final class NioUdpClient extends NioClient implements UdpIoClient {
5454
setCloseTask(this::closeUdp, false);
5555
}
5656

57-
private void processPendingRegistrations() {
57+
private void processPendingRegistrations(Selector selector) {
5858
while (!registrationQueue.isEmpty()) {
5959
Transaction t = registrationQueue.poll();
6060
if (t == null) {
@@ -63,7 +63,7 @@ private void processPendingRegistrations() {
6363

6464
try {
6565
log.trace("Registering OP_READ for transaction with id {}", t.id);
66-
t.channel.register(selector(), SelectionKey.OP_READ, t);
66+
t.channel.register(selector, SelectionKey.OP_READ, t);
6767
t.send();
6868
} catch (IOException e) {
6969
t.completeExceptionally(e);

0 commit comments

Comments
 (0)