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 loop memory leak #3865

Merged
merged 4 commits into from
May 13, 2021
Merged

Conversation

TPGamesNL
Copy link
Member

@TPGamesNL TPGamesNL commented Mar 28, 2021

Description

Fix a memory leak in loops by removing from the Loop#current map after the loop is done.
The memory leak is caused by some plugin storing an instance of the event used, so the WeakHashMap entries aren't removed.

Currently, if the stop / return effect is used within the loop, the memory leak may still occur.


Target Minecraft Versions: any
Requirements: none
Related Issues: #2337 #2877

@TPGamesNL TPGamesNL added 2.5 bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. labels Mar 28, 2021
@TPGamesNL
Copy link
Member Author

Loops are bound to the Event instance, just like local variables, so the best way to fix this is to include it in the methods used to store/restore local variables (Variables.removeLocals and Variables.setLocalVariables) (although those methods should be renamed, and return some object that includes both the VariablesMap and the loop stuff)

@TPGamesNL TPGamesNL marked this pull request as draft May 9, 2021 20:26
@TPGamesNL
Copy link
Member Author

TPGamesNL commented May 13, 2021

Assuming there is a strong reference for the Event instance (even when the event has long passed), I see two ways to fix this:

  1. Explicitly tell the Loop to remove the map entries for the event when:
    1. The loop is over.
    2. EffExit is used (one that exits the loop)
    3. EffReturn is used.
  2. Tell all Loops to remove map entries, when TriggerItem.walk / Trigger.execute ends. If we choose this, any async effects / delays will corrupt loops and delete the current loop-value (similar to what used to happen to local variables), unless we specifically add a way to restore them (as with Variables.removeLocals and Variables.setLocalVariables).

If we assume Event instances are GC'd at some point, this has already been fixed with #3860.

@TPGamesNL TPGamesNL marked this pull request as ready for review May 13, 2021 11:46
@TPGamesNL
Copy link
Member Author

After discussing with devs, we chose the first option

@FranKusmiruk FranKusmiruk merged commit ea9592a into SkriptLang:master May 13, 2021
@TPGamesNL TPGamesNL deleted the fix/loop-memory-leak branch May 13, 2021 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue that needs to be fixed. Alternatively, a PR fixing an issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants