CompassService memory leak #1810

Closed
sfPlayer1 opened this Issue Aug 16, 2015 · 4 comments

Comments

Projects
None yet
2 participants
@sfPlayer1

Apparently appeng.services.CompassService (temporarily?) leaks some (~5?) invalid world references through worldSet.

@yueh

This comment has been minimized.

Show comment
Hide comment
@yueh

yueh Aug 16, 2015

Member

Yes, there should be a potential memory leak when worlds are unloaded as it does not clean up the reference to the world in this case.

"Temporarily" is probably caused as a server stop will explicitly free these references. "some" most likely due to not everyone enters a world with a compass, thus the reader for this world is never loaded.

I would appreciate, if you could point in some directions to verify it. Also to test a potential fix and not just hope it might work.
For example just changing it to a WeakReference, can cause real memory leaks (native), as we are potentially no longer be able to close the compassdata files. So we most likely have to deal explicitly with WorldEvent.Unload for a cleanup.

Member

yueh commented Aug 16, 2015

Yes, there should be a potential memory leak when worlds are unloaded as it does not clean up the reference to the world in this case.

"Temporarily" is probably caused as a server stop will explicitly free these references. "some" most likely due to not everyone enters a world with a compass, thus the reader for this world is never loaded.

I would appreciate, if you could point in some directions to verify it. Also to test a potential fix and not just hope it might work.
For example just changing it to a WeakReference, can cause real memory leaks (native), as we are potentially no longer be able to close the compassdata files. So we most likely have to deal explicitly with WorldEvent.Unload for a cleanup.

@sfPlayer1

This comment has been minimized.

Show comment
Hide comment
@sfPlayer1

sfPlayer1 Aug 16, 2015

I unfortunately don't know any details as I've only stumbled across it through analyzing a heap dump after 1-2 days of uptime (FC2). I can only offer to install a new build and observe its behavior.

The finalizer of whatever holds the file descriptor should eventually close the file, which is of course no alternative to (also presumably) stopping the CompassService explicitly from the unload event.

I unfortunately don't know any details as I've only stumbled across it through analyzing a heap dump after 1-2 days of uptime (FC2). I can only offer to install a new build and observe its behavior.

The finalizer of whatever holds the file descriptor should eventually close the file, which is of course no alternative to (also presumably) stopping the CompassService explicitly from the unload event.

@yueh

This comment has been minimized.

Show comment
Hide comment
@yueh

yueh Aug 16, 2015

Member

Is FC2 currently on rv2 or rv3?

Member

yueh commented Aug 16, 2015

Is FC2 currently on rv2 or rv3?

yueh added a commit to yueh/Applied-Energistics-2 that referenced this issue Aug 19, 2015

Fixes #1810: Removes a CompassReader once the world is unloaded.
This should no longer keep a reference to a World around and potentially
keep them loaded.
Also added a finalize() to CompassRegion to ensure the file is closed on a
GC.

Some cleanup regarding member order, final, etc

yueh added a commit to yueh/Applied-Energistics-2 that referenced this issue Aug 20, 2015

Fixes #1810: Removes a CompassReader once the world is unloaded.
This should no longer keep a reference to a World around and potentially
keep them loaded.
Also added a finalize() to CompassRegion to ensure the file is closed on a
GC.

Some cleanup regarding member order, final, etc

@yueh yueh closed this in #1823 Aug 20, 2015

yueh added a commit that referenced this issue Aug 20, 2015

Merge pull request #1823 from yueh/fix-1810
Fixes #1810: Removes a CompassReader once the world is unloaded.
@yueh

This comment has been minimized.

Show comment
Hide comment
@yueh

yueh Aug 22, 2015

Member

Keep it open for further feedback.

Member

yueh commented Aug 22, 2015

Keep it open for further feedback.

@yueh yueh reopened this Aug 22, 2015

@yueh yueh closed this Jul 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment