DefaultHttpDataSource: A connection to XXX was leaked. Did you forget to close a response body?

[chariotsystems]

com.google.android.exoplayer2.upstream.DefaultHttpDataSource

will leak connections for a 404 response from the server

W/OkHttpClient: A connection to http://203.36.100.86/ was leaked. Did you forget to close a response body?
W/OkHttpClient: A connection to http://203.36.100.37/ was leaked. Did you forget to close a response body?
W/OkHttpClient: A connection to http://127.0.0.1:8080/ was leaked. Did you forget to close a response body?

The reason is that the okhttp3.internal.connection.RealConnection keeps track of which of the three streams (input, output and error) it believes its client has opened. If they are not closed before disconnect then it sees this as an error.

The fix to DefaultDataSource is as follows change close to being as shown below:
Also instead of calling connection.disconnect(); or closeConnectionQuietly() directly - just call close() instead.

  @Override
    public void close() throws HttpDataSourceException {
        try {
            if (connection != null) {
                if (inputStream == null) {
                    try {
                        inputStream = connection.getInputStream();
                    } catch (IOException e) {
                    }
                }
                if (inputStream != null) {
                    maybeTerminateInputStream(connection, bytesRemaining());
                    try {
                        inputStream.close();
                    } catch (IOException e) {
                        throw new HttpDataSourceException(e, dataSpec, HttpDataSourceException.TYPE_CLOSE);
                    }
                }
                try {
                    OutputStream outputStream = connection.getOutputStream();
                    if (outputStream != null) {
                        outputStream.close();
                    }
                } catch (Exception e) {
                }
                try {
                    InputStream errorStream = connection.getErrorStream();
                    if (errorStream != null) {
                        errorStream.close();
                    }
                } catch (Exception e) {
                }
            }
        } finally {
            inputStream = null;
            closeConnectionQuietly();
            if (opened) {
                opened = false;
                if (listener != null) {
                    listener.onTransferEnd(this);
                }
            }
        }
    }

[chariotsystems]

I am using my own modified version of the DefaultHttpDataSource as I need to support unicast and multicast streams and cross over - so there is no urgency for me that this be fixed - but this fix will help others :)

[ojw28]

DefaultHttpDataSource relies on the underlying platform network stack, via HttpURLConnection. So I think the logging you're referring to must be coming from the underlying platform. I'm unable to reproduce the issue locally, so I suspect it may only occur on certain devices and/or versions of Android, and that it's a platform bug specific to those.

The reason is that the okhttp3.internal.connection.RealConnection keeps track of which of the three streams (input, output and error) it believes its client has opened. If they are not closed before disconnect then it sees this as an error.

Android has (by default) never used okhttp3 as the underlying platform network stack. So unless the manufacturer of the device you're using has replaced the network stack, I think it's more likely this is coming from a version of okhttp 2.x.x, which has been used (version 2.7.5 includes the same logging).

For us to further investigate, please could you provide a bug report captured using adb bugreport shortly after reproducing the issue, details of the device and Android version, and concrete reproduction steps? Thanks.

[chariotsystems]

Hi, Thank you for your interest in this matter - appreciated 😄👍 I am using a Samsung S8 running Android O. Whether is okhttp2 or 3 I'm not sure in debug I see - com.android.okhttp.internal.huc.HttpURLConnectionImpl. I just used Jake Wharton's GitHub of okhttp to establish the root cause. In order that you can see clearly the problem - I have attached a nefarious version of the DefaultDataSource (called LeakyHttpDataSource) which shows the problem every time - just toggle the boolean to see the behaviour. private final boolean willLeak = true; Jake does say in his comments that although not closing all the streams is a developer bug, for most use cases it shouldn't cause any problems. I believe that it's important that my logs should be clean of these warnings. I hope this helps. Cheers Alex

…

On Sat, Jan 20, 2018 at 1:53 AM, ojw28 ***@***.***> wrote: DefaultHttpDataSource relies on the underlying platform network stack, via HttpURLConnection. So I think the logging you're referring to must be coming from the underlying platform. I'm unable to reproduce the issue locally, so I suspect it may only occur on certain devices and/or versions of Android, and that it's a platform bug specific to those. The reason is that the okhttp3.internal.connection.RealConnection keeps track of which of the three streams (input, output and error) it believes its client has opened. If they are not closed before disconnect then it sees this as an error. Android has (by default) never used okhttp3 as the underlying platform network stack. So unless the manufacturer of the device you're using has replaced the network stack, I think it's more likely this is coming from a version of okhttp 2.x.x (version 2.7.5 includes the same logging). For us to further investigate, please could you provide a bug report captured using adb bugreport shortly after reproducing the issue, details of the device and Android version, and concrete reproduction steps? Thanks. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3726 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE4uoL3bGfDB5abGPVkTTw7TYIhkeoBhks5tMKxrgaJpZM4RjtHg> .

[chariotsystems]

removed wrong file

[chariotsystems]

The solution provided in LeakyHttpDataSource doesn't comprehensively call close for all instances where connection.disconnect() is called - so ensure that this is done in your final solution.

[ojw28]

I still don't understand this:

• There is nothing in the okhttp GitHub issue you reference that saying that you need to get and close all of the streams explicitly, as far as I can see. That issue relates to okhttp3 and only says that you need to close the response body. This is not relevant to the case of dealing with okhttp2 via HttpURLConnection; I think that calling connection.disconnect() should be enough. If you know of a comment relating to what you're doing here, please could you point out exactly where it is.
• I don't see a willLeak boolean anywhere in the attached file.
• I don't think the S8 Oreo release has rolled out yet. Is this a beta version, or a non-official build provided by someone other than Samsung? I suspect this is an issue specific to the version of Android that you're running on the device, since I don't think the ExoPlayer code is doing anything wrong. If this is a beta version or non-official build, that becomes even more likely.

Please clarify.

[ojw28]

Please also attach a bugreport attached with adb bugreport, as requested in the original issue template and above.

[chariotsystems]

LeakyHttpDataSource.java.zip

[chariotsystems]

A big apology here. I seem to have attached an earlier version of the file without the fix.
This is what I have modified as well as the implementation of the close() method details as given above. Note that the url is deliberately mangled to force the makeConnection's bowels to cough up an error and hence implicitly open the error stream.

 private HttpURLConnection makeConnection(DataSpec dataSpec) throws IOException {
        URL url = new URL(dataSpec.uri.toString());
        byte[] postBody = dataSpec.postBody;
        long position = dataSpec.position;
        long length = dataSpec.length;
        boolean allowGzip = dataSpec.isFlagSet(DataSpec.FLAG_ALLOW_GZIP);

        for (int i = 0; i < 20; i++) {
            HttpURLConnection connection = makeConnection(
                    new URL(dataSpec.uri.toString() + "bad"), postBody, position, length, allowGzip, false /* followRedirects */);
            if (willLeak) {
                connection.disconnect();
            } else {
                close(connection);
            }
        }

        if (!allowCrossProtocolRedirects) {
            // HttpURLConnection disallows cross-protocol redirects, but otherwise performs redirection

[chariotsystems]

Yes I am using an advanced Engineering Build of S8 from Samsung. You might not have access to such a device and build. So I will reproduce on an S7 for you later and attach the adb bugreport as requested.

I presume you know how to run this LeakyHttpDataSource with a multi segment dash manifest.
That you know to modify the HttpDataSourceFactory to return this rather than the DefaultHttpDataSource.

 @Override
    public LeakyHttpDataSource createDataSource() {
        return new LeakyHttpDataSource(userAgent, null, listener, connectTimeoutMillis,
                readTimeoutMillis);
      
    }

[ojw28]

Thanks. I can reproduce this with the new makeConnection code snippet. It's not specific to the S8 build. Marking as a bug.

[chariotsystems]

That's great :)

[ojw28]

I dug into this a bit more. It looks like it was decided this was a platform issue, rather than anything at the app level. As a result, the logged warning was removed from the platform in https://android-review.googlesource.com/c/platform/external/okhttp/+/460418. I think this will probably land in Android P.

I have no objection to explicitly getting and closing the streams to avoid the error, but only if this occurs no overhead. It seems possible that calling getXStream might actually cause the stream to be allocated or something, just so we can immediately release it. We need to double check that this definitely wont happen before making the change, since if it does the harmless log message is preferable.

[chariotsystems]

Yes I hear what you are saying For the normal user of the Exoplayer, unless they are using the cross domain code at the bottom of the method, will not be using .disconnect() after a non-200 response. In my case we frequently in fact the whole movie, can be a series of 307 and 302s. So in order to keep our logs clean - its a good idea that I close them - but I have my own implementation of this class in any case. Tracking this issue down (and trying to establish if it was harmless) took me several hours so I wanted to save others in the community the same work - hence I raised this issue. Based on what Jake says - yes it's probably harmless for most scenarios.

…

On Thu, Jan 25, 2018 at 5:26 AM, ojw28 ***@***.***> wrote: I dug into this a bit more. It looks like it was decided this was a platform issue, rather than anything at the app level. As a result, the logged warning was removed from the platform in https://android-review. googlesource.com/c/platform/external/okhttp/+/460418. I think this will probably land in Android P. I have no objection to explicitly getting and closing the streams to avoid the error, but only if this occurs no overhead. It seems possible that calling getXStream might actually cause the stream to be allocated or something, just so we can immediately release it. We need to double check that this definitely wont happen before making the change, since if it does the harmless log message is preferable. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3726 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE4uoLI8DC2yR0LHO0HYzM3Wav-5neClks5tN3XFgaJpZM4RjtHg> .

[ojw28]

I don't think we're going to spend any time fixing this. The logged warning was removed from the platform in newer releases, and in older versions it seems it's considered harmless.

You can also avoid the issue by using one of our two network stack extensions (the OkHttp extension and the Cronet extension). It's probably a good idea to use one of those regardless, so you're using the same network stack across all versions of Android on which your app runs.

[chariotsystems]

Thank you for all your help here.

…

On Thu, May 10, 2018 at 9:21 AM, ojw28 ***@***.***> wrote: Closed #3726 <#3726>. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3726 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE4uoJC487HiVy-jYa2vRRCdMDiOIdXhks5tw3n4gaJpZM4RjtHg> .