On Thu, Jun 6, 2019 at 10:46 PM <ma...@apache.org> wrote: > This is an automated email from the ASF dual-hosted git repository. > > markt pushed a commit to branch master > in repository https://gitbox.apache.org/repos/asf/tomcat.git > > commit e967bbc7f106f8ec5384a294d98d5cbc44b474f1 > Author: Mark Thomas <ma...@apache.org> > AuthorDate: Thu Jun 6 21:45:30 2019 +0100 > > Remove a source of potential deadlocks > > SocketWrapperBase.vectoredOperation acquires the (read|write) Semaphore > and then locks the SocketWrapper during completion before the Semaphore > is release. If the SocketWrapper is locked on some, but not all, paths > before acquiring the Semaphore, a deadlock becomes possible. > > Fixed by removing the initial lock on SocketWrapper since the > Semaphores > make it unnecessary. >
I wished it worked, as this sync lowers performance a lot, but unfortunately it doesn't. The lock ensures that HPACK behaves and that everything gets written in order, more or less. Details and test is in https://bz.apache.org/bugzilla/show_bug.cgi?id=61740 It fails again after removing the sync. Rémy > --- > .../apache/coyote/http2/Http2AsyncUpgradeHandler.java | 17 > +++++++---------- > webapps/docs/changelog.xml | 9 +++++++++ > 2 files changed, 16 insertions(+), 10 deletions(-) > > diff --git a/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java > b/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java > index 969bfda..8a57c53 100644 > --- a/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java > +++ b/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java > @@ -169,16 +169,13 @@ public class Http2AsyncUpgradeHandler extends > Http2UpgradeHandler { > @Override > void writeHeaders(Stream stream, int pushedStreamId, MimeHeaders > mimeHeaders, > boolean endOfStream, int payloadSize) throws IOException { > - // This ensures the Stream processing thread has control of the > socket. > - synchronized (socketWrapper) { > - AsyncHeaderFrameBuffers headerFrameBuffers = > (AsyncHeaderFrameBuffers) > - doWriteHeaders(stream, pushedStreamId, mimeHeaders, > endOfStream, payloadSize); > - if (headerFrameBuffers != null) { > - socketWrapper.write(BlockingMode.SEMI_BLOCK, > protocol.getWriteTimeout(), > - TimeUnit.MILLISECONDS, null, > SocketWrapperBase.COMPLETE_WRITE, > - applicationErrorCompletion, > headerFrameBuffers.bufs.toArray(BYTEBUFFER_ARRAY)); > - handleAsyncException(); > - } > + AsyncHeaderFrameBuffers headerFrameBuffers = > (AsyncHeaderFrameBuffers) > + doWriteHeaders(stream, pushedStreamId, mimeHeaders, > endOfStream, payloadSize); > + if (headerFrameBuffers != null) { > + socketWrapper.write(BlockingMode.SEMI_BLOCK, > protocol.getWriteTimeout(), > + TimeUnit.MILLISECONDS, null, > SocketWrapperBase.COMPLETE_WRITE, > + applicationErrorCompletion, > headerFrameBuffers.bufs.toArray(BYTEBUFFER_ARRAY)); > + handleAsyncException(); > } > if (endOfStream) { > stream.sentEndOfStream(); > diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml > index 7f55e3d..d65a1f3 100644 > --- a/webapps/docs/changelog.xml > +++ b/webapps/docs/changelog.xml > @@ -45,6 +45,15 @@ > issues do not "pop up" wrt. others). > --> > <section name="Tomcat 9.0.22 (markt)" rtext="in development"> > + <subsection name="Coyote"> > + <changelog> > + <fix> > + Remove a source of potential deadlocks when using HTTP/2 when the > + Connector is configured with <code>useAsyncIO</code> as > + <code>true</code>. (markt) > + </fix> > + </changelog> > + </subsection> > </section> > <section name="Tomcat 9.0.21 (markt)" rtext="release in progress"> > <subsection name="Catalina"> > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org > For additional commands, e-mail: dev-h...@tomcat.apache.org > >
