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

[BUG] Potential memory leak #64

Open
koenbeuk opened this issue Oct 25, 2023 · 3 comments
Open

[BUG] Potential memory leak #64

koenbeuk opened this issue Oct 25, 2023 · 3 comments
Labels
bug Something isn't working

Comments

@koenbeuk
Copy link
Contributor

We're using this library in production for sending non-critical signals to users. Over time we notice a degradation in performance and an increase in memory consumption until at some point we get an application restart with an OOM message.

We suspect the problem is with RewindableMessageGrain. As its associated to Clients (which come and go) and persists messages in state, State however is not cleared when the client goes away and will always stay persisted.

2 relatively easy wins would be to never persist when MaxMessageRewind is configured as 0 and to make persistence optional. A proper fix would involve observing the client and clearing state when the client goes away.

We're using MemoryStorage which exacerbates the issue. If we were to use a proper storage provider then we would expect to see an ever increasing db size.

I'm happy to contribute here.

@koenbeuk koenbeuk added the bug Something isn't working label Oct 25, 2023
@LiamMorrow
Copy link
Owner

Ah yeah that's definitely an issue, I'd say even (to a lesser extent) even with the User and Group grains which contain a hashset of the connectionIds. It's potentially worth having some sort of TTL since last accessed, and a monitor grain which clears the state for each entity.

@koenbeuk
Copy link
Contributor Author

koenbeuk commented Oct 26, 2023

Opened up a PR that would partly solve this issue, I agree that it's not completely solved and that we need some TTL mechanism. Would really appreciate it if the PR could be accepted and a new version could be released.

@LiamMorrow
Copy link
Owner

Thanks for that! Gave it a test, and seems to work without issue, releasing 2.3.1 now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants