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 memory leak #282

Merged
merged 1 commit into from
Sep 29, 2016
Merged

fix memory leak #282

merged 1 commit into from
Sep 29, 2016

Conversation

muzuro
Copy link
Contributor

@muzuro muzuro commented Sep 28, 2016

I noticed a memory leak in my production environment. It can be reproduced with custom TransportCosts and huge array in it(after some amount of invocation same vrp, without stopping jsprit jvm and transportCost should be created each invocation, you wil recieve OOM). If we don`t remove shutdown hook it will be stored in runtime.

@muzuro muzuro mentioned this pull request Sep 28, 2016
@oblonski
Copy link
Member

Great. Thanks a lot @muzuro. Let me reproduce this first and recall why we actually need this hook ;).

@oblonski
Copy link
Member

You are absolutely correct. Could reproduce is. This is evil. Thank you very much.

@oblonski oblonski merged commit 915b296 into graphhopper:master Sep 29, 2016
@karussell
Copy link
Member

Additionally I would move the addition to the shutdown into the if. IMO something 'external' should not be manipulated.

oblonski added a commit that referenced this pull request Sep 29, 2016
@oblonski
Copy link
Member

already done 👍 . thanks.

@karussell
Copy link
Member

BTW1: Wouldn't it then make sense to have just one listener and add

Runtime.getRuntime().removeShutdownHook(hook);

BTW2: here is the problematic leaking explained for inner non-static classes

@oblonski
Copy link
Member

oblonski commented Sep 29, 2016

Heap memory before and after fix :)
screen shot 2016-09-29 at 13 01 53
screen shot 2016-09-29 at 12 59 41

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.

3 participants