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.

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

Fix native memory leak in GzipHandler #1429

Merged
merged 1 commit into from Mar 30, 2017
Merged

Fix native memory leak in GzipHandler #1429

merged 1 commit into from Mar 30, 2017

Conversation

denvned
Copy link
Contributor

@denvned denvned commented Mar 27, 2017

Looks like this leak shows itself only with asynchronous responses.

Some relevant info: http://www.devguli.com/blog/eng/java-deflater-and-outofmemoryerror/

@WalkerWatch
Copy link
Contributor

@denvned Before we can accept this Pull Request, you will need to sign an Eclipse Contributor Agreement. This is a requirement of the Eclipse foundation and is mandatory. It is very easy to set up, and instructions can be found here.

@joakime
Copy link
Contributor

joakime commented Mar 27, 2017

Calling .reset() and .end() on the Java Deflater does not fix the memory leak.

See Issue #293 along with the JVM bugs

There are plans to add a Deflater/Inflater pool to get around this issue (See Issue #300), so that GzipHandler, Jetty Client, and WebSocket's Permessage-Deflate all benefit from the reusing Deflater/Inflater objects, and minimize the memory leak effect.

@denvned
Copy link
Contributor Author

denvned commented Mar 27, 2017

Calling .reset() and .end() on the Java Deflater does not fix the memory leak.

@joakime But the issues on bugs.java.com you linked says that calling the end() on Deflater solves memory leak problem. What did I miss?

@joakime
Copy link
Contributor

joakime commented Mar 27, 2017

We've been down this path before, several testcases on multiple Deflater use alone have shown the bug is still present, even if using .end().

This is not a big problem for most Java applications, that only use 1 Deflater over their lifetime, but Jetty can use 1 for each HTTP/1.1 connection, and multiple for each WebSocket connection (that uses permessage-deflate) extension, potentially 10's of thousands of Deflater/Inflater instances, when it only needs a few hundred and reuse them.

@joakime
Copy link
Contributor

joakime commented Mar 27, 2017

I'm not trying to get you to change this PR, its still useful, as its right thing to do for a Deflater.
Just don't expect to to have much of an impact on the Deflater memory leak.

@denvned
Copy link
Contributor Author

denvned commented Mar 27, 2017

We've been down this path before, several testcases on multiple Deflater use alone have shown the bug is still present, even if using .end().

@joakime Are you saying that calling Deflater#end() does not actually free the off-heap memory natively allocated by Deflater? Have you reported it on http://bugs.java.com/?

Looks like this leak shows itself only with asynchronous responses.

Some relevant info: http://www.devguli.com/blog/eng/java-deflater-and-outofmemoryerror/

Signed-off-by: Denis Nedelyaev <denvned@gmail.com>
@denvned
Copy link
Contributor Author

denvned commented Mar 27, 2017

Signed ECA.

@denvned denvned changed the title Fix memory leak in GzipHandler Fix native memory leak in GzipHandler Mar 28, 2017
@gregw gregw merged commit 724c5b7 into jetty:master Mar 30, 2017
@gregw
Copy link
Contributor

gregw commented Mar 30, 2017

Thanks!
But we should get a hurry on for #300 as well. @denvned a PR for that would be welcome as well :)

@gregw
Copy link
Contributor

gregw commented Mar 30, 2017

Note that the GzipHandler is already using a Threadlocal "pool" of deflators

gregw pushed a commit that referenced this pull request Mar 30, 2017
Looks like this leak shows itself only with asynchronous responses.

Some relevant info: http://www.devguli.com/blog/eng/java-deflater-and-outofmemoryerror/

Signed-off-by: Denis Nedelyaev <denvned@gmail.com>
@gregw
Copy link
Contributor

gregw commented Mar 30, 2017

Just to clarify the discussion here. The GzipHandler is already recycling Deflaters and this PR just improved the handling by calling reset() if the Deflater is recycled and end() if it isn't.

However #300 still applies to Websocket and it could use a similar strategy.

gregw added a commit that referenced this pull request Mar 31, 2017
Improve low resource solution for scheduling strategy.

Replaced the dual scheduling strategy with a single re-implementation of EatWhatYouKill
that can adapt to act as ProduceConsume, ExectureProduceConsume or ProduceExecuteConsume
as need be.

Squashed commit of the following:

commit 25eeb32
Author: Greg Wilkins <gregw@webtide.com>
Date:   Sat Apr 1 09:08:49 2017 +1100

    renamed variables

commit 4f370d8
Merge: 8159c50 823cbe1
Author: Greg Wilkins <gregw@webtide.com>
Date:   Fri Mar 31 11:54:26 2017 +1100

    Merge branch 'jetty-9.4.x' into jetty-9.4.x-ewyk

commit 8159c50
Merge: 5805a92 daf61cd
Author: Greg Wilkins <gregw@webtide.com>
Date:   Thu Mar 30 17:28:00 2017 +1100

    Merge remote-tracking branch 'origin/jetty-9.3.x' into jetty-9.4.x-ewyk

commit daf61cd
Author: Denis Nedelyaev <denvned@gmail.com>
Date:   Thu Mar 30 04:15:32 2017 +0300

    Fix memory leak in GzipHandler (#1429)

    Looks like this leak shows itself only with asynchronous responses.

    Some relevant info: http://www.devguli.com/blog/eng/java-deflater-and-outofmemoryerror/

    Signed-off-by: Denis Nedelyaev <denvned@gmail.com>

commit 5805a92
Merge: cfabbd2 dc759db
Author: Greg Wilkins <gregw@webtide.com>
Date:   Thu Mar 30 17:12:38 2017 +1100

    Merge branch 'jetty-9.4.x' into jetty-9.4.x-ewyk

commit cfabbd2
Author: Greg Wilkins <gregw@webtide.com>
Date:   Thu Mar 30 16:04:57 2017 +1100

    minor cleanups

commit c7aa64a
Merge: bacf51a 18f17ac
Author: Greg Wilkins <gregw@webtide.com>
Date:   Thu Mar 30 14:58:37 2017 +1100

    Merge branch 'jetty-9.4.x' into jetty-9.4.x-ewyk

commit bacf51a
Merge: 11ba4bc 2fafa1d
Author: Greg Wilkins <gregw@webtide.com>
Date:   Thu Mar 30 14:13:36 2017 +1100

    Merge branch 'jetty-9.4.x' into jetty-9.4.x-ewyk

commit 11ba4bc
Merge: 69003d3 1a0b2df
Author: Greg Wilkins <gregw@webtide.com>
Date:   Thu Mar 30 13:48:09 2017 +1100

    Merge branch 'jetty-9.4.x' into jetty-9.4.x-ewyk

commit 69003d3
Merge: f89b08d a8ff18d
Author: Greg Wilkins <gregw@webtide.com>
Date:   Thu Mar 30 12:35:27 2017 +1100

    Merge branch 'jetty-9.4.x' into jetty-9.4.x-ewyk

commit f89b08d
Merge: 7a87c8e 00b42ca
Author: Greg Wilkins <gregw@webtide.com>
Date:   Thu Mar 23 16:01:00 2017 +1100

    Merge branch 'jetty-9.4.x' into jetty-9.4.x-ewyk

commit 7a87c8e
Merge: 1a92015 12dc169
Author: Greg Wilkins <gregw@webtide.com>
Date:   Thu Mar 23 10:27:14 2017 +1100

    Merge remote-tracking branch 'origin/jetty-9.4.x' into jetty-9.4.x-ewyk

commit 1a92015
Author: Greg Wilkins <gregw@webtide.com>
Date:   Tue Mar 21 09:23:53 2017 +1100

    better spruious wakeup handling and other simplifications

commit c01a910
Merge: 0b2b9ea 67ec4b0
Author: Greg Wilkins <gregw@webtide.com>
Date:   Fri Mar 17 14:59:37 2017 +1100

    Merge branch 'jetty-9.4.x' into jetty-9.4.x-ewyk

commit 0b2b9ea
Author: Greg Wilkins <gregw@webtide.com>
Date:   Fri Mar 17 14:52:49 2017 +1100

    cleanup

commit c1d92eb
Author: Greg Wilkins <gregw@webtide.com>
Date:   Fri Mar 17 13:41:45 2017 +1100

    Fixed push

commit d2d6bc3
Author: Greg Wilkins <gregw@webtide.com>
Date:   Fri Mar 17 12:18:03 2017 +1100

    minor cleanups

commit c1a159b
Merge: 01349ac 78f4712
Author: Greg Wilkins <gregw@webtide.com>
Date:   Fri Mar 17 09:30:44 2017 +1100

    Merge branch 'jetty-9.4.x' into jetty-9.4.x-ewyk

commit 01349ac
Merge: 4dc1503 08f351b
Author: Greg Wilkins <gregw@webtide.com>
Date:   Fri Mar 17 08:16:06 2017 +1100

    Merge remote-tracking branch 'origin/jetty-9.4.x' into jetty-9.4.x-ewyk

commit 4dc1503
Author: Greg Wilkins <gregw@webtide.com>
Date:   Thu Mar 16 23:26:59 2017 +1100

    work in progress

commit 5d18c65
Author: Greg Wilkins <gregw@webtide.com>
Date:   Thu Mar 16 22:05:03 2017 +1100

    work in progress

commit d52a09a
Author: Greg Wilkins <gregw@webtide.com>
Date:   Thu Mar 16 18:03:16 2017 +1100

    work in progress

commit c097db3
Author: Greg Wilkins <gregw@webtide.com>
Date:   Thu Mar 16 15:59:29 2017 +1100

    Experiement enhancement to EatWhatYouKill ExecutionStrategy

    Use the existence of a pending producer threads to determine if low resources or not.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants