This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch 8.5.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/8.5.x by this push:
new a04db5ecbd Refactor to remove code paths that could result in lost
notifications
a04db5ecbd is described below
commit a04db5ecbdfc2e2ac8c5c07a59771782558ec2c9
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Thu Jul 6 13:25:14 2023 +0100
Refactor to remove code paths that could result in lost notifications
The scenario is considered unlikely but if, in the original code, the
Poller notified the [read|write]Lock after the read/write registration
but before execution entered the synchronized block, that notification
would be lost. This could lead to a timeout rather than the expected
read/write.
Flagged by Coverity Scan for issues with spurious wake-ups. That wasn't
an issue but this was.
---
java/org/apache/tomcat/util/net/NioEndpoint.java | 77 ++++++++++++------------
webapps/docs/changelog.xml | 5 ++
2 files changed, 43 insertions(+), 39 deletions(-)
diff --git a/java/org/apache/tomcat/util/net/NioEndpoint.java
b/java/org/apache/tomcat/util/net/NioEndpoint.java
index f5696b1284..fcf8c80ea8 100644
--- a/java/org/apache/tomcat/util/net/NioEndpoint.java
+++ b/java/org/apache/tomcat/util/net/NioEndpoint.java
@@ -1217,26 +1217,25 @@ public class NioEndpoint extends
AbstractJsseEndpoint<NioChannel,SocketChannel>
throw new SocketTimeoutException();
}
}
- n = getSocket().read(buffer);
- if (n == -1) {
- throw new EOFException();
- } else if (n == 0) {
- if (!readBlocking) {
- readBlocking = true;
- registerReadInterest();
- }
- synchronized (readLock) {
- if (readBlocking) {
- try {
- if (timeout > 0) {
- startNanos = System.nanoTime();
- readLock.wait(timeout);
- } else {
- readLock.wait();
- }
- } catch (InterruptedException e) {
- // Continue
+ synchronized (readLock) {
+ n = getSocket().read(buffer);
+ if (n == -1) {
+ throw new EOFException();
+ } else if (n == 0) {
+ // Ensure a spurious wake-up doesn't trigger a
duplicate registration
+ if (!readBlocking) {
+ readBlocking = true;
+ registerReadInterest();
+ }
+ try {
+ if (timeout > 0) {
+ startNanos = System.nanoTime();
+ readLock.wait(timeout);
+ } else {
+ readLock.wait();
}
+ } catch (InterruptedException e) {
+ // Continue
}
}
}
@@ -1328,33 +1327,33 @@ public class NioEndpoint extends
AbstractJsseEndpoint<NioChannel,SocketChannel>
throw previousIOException;
}
}
- n = getSocket().write(buffer);
- if (n == 0 && (buffer.hasRemaining() ||
getSocket().getOutboundRemaining() > 0)) {
+ synchronized (writeLock) {
+ n = getSocket().write(buffer);
// n == 0 could be an incomplete write but it could
also
// indicate that a previous incomplete write of the
// outbound buffer (for TLS) has now completed. Only
// block if there is still data to write.
- writeBlocking = true;
- registerWriteInterest();
- synchronized (writeLock) {
- if (writeBlocking) {
- try {
- if (timeout > 0) {
- startNanos = System.nanoTime();
- writeLock.wait(timeout);
- } else {
- writeLock.wait();
- }
- } catch (InterruptedException e) {
- // Continue
+ if (n == 0 && (buffer.hasRemaining() ||
getSocket().getOutboundRemaining() > 0)) {
+ // Ensure a spurious wake-up doesn't trigger a
duplicate registration
+ if (!writeBlocking) {
+ writeBlocking = true;
+ registerWriteInterest();
+ }
+ try {
+ if (timeout > 0) {
+ startNanos = System.nanoTime();
+ writeLock.wait(timeout);
+ } else {
+ writeLock.wait();
}
- writeBlocking = false;
+ } catch (InterruptedException e) {
+ // Continue
}
+ } else if (startNanos > 0) {
+ // If something was written, reset timeout
+ timeout = getWriteTimeout();
+ startNanos = 0;
}
- } else if (startNanos > 0) {
- // If something was written, reset timeout
- timeout = getWriteTimeout();
- startNanos = 0;
}
} while (buffer.hasRemaining() ||
getSocket().getOutboundRemaining() > 0);
} else {
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 78a08b6b57..5f67af67cd 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -144,6 +144,11 @@
based keys/certificates and logs the relevant information for each.
(markt)
</fix>
+ <fix>
+ Refactor blocking reads and writes for the NIO connector to remove
+ code paths that could allow a notification from the Poller to be missed
+ resuting in a timeout rather than the expected read or write. (markt)
+ </fix>
</changelog>
</subsection>
<subsection name="WebSocket">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
