Conversation
LCOV of commit
|
bosschaert
left a comment
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
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.
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.