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

Redis extension not subscribing to changes after document is closed and reopened. #131

Closed
georeith opened this issue Jul 9, 2021 · 5 comments
Labels
bug Something isn't working

Comments

@georeith
Copy link
Collaborator

georeith commented Jul 9, 2021

I am running into an issue where the redis extension works correctly the first time the server is restarted, however after the initial connection to a document is closed, whenever a new connection is reopened it no longer subscribes to changes of that document.

This is what I think is happening:

  1. When you first restart the server a YJS doc is created, this is subscribed to by the y-redis package, because the y-redis package is subscribing to the same document that is being edited in memory it is aware of the changes
  2. After that document is closed (all clients disconnect) the y-redis package still remembers the document but @hocuspocus/server forgets the document
  3. When a client now connects to that document @hocuspocus/server creates a new in memory document and the y-redis package does not subscribe to it because it remembers it.
    The issue with 3. is that the onCreateDocument hook claims that if you return a document from that hook it is loaded. Which the @hocuspocus/redis package does (
    return binding.doc
    ), however if you look at the implementation of that hook it doesn't swap the in-memory doc for the one you use it merges them together (
    applyUpdate(document, encodeStateAsUpdate(loadedDocument))
    ), which means you end up with the same in-memory doc being used by @hocuspocus/server which is not the document that y-redis is subscribed to.

I think this can be solved simply by having the @hocuspocus/redis move the creation of RedisPersistence to onConnect and destroy it in onDisconnect when its the last client:

    async onConnect(data) {
        if (!this.persistence) {
            this.persistence = new yRedis.RedisPersistence(
            // @ts-ignore
            this.cluster
                ? { redisClusterOpts: this.configuration }
                : { redisOpts: this.configuration });
        }
    }
    async onDisconnect(data) {
        if (data.clientsCount === 0) {
            this.persistence.destroy();
            this.persistence = undefined;
        }
    }

I have tested this locally and it works for me

@georeith georeith added the bug Something isn't working label Jul 9, 2021
@hanspagel
Copy link
Contributor

Thanks for the report! I refactored the Redis extension. Unfortunately there’s still a major issue I can’t really grasp. Sometimes changes aren’t correctly sent to the server.

I’m still waiting for a rewrite of y-redis, not sure if it’s worth the time to look into issues related to Redis now - or if it’ll be better to wait for the rewrite.

@georeith
Copy link
Collaborator Author

georeith commented Jul 14, 2021

@hanspagel thanks for that refactor, will see if I can run into that issue you posted above. By not correctly sent to the server you mean @hocuspocus/server receives the update and @hocuspocus/extension-redis doesn't or @hocuspocus/server doesn't receive it at all?

I saw that mentioned a couple of times that the redis adapter is due a rewrite, would be interested in reading what is due to be changed with it. I noticed it in another thread about horizontal scalability which is something we're super interested in too. But was also interested if there were plans to allow reduction of the data size in it by doing set of the whole document instead of append of individual updates (as appending updates for our data at least ends up growing a lot larger than encoding state at any given time as an update as we have a lot of garbage collected data from update to update).

@solirpa

This comment has been minimized.

@dmonad
Copy link
Collaborator

dmonad commented Jul 23, 2021

Hi @solirpa,

@hanspagel is still patiently waiting for my rewrite of y-redis. I'm aware that a ready-to-use solution for horizontal scaling is a highly requested feature. Another requested feature is the ability to sync all documents that a user has access to with a backend. I spent the last weeks finding a new scalable approach that allows syncing thousands of documents efficiently over a single connection for offline usage. I took some inspiration from the exchanges that I had with the author of Legendkeeper. The y-redis approach that I'm working on will be scalable and will enable syncing subdocuments. This significantly increases complexity, but I'd rather work on a complete solution than an incomplete solution that only works for some.

I updated the current y-redis implementation which should be usable and allow horizontal scaling. I will release some information about the rewrite of y-redis that allows syncing of subdocuments soon.

@hanspagel hanspagel moved this to Consider in hocuspocus Oct 30, 2021
@hanspagel
Copy link
Contributor

Thanks for reporting! I don’t think this issue applies to the upcoming rewrite of the Redis extension #285.

Repository owner moved this from Consider to Done in hocuspocus Feb 28, 2022
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
No open projects
Status: Done
Development

No branches or pull requests

4 participants