Rémy,
On 9/19/23 08:24, r...@apache.org wrote:
This is an automated email from the ASF dual-hosted git repository.
remm pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/main by this push:
new ceaa4fd26d Tighten up readNotify and writeNotify
ceaa4fd26d is described below
commit ceaa4fd26d2c6999c980d5e5acb63989393150ab
Author: remm <r...@apache.org>
AuthorDate: Tue Sep 19 14:24:15 2023 +0200
Tighten up readNotify and writeNotify
The two flags are important, so it is best to fully protect reading and
updating them. Hopefully will not cause any regressions.
Found by coverity.
---
java/org/apache/tomcat/util/net/Nio2Endpoint.java | 40 +++++++++++++++++------
webapps/docs/changelog.xml | 4 +++
2 files changed, 34 insertions(+), 10 deletions(-)
diff --git a/java/org/apache/tomcat/util/net/Nio2Endpoint.java
b/java/org/apache/tomcat/util/net/Nio2Endpoint.java
index 610eab9dff..30f3ec7d8d 100644
--- a/java/org/apache/tomcat/util/net/Nio2Endpoint.java
+++ b/java/org/apache/tomcat/util/net/Nio2Endpoint.java
@@ -614,8 +614,9 @@ public class Nio2Endpoint extends
AbstractNetworkChannelEndpoint<Nio2Channel,Asy
if (log.isDebugEnabled()) {
log.debug("Socket: [" + Nio2SocketWrapper.this + "], Interest:
[" + readInterest + "]");
}
- readNotify = false;
+ boolean notify = false;
synchronized (readCompletionHandler) {
+ readNotify = false;
if (nBytes.intValue() < 0) {
failed(new EOFException(), attachment);
} else {
@@ -628,8 +629,9 @@ public class Nio2Endpoint extends
AbstractNetworkChannelEndpoint<Nio2Channel,Asy
}
readInterest = false;
}
+ notify = readNotify;
}
- if (readNotify) {
+ if (notify) {
getEndpoint().processSocket(Nio2SocketWrapper.this,
SocketEvent.OPEN_READ, false);
}
}
@@ -657,9 +659,9 @@ public class Nio2Endpoint extends
AbstractNetworkChannelEndpoint<Nio2Channel,Asy
this.writeCompletionHandler = new CompletionHandler<>() {
@Override
public void completed(Integer nBytes, ByteBuffer attachment) {
- writeNotify = false;
boolean notify = false;
synchronized (writeCompletionHandler) {
+ writeNotify = false;
if (nBytes.intValue() < 0) {
failed(new
EOFException(sm.getString("iob.failedwrite")), attachment);
} else if (!nonBlockingWriteBuffer.isEmpty()) {
@@ -711,9 +713,9 @@ public class Nio2Endpoint extends
AbstractNetworkChannelEndpoint<Nio2Channel,Asy
gatheringWriteCompletionHandler = new CompletionHandler<>() {
@Override
public void completed(Long nBytes, ByteBuffer[] attachment) {
- writeNotify = false;
boolean notify = false;
synchronized (writeCompletionHandler) {
+ writeNotify = false;
if (nBytes.longValue() < 0) {
failed(new
EOFException(sm.getString("iob.failedwrite")), attachment);
} else if (!nonBlockingWriteBuffer.isEmpty() ||
buffersArrayHasRemaining(attachment, 0, attachment.length)) {
@@ -830,7 +832,12 @@ public class Nio2Endpoint extends
AbstractNetworkChannelEndpoint<Nio2Channel,Asy
throw new IOException(sm.getString("socket.closed"));
}
- if (!readNotify) {
+ boolean notify = false;
+ synchronized (readCompletionHandler) {
+ notify = readNotify;
+ }
+
+ if (!notify) {
if (block) {
try {
readPending.acquire();
@@ -850,7 +857,9 @@ public class Nio2Endpoint extends
AbstractNetworkChannelEndpoint<Nio2Channel,Asy
int nRead = populateReadBuffer(b, off, len);
if (nRead > 0) {
// The code that was notified is now reading its data
- readNotify = false;
+ synchronized (readCompletionHandler) {
+ readNotify = false;
+ }
// This may be sufficient to complete the request and we
// don't want to trigger another read since if there is no
// more data to read and this request takes a while to
@@ -887,7 +896,12 @@ public class Nio2Endpoint extends
AbstractNetworkChannelEndpoint<Nio2Channel,Asy
throw new IOException(sm.getString("socket.closed"));
}
- if (!readNotify) {
+ boolean notify = false;
+ synchronized (readCompletionHandler) {
+ notify = readNotify;
+ }
+
+ if (!notify) {
if (block) {
try {
readPending.acquire();
@@ -907,7 +921,9 @@ public class Nio2Endpoint extends
AbstractNetworkChannelEndpoint<Nio2Channel,Asy
int nRead = populateReadBuffer(to);
if (nRead > 0) {
// The code that was notified is now reading its data
- readNotify = false;
+ synchronized (readCompletionHandler) {
+ readNotify = false;
+ }
// This may be sufficient to complete the request and we
// don't want to trigger another read since if there is no
// more data to read and this request takes a while to
@@ -1025,10 +1041,14 @@ public class Nio2Endpoint extends
AbstractNetworkChannelEndpoint<Nio2Channel,Asy
protected void start() {
if (read) {
// Disable any regular read notifications caused by
registerReadInterest
- readNotify = true;
+ synchronized (readCompletionHandler) {
+ readNotify = true;
+ }
} else {
// Disable any regular write notifications caused by
registerWriteInterest
- writeNotify = true;
+ synchronized (writeCompletionHandler) {
+ writeNotify = true;
+ }
}
Nio2Endpoint.startInline();
try {
Are we trying to avoid synchronized() these days to better-support
Virtual Threads? These appear to be very small blocks so maybe it
doesn't matter but I figured I would ask.
-chris
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
