Skip to content

Close gzipOutputStream after the usage#542

Merged
munendrasn merged 1 commit intopippo-java:masterfrom
munendrasn:close-stream
May 27, 2020
Merged

Close gzipOutputStream after the usage#542
munendrasn merged 1 commit intopippo-java:masterfrom
munendrasn:close-stream

Conversation

@munendrasn
Copy link
Copy Markdown
Collaborator

Fixes #541

@coveralls
Copy link
Copy Markdown

coveralls commented May 26, 2020

Pull Request Test Coverage Report for Build 1107

  • 0 of 11 (0.0%) changed or added relevant lines in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.007%) to 21.901%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pippo-core/src/main/java/ro/pippo/core/gzip/GZipResponseStream.java 0 11 0.0%
Files with Coverage Reduction New Missed Lines %
pippo-core/src/main/java/ro/pippo/core/gzip/GZipResponseStream.java 1 0%
Totals Coverage Status
Change from base Build 1106: -0.007%
Covered Lines: 1408
Relevant Lines: 6429

💛 - Coveralls

@munendrasn
Copy link
Copy Markdown
Collaborator Author

Created PR #543 for failing test in master for java11

@decebals
Copy link
Copy Markdown
Member

@munendrasn Please test it with your application(s) and if you don't see a downside I am ready to merge it.

@munendrasn
Copy link
Copy Markdown
Collaborator Author

munendrasn commented May 26, 2020

I did some minor testing, things look good. I haven't done good perf tests were memory could build-up

@decebals
Copy link
Copy Markdown
Member

@munendrasn Again, if you think that this PR is ready for merge please merge it. I don't know if we can add a unit test for it. Try it in your application(s) and if the result is OK please merge it.
@mhagnumdw Do you see any problem with the modifications from this PR?

@mhagnumdw
Copy link
Copy Markdown
Member

@decebals , looks good to me! I ratify what you said.

💭 ps: I was thinking (just thinking) what a unit test would be like to detect memory leak. From what I understand and to be more precise, I would have to start a new jvm just with this test. 💭

@decebals
Copy link
Copy Markdown
Member

@mhagnumdw Thanks for feedback!

looks good to me! I ratify what you said.

In this case, @munendrasn please press the "Squash and merge" button. It's time for your first PR merge in Pippo 😄.

ps: I was thinking (just thinking) what a unit test would be like to detect memory leak. From what I understand and to be more precise, I would have to start a new jvm just with this test.

I think that it's too complicated so let's do it without a test.

@munendrasn
Copy link
Copy Markdown
Collaborator Author

munendrasn commented May 27, 2020

@decebals Thanks for the merge access

In this case, @munendrasn please press the "Squash and merge" button

Regarding this, Github allows Repo admin(in this case you) to set branch rules from Settings, through which only squash-merge can be allowed, all other options remain disabled

@munendrasn munendrasn merged commit 4ac8e30 into pippo-java:master May 27, 2020
@munendrasn munendrasn deleted the close-stream branch May 27, 2020 05:29
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.

Probable Native memory leak with Java11

4 participants