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 potential resource leak in AvoidStandbyModeService #3909

Merged

Conversation

@stejbac
Copy link
Contributor

stejbac commented Jan 19, 2020

Replace tail recursion of the AvoidStandbyModeService.play method with an ordinary loop, to prevent a new open JAR resource InputStream + sound file OutputStream (which were created every 4 minute playback) from accumulating on the stack, closing them inside the loop instead. (This also prevents eventual stack overflow.)

Also tidy up FileUtil.resourceToFile and put the JAR URL InputStream in a try-with-resources block, to ensure that it doesn't leak either.

--

Running lsof does reveal an accumulation of file handles without this fix, although not as many as one would expect from a new stream being opened every 4 minutes - perhaps they were being closed somehow in a finaliser?

Screenshot from 2020-01-17 00-14-05

Also, JProfiler revealed an accumulation of ZipInputStream objects from loading the .aiff JAR resource.

@stejbac stejbac requested a review from ripcurlx as a code owner Jan 19, 2020
Copy link
Member

ripcurlx left a comment

ACK - Tested it locally on Regtest. Toggling the avoid standy by feature (I can hear the sound behaving as before) everything works as before from a user perspective. Thanks for looking into this 👍

I just don't merge it immediately as I want to wait until #3889 gets merged so @cbeams doesn't need to do any unnecessary merges.

@ripcurlx

This comment has been minimized.

Copy link
Member

ripcurlx commented Jan 22, 2020

@stejbac Could you please resolve the conflict, caused by the recent gRPC PR merge?

@ripcurlx

This comment has been minimized.

Copy link
Member

ripcurlx commented Jan 22, 2020

Afterwards I'm happy to merge your PR

Replace tail recursion of the play() method with an ordinary loop, to
prevent a new open JAR resource InputStream + sound file OutputStream
(which were created every 4 minute playback) from accumulating on the
stack, closing them inside the loop instead. (This also prevents
eventual stack overflow.)

Also tidy up FileUtil.resourceToFile and put the JAR URL InputStream in
a try-with-resources block, to ensure that it doesn't leak either.
@stejbac stejbac force-pushed the stejbac:fix-silent-sound-player-resource-leak branch from d6d23cc to a073dbf Jan 22, 2020
@stejbac

This comment has been minimized.

Copy link
Contributor Author

stejbac commented Jan 22, 2020

I've rebased onto master. (The conflict in AvoidStandbyModeService was caused by imports and a move from BisqEnvironment.getAppDataDir() to config.appDataDir.)

Copy link
Member

ripcurlx left a comment

@ripcurlx ripcurlx merged commit 4243236 into bisq-network:master Jan 27, 2020
2 checks passed
2 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ripcurlx ripcurlx added this to the v1.2.6 milestone Jan 27, 2020
@stejbac stejbac mentioned this pull request Feb 10, 2020
@ripcurlx ripcurlx mentioned this pull request Feb 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.