Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

HttpUrlFetcher leaks resources #2352

Closed
sjudd opened this issue Sep 7, 2017 · 2 comments
Closed

HttpUrlFetcher leaks resources #2352

sjudd opened this issue Sep 7, 2017 · 2 comments
Assignees
@sjudd
Labels
bug
Milestone
4.2

Comments

@sjudd
Copy link
Collaborator

sjudd commented Sep 7, 2017 •
edited
Loading

Similar to #1996, but without the okhttp3 integration library.

Resources are leaked in the flickr sample app just by opening and scrolling:

09-07 07:19:24.684 W/OkHttpClient( 2603): A connection to http://farm5.staticflickr.com/ was leaked. Did you forget to close a response body?
09-07 07:19:30.298 W/OkHttpClient( 2603): A connection to http://farm5.staticflickr.com/ was leaked. Did you forget to close a response body?
09-07 07:19:30.299 W/OkHttpClient( 2603): A connection to http://farm5.staticflickr.com/ was leaked. Did you forget to close a response body?
09-07 07:22:32.056 W/OkHttpClient( 2603): A connection to http://farm5.staticflickr.com/ was leaked. Did you forget to close a response body?
09-07 07:59:48.489 W/OkHttpClient( 4621): A connection to http://farm5.staticflickr.com/ was leaked. Did you forget to close a response body?
09-07 07:59:48.489 W/OkHttpClient( 4621): A connection to http://farm5.staticflickr.com/ was leaked. Did you forget to close a response body?
09-07 08:00:00.305 W/OkHttpClient( 4621): A connection to http://farm5.staticflickr.com/ was leaked. Did you forget to close a response body?
09-07 08:00:00.306 W/OkHttpClient( 4621): A connection to http://farm5.staticflickr.com/ was leaked. Did you forget to close a response body?
09-07 08:00:55.958 W/OkHttpClient( 4621): A connection to http://farm5.staticflickr.com/ was leaked. Did you forget to close a response body?
09-07 08:00:55.958 W/OkHttpClient( 4621): A connection to http://farm5.staticflickr.com/ was leaked. Did you forget to close a response body?
09-07 08:29:08.455 W/OkHttpClient( 4621): A connection to http://farm5.staticflickr.com/ was leaked. Did you forget to close a response body?
09-07 08:29:08.455 W/OkHttpClient( 4621): A connection to http://farm5.staticflickr.com/ was leaked. Did you forget to close a response body?
09-07 08:31:43.044 W/OkHttpClient( 4862): A connection to http://farm5.staticflickr.com/ was leaked. Did you forget to close a response body?
09-07 08:31:43.046 W/OkHttpClient( 4862): A connection to http://farm5.staticflickr.com/ was leaked. Did you forget to close a response body?
09-07 08:32:08.491 W/OkHttpClient( 4862): A connection to http://farm5.staticflickr.com/ was leaked. Did you forget to close a response body?
09-07 08:34:11.030 W/OkHttpClient( 4978): A connection to http://farm5.staticflickr.com/ was leaked. Did you forget to close a response body?
@sjudd sjudd added the bug label Sep 7, 2017
@sjudd sjudd added this to the 4.2 milestone Sep 7, 2017
@sjudd sjudd self-assigned this Sep 7, 2017
@sjudd
Copy link
Collaborator Author

sjudd commented Sep 7, 2017

I've found a few sources of this so far:

  1. When HttpUrlFetcher follows a redirect, it doesn't disconnect the connection that was redirected.
  2. When you disconnect an HttpUrlConnection, it doesn't close the underlying OkHttp ResponseBody. You have to obtain and the close the stream to do so.
  3. When HttpUrlFetcher is cancelled, it returns before obtaining the stream so due to (2), it leaks a resource even though cleanup does appear to be called.

sjudd added a commit to sjudd/glide that referenced this issue Sep 7, 2017
@sjudd
Close HttpUrlConnections when following redirects in HttpUrlFetcher.
7f62597 
Progress towards bumptech#2352.
sjudd added a commit to sjudd/glide that referenced this issue Sep 7, 2017
@sjudd
Always set stream when connecting HttpUrlConnection
73a8054 
That way we will always close the stream in cleanup(). Prior to this change, when cancel was called, stream was never set to non-null and therefore never closed in cleanup. Because disconnecting the stream doesn’t seem to close the contained ResponseBody object, failing to obtain and then close the stream results in leaked objects.

Progress towards bumptech#2352
@sjudd sjudd changed the title HttpUrlFetcher leaks resources when following redirects HttpUrlFetcher leaks resources Sep 8, 2017
@sjudd
Copy link
Collaborator Author

sjudd commented Sep 8, 2017

Calling this fixed for now. I might have seen this one time after both changes, so there might still be a race, but it's much less frequent and difficult to reproduce.

@sjudd sjudd closed this as completed Sep 8, 2017
sjudd added a commit to sjudd/glide that referenced this issue Sep 8, 2017
@sjudd
Clean up DataFetchers even if DecodeJobs are cancelled prior to run().
d482b8e 
Previously some of the logic in run() might null out the data fetcher
instance variable, even if it had been set when run() was first called.
As a result, the data fetcher would not be cleared. Now the initial 
data fetcher is held on to as a local variable so the instance variable
can be freed without affecting the behavior of cleanup().

Progress towards bumptech#1996, bumptech#2352
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sjudd sjudd

Labels
bug
Projects
None yet
Milestone
4.2
Development

No branches or pull requests
1 participant
@sjudd