canvasxyz / okra-js

MIT License
26 stars 2 forks source link

Fix okra-idb nodes and entries to handle null transaction OK #3

Open garbados opened 9 months ago

garbados commented 9 months ago

The nodes and entries methods on okra-idb's IDBTree contain this worrying comment:

// TODO: fix this

The absence of logic for handling a null transaction means that methods like sync fail, like this:

Error: can only call nodes() from within a managed transaction

I've prototyped a simple fix, but I'm not familiar with the ins and outs of this project enough to add tests and such.

    async *nodes(level, lowerBound = null, upperBound = null, { reverse = false } = {}) {
        if (this.store.txn === null) {
            return this.store.read(() => this.nodes(level, lowerBound, upperBound, { reverse }))
        }
        else {
            yield* super.nodes(level, lowerBound, upperBound, { reverse });
        }
    }
    async *entries(lowerBound = null, upperBound = null, { reverse = false } = {}) {
        if (this.store.txn === null) {
            return this.store.read(() => this.entries(lowerBound, upperBound, { reverse }))
        }
        else {
            for await (const leaf of this.nodes(0, lowerBound ?? { key: null, inclusive: false }, upperBound, { reverse })) {
                assert(leaf.key !== null && leaf.value !== undefined, "invalid leaf entry");
                yield [leaf.key, leaf.value];
            }
        }
    }

That seems to do the trick on my machine, for whatever that's worth. Not sure if there are more complicated caveats lurking in the details.

garbados commented 9 months ago

For context, I'm developing an HTTP server whose API supports a ServerTree class which implements Tree, using the aforementioned server to manage persistence. This allows CouchDB-like syncing between a server and a browser. Of course, as is, I can only sync from the browser to the server. The other way around runs into this issue of null transactions. The fix I shared allows me to sync both ways, but as I mentioned, I don't know that my provisional fix is a serious solution.

joeltg commented 9 months ago

The fundamental issue here is with IndexedDB, where transactions auto-commit as soon as they reach zero scheduled operations. Transaction operations are async, but you can't hold the transaction open for arbitrarily long. You can make a series of async operations as long as you start each one in the same event loop tick that the last one resolved in, but as soon as there are no scheduled ticks the transaction closes itself and will throw an error if you try to use it again. A lot of awkwardness within okra-idb comes from trying to work around this.

Anyway, the way this is relates here is that your patch is correct and it'll work great AS LONG AS you don't await any other promises inside the nodes() or entries() iterators. The iterator body has to be synchronous, even though it's an AsyncGenerator.

I had left this as a TODO because I wasn't sure if there was a different way I could arrange this that didn't have this limitation... for now I guess I'll just do the thing you did and document the constraint.

joeltg commented 9 months ago

More generally: to use sync, you need to implement the Source interface for the remote database ("Source" == "where the data comes from"... maybe should have named this something else).

interface Source {
    getRoot(): Awaitable<Node>
    getNode(level: number, key: Key): Awaitable<Node | null>
    getChildren(level: number, key: Key): Awaitable<Node[]>
}

So in order to sync server-to-browser, where the browser acts as the "server" and the server acts as the "client", you need to make getRoot, getNode, and getChildren requests from the server to the browser. You probably can't do this with just an HTTP API, which is request-response in the wrong direction (easy to use for browser-to-server syncing though). You'll need WebSockets, WebRTC, WebTransport, or something else that lets the server dispatch requests to the browser.

Also, those three methods are already methods on the Tree class, so really you just have to proxy them... the getChildren method of IDBTree already handles creating a transaction if one doesn't exist before calling .nodes(), so you should never really have to call .nodes() or .entries() yourself during syncing.

garbados commented 9 months ago

That's an interesting note about the HTTP server needing to make requests of the browser. I hadn't noticed it in my testing but I admit my testing wasn't exactly rigorous. I'll look into using WebRTC or such.