Eclipse
/
jetty.project
mirror of https://github.com/jetty/jetty.project.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

fix deadlock between HttpInput's lock and BlockingContentProducer's semaphore 

Signed-off-by: Ludovic Orban <lorban@bitronix.be>
This commit is contained in:
Ludovic Orban 2021-02-11 15:33:54 +01:00
parent 14108c8e58
commit 59b397b1a0
6 changed files with 54 additions and 16 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContentProducer.java
View File

@ -17,11 +17,12 @@ import java.util.concurrent.TimeUnit;
import org.eclipse.jetty.http.BadMessageException;
import org.eclipse.jetty.http.HttpStatus;
import org.eclipse.jetty.util.thread.AutoLock;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
* Non-blocking {@link ContentProducer} implementation. Calling {@link #nextContent()} will never block
* Non-blocking {@link ContentProducer} implementation. Calling {@link ContentProducer#nextContent(AutoLock)} will never block
* but will return null when there is no available content.
*/
class AsyncContentProducer implements ContentProducer
@ -192,8 +193,11 @@ class AsyncContentProducer implements ContentProducer
}
@Override
public HttpInput.Content nextContent()
public HttpInput.Content nextContent(AutoLock lock)
{
if (!lock.isHeldByCurrentThread())
throw new IllegalStateException("nextContent must be called with the lock held");
HttpInput.Content content = nextTransformedContent();
if (LOG.isDebugEnabled())
LOG.debug("nextContent = {} {}", content, this);

25
jetty-server/src/main/java/org/eclipse/jetty/server/BlockingContentProducer.java
View File

@ -15,11 +15,12 @@ package org.eclipse.jetty.server;
import java.util.concurrent.Semaphore;
import org.eclipse.jetty.util.thread.AutoLock;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
* Blocking implementation of {@link ContentProducer}. Calling {@link #nextContent()} will block when
* Blocking implementation of {@link ContentProducer}. Calling {@link ContentProducer#nextContent(AutoLock)} will block when
* there is no available content but will never return null.
*/
class BlockingContentProducer implements ContentProducer
@ -82,11 +83,11 @@ class BlockingContentProducer implements ContentProducer
}
@Override
public HttpInput.Content nextContent()
public HttpInput.Content nextContent(AutoLock lock)
{
while (true)
{
HttpInput.Content content = _asyncContentProducer.nextContent();
HttpInput.Content content = _asyncContentProducer.nextContent(lock);
if (LOG.isDebugEnabled())
LOG.debug("nextContent async producer returned {}", content);
if (content != null)
@ -103,14 +104,32 @@ class BlockingContentProducer implements ContentProducer
if (LOG.isDebugEnabled())
LOG.debug("nextContent async producer is not ready, waiting on semaphore {}", _semaphore);
int lockReentranceCount = 0;
try
{
// Do not hold the lock during the wait on the semaphore.
// The lock must be unlocked more than once because it is
// reentrant so it fan be acquired multiple times by the
// same thread, so it can still be held by the thread after
// a single unlock call.
while (lock.isHeldByCurrentThread())
{
lock.close();
lockReentranceCount++;
}
_semaphore.acquire();
}
catch (InterruptedException e)
{
return new HttpInput.ErrorContent(e);
}
finally
{
// Re-lock the lock as many times as it was held
// before the unlock preceding the semaphore acquisition.
for (int i = 0; i < lockReentranceCount; i++)
lock.lock();
}
}
}

8
jetty-server/src/main/java/org/eclipse/jetty/server/ContentProducer.java
View File

@ -13,6 +13,8 @@
package org.eclipse.jetty.server;
import org.eclipse.jetty.util.thread.AutoLock;
/**
* ContentProducer is the bridge between {@link HttpInput} and {@link HttpChannel}.
* It wraps a {@link HttpChannel} and uses the {@link HttpChannel#needContent()},
@ -94,8 +96,10 @@ public interface ContentProducer
* After this call, state can be either of UNREADY or IDLE.
* @return the next content that can be read from or null if the implementation does not block
* and has no available content.
* @param lock The lock that is currently held. It will be released if this call has to block for the
* duration of the internal blocking.
*/
HttpInput.Content nextContent();
HttpInput.Content nextContent(AutoLock lock);
/**
* Free up the content by calling {@link HttpInput.Content#succeeded()} on it
@ -105,7 +109,7 @@ public interface ContentProducer
/**
* Check if this {@link ContentProducer} instance has some content that can be read without blocking.
* If there is some, the next call to {@link #nextContent()} will not block.
* If there is some, the next call to {@link #nextContent(AutoLock)} will not block.
* If there isn't any and the implementation does not block, this method will trigger a
* {@link javax.servlet.ReadListener} callback once some content is available.
* This call is always non-blocking.

4
jetty-server/src/main/java/org/eclipse/jetty/server/HttpInput.java
View File

@ -248,7 +248,7 @@ public class HttpInput extends ServletInputStream implements Runnable
// Calculate minimum request rate for DoS protection
_contentProducer.checkMinDataRate();
Content content = _contentProducer.nextContent();
Content content = _contentProducer.nextContent(lock);
if (content == null)
throw new IllegalStateException("read on unready input");
if (!content.isSpecial())
@ -341,7 +341,7 @@ public class HttpInput extends ServletInputStream implements Runnable
LOG.debug("running but not ready {}", this);
return;
}
content = _contentProducer.nextContent();
content = _contentProducer.nextContent(lock);
if (LOG.isDebugEnabled())
LOG.debug("running on content {} {}", content, this);
}

17
jetty-server/src/test/java/org/eclipse/jetty/server/AsyncContentProducerTest.java
View File

@ -28,8 +28,8 @@ import java.util.zip.GZIPOutputStream;
import org.eclipse.jetty.io.ArrayByteBufferPool;
import org.eclipse.jetty.io.EofException;
import org.eclipse.jetty.server.handler.gzip.GzipHttpInputInterceptor;
import org.eclipse.jetty.util.compression.CompressionPool;
import org.eclipse.jetty.util.compression.InflaterPool;
import org.eclipse.jetty.util.thread.AutoLock;
import org.hamcrest.core.Is;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
@ -176,6 +176,7 @@ public class AsyncContentProducerTest
int isReadyFalseCount = 0;
int isReadyTrueCount = 0;
Throwable error = null;
AutoLock lock = new AutoLock();
while (true)
{
@ -184,13 +185,17 @@ public class AsyncContentProducerTest
else
isReadyFalseCount++;
HttpInput.Content content = contentProducer.nextContent();
nextContentCount++;
if (content == null)
HttpInput.Content content;
try (AutoLock autoLock = lock.lock())
{
barrier.await(5, TimeUnit.SECONDS);
content = contentProducer.nextContent();
content = contentProducer.nextContent(lock);
nextContentCount++;
if (content == null)
{
barrier.await(5, TimeUnit.SECONDS);
content = contentProducer.nextContent(lock);
nextContentCount++;
}
}
assertThat(content, notNullValue());

8
jetty-server/src/test/java/org/eclipse/jetty/server/BlockingContentProducerTest.java
View File

@ -27,6 +27,7 @@ import org.eclipse.jetty.io.ArrayByteBufferPool;
import org.eclipse.jetty.io.EofException;
import org.eclipse.jetty.server.handler.gzip.GzipHttpInputInterceptor;
import org.eclipse.jetty.util.compression.InflaterPool;
import org.eclipse.jetty.util.thread.AutoLock;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
@ -174,9 +175,14 @@ public class BlockingContentProducerTest
int nextContentCount = 0;
String consumedString = "";
Throwable error = null;
AutoLock lock = new AutoLock();
while (true)
{
HttpInput.Content content = contentProducer.nextContent();
HttpInput.Content content;
try (AutoLock autoLock = lock.lock())
{
content = contentProducer.nextContent(lock);
}
nextContentCount++;
if (content.isSpecial())