Skip to content

fix: Memory leak in Direct Connection#895

Merged
janthurau merged 3 commits intoueberdosis:mainfrom
bencarletonn:direct-connection-memory-leak
Feb 20, 2025
Merged

fix: Memory leak in Direct Connection#895
janthurau merged 3 commits intoueberdosis:mainfrom
bencarletonn:direct-connection-memory-leak

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?

@bencarletonn
Copy link
Contributor Author

@janthurau bumping this again since this is causing problems

@janthurau janthurau force-pushed the direct-connection-memory-leak branch from 8b75816 to c786618 Compare February 20, 2025 18:53
@janthurau janthurau merged commit 70ab0e2 into ueberdosis:main Feb 20, 2025
3 checks passed
@bencarletonn bencarletonn deleted the direct-connection-memory-leak branch March 19, 2025 15:22
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.

2 participants