-
Notifications
You must be signed in to change notification settings - Fork 2
feat: move docs map to docroom #83
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
base: main
Are you sure you want to change the base?
Conversation
LCOV of commit
|
bosschaert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks for looking into this!
src/edge.js
Outdated
| this.docs.set(docName, ydoc); | ||
| } | ||
|
|
||
| webSocket.addEventListener('close', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is correct. Wouldn't this basically remove the doc if a connection goes away?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On connection close, the doc is removed from the map ("local cache"). This is the same logic as before, just moved the code here to have access to the cache.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, what was missing in the new version is the check if there is no more connections. Added.
| // when bound. | ||
| doc.promise = persistence.bindState(docname, doc, conn, storage); | ||
| } | ||
| doc.promise = persistence.bindState(docname, doc, conn, storage, docsCache); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is not doing what happened before. IIRC, the idea was to use the promise to make sure the doc doesn't get bound to persistence called concurrently. @bosschaert, are you sure this is still correct (I would have expected to have this move up to the caller like it used to for the await)...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, the doc.promise should only be set if it wasn't there yet. Then we await on it in the next line of code. If it resolved already the await will be a no-op.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I restored the logic but split the method in 2: createYDoc and setupYDoc. Create is only called once, setup each time a new session starts.
Too bad this is not tested.
bosschaert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, but I would also get approval from @karlpauls
|
Do we want to pull this down to stage given the comments? |
|
We could go to stage first but this means someone would test there. I tested it locally as much as I can, basic features work as expected (editing, saving, awareness). What I cannot control is the long run on CF but this will be true also on staging. We can merge to prod and I'll monitor as I do for all changes I am doing on this project anyway. |
This is a first attempt to fix #76
@karlpauls and I brainstormed and the current hypothesis is:
DocRoomis the Durable Object.storage.ydocinstances which are cached in the docs map.docsobject is a global object, not necessarily bound to the Durable Object.storage)This is a pure hypothesis and we need to make sure:
In any case, I think we should continue re-working the code: the
DocRoomcode should be isolated in its own file and, in the ideal scenario, contain all code it need - not sharing anything, to avoid those kind of memory mix. This would allow to clearly visualize what belongs to the DO (super singleton) vs what belong to the worker.