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 in Direct Connection #895

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

bencarletonn
Copy link
Contributor

  • As described in Memory leak in direct connection #846 there is a scenario where: a server calls .disconnect() on a direct connection to a document, the document is also loaded in an alternative server, occasionally the redis extensions onStoreDocument returns a rejected promise due to failing to acquire a lock
  • The rejected promise prevents the promise chain .then to continue to call storeDocumentHooks, preventing afterStoreDocument, unloadDocument and afterUnloadDocument to be called. This is how the memory leak occurs
  • The redis extensions afterStoreDocument is never triggered. That's fine, we don't need to release a lock that we never acquired.
  • More importantly, since we do not trigger the onDisconnect hooks when calling .disconnect() on the direct connection, the redis extensions onDisconnect logic isn't triggered either. The fix to unload the document associated with the direct connection here https://github.com/ueberdosis/hocuspocus/pull/831/files is never reached.

I don't think we should unload the document if the store document hook returns a rejected promise. Take for example, an autosave that is implemented through a debounced onStoreDocument. We wouldn't want the server to suddenly unload the active document if for some reason that autosave failed.

However, if we confirm that the direct connection is the ONLY connection to that document, we can trigger the 'onDisconnect' hook and can call unloadDocument for that document when we call .disconnect() on the direct connection. This should ensure the document is unloaded, consequently preventing unused loaded documents from accumulating on the server.

@bencarletonn
Copy link
Contributor Author

@janthurau any update on this one?

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.

1 participant