On Tue, Sep 19, 2023 at 2:31 PM Christopher Schultz <ch...@christopherschultz.net> wrote: > > 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.
These are useful. But the concurrency level is not very high, so I think there's no real impact. Also this is not the default (instead NIO has been checked for virtual threads). Rémy > -chris > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org > For additional commands, e-mail: dev-h...@tomcat.apache.org > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
