This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch 9.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/9.0.x by this push:
new 879d81be35 Refactor to remove code paths that could result in lost
notifications
879d81be35 is described below
commit 879d81be35fdbe3e27fbf726a049a1827c827ab8
Author: Mark Thomas <[email protected]>
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 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 | 9 +++
2 files changed, 47 insertions(+), 39 deletions(-)
diff --git a/java/org/apache/tomcat/util/net/NioEndpoint.java
b/java/org/apache/tomcat/util/net/NioEndpoint.java
index 915a057297..e7491ea3f9 100644
--- a/java/org/apache/tomcat/util/net/NioEndpoint.java
+++ b/java/org/apache/tomcat/util/net/NioEndpoint.java
@@ -1317,26 +1317,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
}
}
}
@@ -1428,33 +1427,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 f897437162..cbc85b9147 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -117,6 +117,15 @@
</fix>
</changelog>
</subsection>
+ <subsection name="Coyote">
+ <changelog>
+ <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>
</section>
<section name="Tomcat 9.0.78 (remm)" rtext="release in progress">
<subsection name="Other">
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
