From 7dc093c08b07a2a23688775017edda7f38c90e7e Mon Sep 17 00:00:00 2001 From: Tobias Thierer Date: Wed, 29 May 2019 00:21:22 +0100 Subject: [PATCH] Fix MediaHTTPConnection.disconnect() blocking for a long time. MediaHTTPConnection.seek() was creating new connections in a while loop without checking whether another thread was busy concurrently disconnecting. When a new connection was created between the time the disconnect()ing thread had disconnected the old one and acquired the synchronized block, the new connection wouldn't be disconnected and therefore seek() would not encounter an IOException; therefore, seek() would not return quickly, leaving the disconnecting thread waiting to acquire the synchronized block for a long time. This CL fixes this by making seek() throw IOException quickly if it discovers that another thread is trying to disconnect. This is checked shortly before and after the new connection is created, to avoid a race based on the order between the new connection being created and the disconnecting thread reading the old connection value. Note that this still doesn't stop a new connection being created shortly after the previous one was torn down - it only stops the disconnecting thread waiting for a long time to acquire the synchronized lock. Fixes: 131894499 Test: The following command hang 3/3 times before this CL, but succeeded 3/3 times afterwards: atest android.media.cts.NativeDecoderTest#testAMediaDataSourceClose Change-Id: I3862a4367d0e46c64c0cbf7bcaa369aca5692871 --- .../android/media/MediaHTTPConnection.java | 61 +++++++++++++++---- 1 file changed, 48 insertions(+), 13 deletions(-) diff --git a/media/java/android/media/MediaHTTPConnection.java b/media/java/android/media/MediaHTTPConnection.java index b5c4cca12ff7d..8ee929e778992 100644 --- a/media/java/android/media/MediaHTTPConnection.java +++ b/media/java/android/media/MediaHTTPConnection.java @@ -38,6 +38,7 @@ import java.net.URL; import java.net.UnknownServiceException; import java.util.HashMap; import java.util.Map; +import java.util.concurrent.atomic.AtomicInteger; /** @hide */ public class MediaHTTPConnection extends IMediaHTTPConnection.Stub { @@ -83,6 +84,10 @@ public class MediaHTTPConnection extends IMediaHTTPConnection.Stub { private final static int HTTP_TEMP_REDIRECT = 307; private final static int MAX_REDIRECTS = 20; + // The number of threads that are currently running disconnect() (possibly + // not yet holding the synchronized lock). + private final AtomicInteger mNumDisconnectingThreads = new AtomicInteger(0); + @UnsupportedAppUsage public MediaHTTPConnection() { CookieHandler cookieHandler = CookieHandler.getDefault(); @@ -155,19 +160,24 @@ public class MediaHTTPConnection extends IMediaHTTPConnection.Stub { @Override @UnsupportedAppUsage public void disconnect() { - HttpURLConnection connectionToDisconnect = mConnection; - // Call disconnect() before blocking for the lock in order to ensure that any - // other thread that is blocked in readAt() will return quickly. - if (connectionToDisconnect != null) { - connectionToDisconnect.disconnect(); - } - synchronized (this) { - // It's unlikely but possible that while we were waiting to acquire the lock, another - // thread concurrently started a new connection; if so, we're disconnecting that one - // here, too. - teardownConnection(); - mHeaders = null; - mURL = null; + mNumDisconnectingThreads.incrementAndGet(); + try { + HttpURLConnection connectionToDisconnect = mConnection; + // Call disconnect() before blocking for the lock in order to ensure that any + // other thread that is blocked in readAt() will return quickly. + if (connectionToDisconnect != null) { + connectionToDisconnect.disconnect(); + } + synchronized (this) { + // It's possible that while we were waiting to acquire the lock, another thread + // concurrently started a new connection; if so, we're disconnecting that one + // here, too. + teardownConnection(); + mHeaders = null; + mURL = null; + } + } finally { + mNumDisconnectingThreads.decrementAndGet(); } } @@ -224,11 +234,36 @@ public class MediaHTTPConnection extends IMediaHTTPConnection.Stub { boolean noProxy = isLocalHost(url); while (true) { + // If another thread is concurrently disconnect()ing, there's a race + // between them and us. Therefore, we check mNumDisconnectingThreads shortly + // (not atomically) before & after writing mConnection. This guarantees that + // we won't "lose" a disconnect by creating a new connection that might + // miss the disconnect. + // + // Note that throwing an instanceof IOException is also what this thread + // would have done if another thread disconnect()ed the connection while + // this thread was blocked reading from that connection further down in this + // loop. + if (mNumDisconnectingThreads.get() > 0) { + throw new IOException("concurrently disconnecting"); + } if (noProxy) { mConnection = (HttpURLConnection)url.openConnection(Proxy.NO_PROXY); } else { mConnection = (HttpURLConnection)url.openConnection(); } + // If another thread is concurrently disconnecting, throwing IOException will + // cause us to release the lock, giving the other thread a chance to acquire + // it. It also ensures that the catch block will run, which will tear down + // the connection even if the other thread happens to already be on its way + // out of disconnect(). + if (mNumDisconnectingThreads.get() > 0) { + throw new IOException("concurrently disconnecting"); + } + // If we get here without having thrown, we know that other threads + // will see our write to mConnection. Any disconnect() on that mConnection + // instance will cause our read from/write to that connection instance below + // to encounter an instanceof IOException. mConnection.setConnectTimeout(CONNECT_TIMEOUT_MS); // handle redirects ourselves if we do not allow cross-domain redirect