Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 7dc093c0 authored by Tobias Thierer's avatar Tobias Thierer
Browse files

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
parent 17615663

media/java/android/media/MediaHTTPConnection.java

+48 −13
Original line number Diff line number Diff line
@@ -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,6 +160,8 @@ public class MediaHTTPConnection extends IMediaHTTPConnection.Stub { 
    @Override

    @UnsupportedAppUsage

    public void disconnect() {

        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.
@@ -162,13 +169,16 @@ public class MediaHTTPConnection extends IMediaHTTPConnection.Stub { 
                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

                // 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();

        }

    }



    private synchronized void teardownConnection() {
@@ -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