Fix http response input stream resource leak by tejksat · Pull Request #633 · docker-java/docker-java

tejksat · 2016-07-12T12:31:29Z

According to https://github.com/netty/netty/wiki/Reference-counted-objects tests logs should be checked for an error message about a memory leak:

PARANOID - Same with ADVANCED except that it's for every single buffer. Useful for automated testing phase. You could fail the build if the build output contains 'LEAK:'.

I don't know how to implement it in a good way. Could you please suggest?

Fixes #624

This change is

tejksat · 2016-07-24T20:28:04Z

@marcuslinke @KostyaSha could you please check the changes? Please also note the question in the comment above.

KostyaSha · 2016-07-29T21:34:07Z

Sorry, lost comment. Could you check coverity results for master branch? Does it has complains about this resource leak? https://scan.coverity.com/projects/docker-java-docker-java?tab=overview

tejksat · 2016-07-30T09:46:42Z

@KostyaSha sure! I've requested the access to coverty.

KostyaSha · 2016-07-30T20:42:41Z

Hm.. you should be able to see any defects without contributor... Press "view defects"

tejksat · 2016-08-01T12:52:51Z

@KostyaSha when I sign in as GitHub user Coverity shows a message Want to view defects or help fix defects? with a button below Add me to project.

KostyaSha · 2016-08-01T12:54:59Z

Added as defect viewer, if you will need "triage?" please ping me.

KostyaSha · 2016-08-01T12:59:09Z

@marcuslinke for review

tejksat · 2016-08-01T13:15:12Z

@KostyaSha thank you! HttpResponseStreamHandler is clean. Does Coverity could find problems with not released Netty buffers?

KostyaSha · 2016-08-01T13:24:19Z

As i understand it analyses only our code and nobody knows what checks it has because it proprietary solution.

tejksat · 2016-08-01T13:37:34Z

Ok.. I'll try to find a solution to check generated logs for messages about ByteBuf leaks. I've checked them locally and on downloading a somewhat big file there is a resource leak message there.

tejksat · 2016-08-25T12:49:57Z

@KostyaSha @marcuslinke could you please check? Travis errors don't seem to be related with PR changes.

KostyaSha · 2016-08-25T12:57:15Z

@tejksat 1.11 tcp build is green and it fine, 1.12 tcp shouldn't have new failures (atm only #670 fails)

KostyaSha · 2016-08-25T12:57:50Z

src/main/java/com/github/dockerjava/core/async/AsyncResultCallbackTemplate.java

+    public void onError(Throwable throwable) {
+        if (closed) return;
+
+        if (this.firstError == null) {


please don't use this where it not required.

codecov-io · 2016-08-29T11:45:51Z

Current coverage is 71.02% (diff: 78.94%)

Merging #633 into master will decrease coverage by 0.01%

@@             master       #633   diff @@
==========================================
  Files           300        301     +1   
  Lines          6131       6225    +94   
  Methods           0          0          
  Messages          0          0          
  Branches        518        532    +14   
==========================================
+ Hits           4355       4421    +66   
- Misses         1526       1550    +24   
- Partials        250        254     +4

Powered by Codecov. Last update eaef1ac...0cb0b08

tejksat · 2016-08-29T17:56:54Z

@KostyaSha pls review!

KostyaSha · 2016-08-30T08:57:00Z

@tejksat https://github.com/codecov/browser-extension if you would like to cover all your logic :)

KostyaSha · 2016-08-30T08:57:31Z

@tejksat i don't understand this routines, would wait bit for @marcuslinke or merge as it passes tests.

tejksat · 2016-08-31T10:40:18Z

@KostyaSha codecov Chrome extension is the best! Thank you!

marcuslinke · 2016-09-02T19:03:35Z

@tejksat Sorry for being late but I don't get the intent of this PR. As I understood from your intitial comment on #624 the problem was that the current ByteBuf wasn't released properly. This could be easily fixed by calling current.discardReadBytes(). Could you explain please?

tejksat · 2016-09-02T20:57:51Z

@marcuslinke there are several problems in the original code:

not releasing ByteBuf queued;
queuing ByteBuf until all data is read what effectively leads to OutOfDirectMemoryError if the data read is too big. This happens if you try to copy too big file or folder from a container. To fix this ResponseCallback (which in awaitResult() waits in fact for readComplete() channel method invocation) replaced with AsyncResultCallback;
actually queuing ByteBuf anyway leads to OutOfDirectMemoryError if data consumption is slower (the next point) than obtaining it;
poor performance because of the byte-by-byte reads.

marcuslinke · 2016-09-02T21:31:35Z

src/main/java/com/github/dockerjava/core/async/AsyncResultCallback.java

+/**
+ * Implementation of {@link ResultCallback} with the single result event expected.
+ */
+public class AsyncResultCallback<A_RES_T> implements ResultCallback<A_RES_T> {


This special callback class should inherit ResultCallbackTemplate ideally and should be moved into InvocationBuilder internally.

…tream.close() but rather a separate method

HttpResponseInputStream reworked not to queue incoming byte buffers. This queuing leads to the OutOfDirectMemoryError for sure when buffer elements are not processed quite fast.

marcuslinke · 2016-09-05T05:40:11Z

LGTM. Thanks @tejksat for fixing all these issues!

tejksat · 2016-09-05T11:44:58Z

@marcuslinke I'm glad to do that!

there are several problems in the original code: - not releasing `ByteBuf` queued; - queuing `ByteBuf` until all data is read what effectively leads to `OutOfDirectMemoryError` if the data read is too big. This happens if you try to copy too big file or folder from a container. To fix this `ResponseCallback` (which in `awaitResult()` waits in fact for `readComplete()` channel method invocation) replaced with `AsyncResultCallback`; - actually queuing `ByteBuf` anyway leads to `OutOfDirectMemoryError` if data consumption is slower (the next point) than obtaining it; - poor performance because of the byte-by-byte reads.

tejksat mentioned this pull request Jul 12, 2016

Memory leak in HttpResponseStreamHandler #624

Closed

KostyaSha assigned marcuslinke Aug 1, 2016

KostyaSha added type/bug type/enhancement transport/netty labels Aug 1, 2016

tejksat force-pushed the fixHttpResponseInputStreamResourceLeak branch from 4a85e72 to b76acde Compare August 24, 2016 13:05

tejksat mentioned this pull request Aug 25, 2016

Trying to copy large data from container ends with OutOfDirectMemoryError exception #679

Closed

KostyaSha reviewed Aug 25, 2016
View reviewed changes

tejksat force-pushed the fixHttpResponseInputStreamResourceLeak branch from 27bffd6 to 0cb0b08 Compare August 29, 2016 11:13

tejksat force-pushed the fixHttpResponseInputStreamResourceLeak branch from 0cb0b08 to 1871e6d Compare August 29, 2016 12:37

marcuslinke reviewed Sep 2, 2016
View reviewed changes

tejksat and others added 7 commits September 3, 2016 14:52

close() of HttpResponseInputStream is not an implementation of InputS…

369a54b

…tream.close() but rather a separate method

Fix for docker-java#624

71692bb

Fix for docker-java#679

9969922

HttpResponseInputStream reworked not to queue incoming byte buffers. This queuing leads to the OutOfDirectMemoryError for sure when buffer elements are not processed quite fast.

HttpResponseInputStream.read() method covered

7a672dd

Stuck on HttpResponseInputStream.read() after stream is closed fixed

321cff7

Logic on the closed state of HttpResponseInputStream covered

ab230cb

AsyncResultCallback class moved to InvocationBuilder

aac58a7

tejksat force-pushed the fixHttpResponseInputStreamResourceLeak branch from bbda847 to aac58a7 Compare September 3, 2016 12:07

marcuslinke merged commit 74d506d into docker-java:master Sep 5, 2016

tejksat deleted the fixHttpResponseInputStreamResourceLeak branch September 5, 2016 11:45

Conversation

tejksat commented Jul 12, 2016 • edited by KostyaSha Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tejksat commented Jul 24, 2016

Uh oh!

KostyaSha commented Jul 29, 2016

Uh oh!

tejksat commented Jul 30, 2016

Uh oh!

KostyaSha commented Jul 30, 2016

Uh oh!

tejksat commented Aug 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KostyaSha commented Aug 1, 2016

Uh oh!

KostyaSha commented Aug 1, 2016

Uh oh!

tejksat commented Aug 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KostyaSha commented Aug 1, 2016

Uh oh!

tejksat commented Aug 1, 2016

Uh oh!

tejksat commented Aug 25, 2016

Uh oh!

KostyaSha commented Aug 25, 2016

Uh oh!

KostyaSha Aug 25, 2016

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Aug 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 71.02% (diff: 78.94%)

Uh oh!

tejksat commented Aug 29, 2016

Uh oh!

KostyaSha commented Aug 30, 2016

Uh oh!

KostyaSha commented Aug 30, 2016

Uh oh!

tejksat commented Aug 31, 2016

Uh oh!

marcuslinke commented Sep 2, 2016

Uh oh!

tejksat commented Sep 2, 2016

Uh oh!

marcuslinke Sep 2, 2016

Choose a reason for hiding this comment

Uh oh!

tejksat Sep 3, 2016

Choose a reason for hiding this comment

Uh oh!

marcuslinke commented Sep 5, 2016

Uh oh!

tejksat commented Sep 5, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tejksat commented Jul 12, 2016 •

edited by KostyaSha

Loading

tejksat commented Aug 1, 2016 •

edited

Loading

tejksat commented Aug 1, 2016 •

edited

Loading

codecov-io commented Aug 29, 2016 •

edited

Loading