ueberdosis / hocuspocus

The CRDT Yjs WebSocket backend for conflict-free real-time collaboration in your app.
https://tiptap.dev/docs/hocuspocus/introduction
MIT License
1.29k stars 125 forks source link

Unnecessary onStoreDocumentHooks call on last connection close (potential data loss) #766

Closed ebads67 closed 9 months ago

ebads67 commented 10 months ago

Description Currently onStoreDocument hooks are called using the debouncer logic in two cases:

  1. On every changes applied to a document (if not coming from redis)
  2. On the last connection close

The latter case clears previously scheduled onStoreDocument hooks by the debouncer, and enforces a new onStoreDocument hooks call even if there has not been any changes to the document since the last store. It appears that it is unnecessary to call onStoreDocument hooks if there is no debounced onStoreDocument hook scheduled to be executed. Because all the changes to the document have already been stored.

Even in the case when the last connection is closing and there is a debounced onStoreDocument waiting to be executed, isn't it better to run that instead of running a new onStoreDocument hooks chain?

This has made an issue in our use case where we have two types of connections; connections with write permission and read-only connections. Read-only connections are for the users with only read permissions and are not able to make any changes to the document. So our customized onStoreDocument logic should not be executed for this type of users/connections. But if a read-only user closes the last connection, onStoreDocument is called for this user, and our customized onStoreDocument fails (our downstream system rejects the update operation). This can cause data loss, as any previously debounced onStoreDocument by users with write permission are discarded.

janthurau commented 9 months ago

Thanks for raising this, and more thanks for providing the pull request!

Will be released this week.