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

Optionally disable execution records #5

Open
vatbub opened this issue Nov 15, 2022 · 1 comment
Open

Optionally disable execution records #5

vatbub opened this issue Nov 15, 2022 · 1 comment

Comments

@vatbub
Copy link

vatbub commented Nov 15, 2022

Hi there,
first thank you for this nice and simple library!
I am facing an issue where my commands hold references to data structures that take up a fair bit of RAM. When I ran into memory limit issues, I debugged my application and ran into the following culprit:

After publishing lots of commands that reference huge amounts of RAM, I publish one command that is not undoable. As a consequence, the undo stack is cleared and I would expect that the commands that were in the undo stack are deallocated and can thus be picked up by the garbage collector. This, however is not the case, as private final SortedSet<ExecutionRecord> executionRecords in CommandDelegator.java keeps references to all commands ever executed, even after the undo stack is cleared.

I understand that the execution record mechanism can be useful, but in my case, it causes a memory leak. My suggestion would therefore be to make it optional or at least have the option to clear it.

Cheers :)

vatbub pushed a commit to vatbub/re-agent that referenced this issue Nov 15, 2022
@vatbub vatbub changed the title Otionally disable execution records Optionally disable execution records Nov 15, 2022
@vatbub
Copy link
Author

vatbub commented Nov 15, 2022

I just realized while testing that disabling the mechanism and thus disabling the listener mechanism as well might not be such a good idea. I decided to create a private fork where I removed CommandDelegator.executionRecords and CommandDelegator.getExecutionRecords(). This way, the memory leak is fixed while keeping the listeners working. This would be a breaking api change though, which is why I decided to not include that in my PR.
Cheers :)

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

No branches or pull requests

1 participant