Add waitForBookmark to DurableObjectStorage #2923
Conversation
justin-mp
force-pushed
the
jmp/waitForBookmark
branch
2 times, most recently
from
30f4a45 to
59ce3bd
Compare
|
59ce3bd is a rebase on main. |
| @@ -735,6 +735,10 @@ kj::Promise<kj::String> DurableObjectStorage::onNextSessionRestoreBookmark(kj::S | |||
| return cache->onNextSessionRestoreBookmark(bookmark); | |||
| } | |||
|
|
|||
| kj::Promise<void> DurableObjectStorage::waitForBookmark(kj::String bookmark) { | |||
| return cache->waitForBookmark(bookmark); | |||
There was a problem hiding this comment.
Can this safely be called multiple times from javascript? As it is here, every call on this will result in a new JavaScript promise backed by a kj::Promise... specifically, if I do..
const p1 = whatever.waitForBookmark('...');
const p2 = whatever.waitForBookmark('...');Then p1 !== p2
Should this always return the same JS promise when the bookmark id is the same and is already being waited on?
There was a problem hiding this comment.
The internal PR waits on the same forked promise, so in production this will wait on the same thing. p1 will still be !== p2, is that a problem?
There was a problem hiding this comment.
Not a problem just something to be aware of. Creating multiple promises that wait on the same thing can add performance cost.
There was a problem hiding this comment.
I think this is fine as-is. Don't think people are likely to call it multiple times in a row like that.
There was a problem hiding this comment.
But for @justin-mp's understanding: One problem with waiting on multiple forks of the KJ promise is that each one's resolution will lead to a separate entering of the isolate, requiring taking the isolate lock, setting up CPU time limits, etc., which is relatively expensive. If we somehow imported just one instance of the KJ promise to JS, and then had all the waiters wait on that shared JS promise, then the isolate only needs to be entered once.
But this is an optimization which can easily be left for later.
This exposes the internal `waitForBookmark` method to JavaScript. We’ll need this in order to implement decent consistency models with SQLite replication.
justin-mp
force-pushed
the
jmp/waitForBookmark
branch
from
59ce3bd to
4d5c89c
Compare
`waitForBookmark` will be part of the public API of read replication, but given that we’re still implementing read replication, this should be behind a flag. I should have done this in #2923.
This exposes the internal
waitForBookmarkmethod to JavaScript. We’ll need this in order to implement decent consistency models with SQLite replication.