Reduce scope to what is strictly needed in levelup

[vweevers]

TLDR: thumbs up if you agree that deferring operations is only useful while the db is opening.

I'm working on getMany(). Which is a new abstract-leveldown and levelup method that gives abstract-leveldown the responsibility of checking whether the db is open when getMany() is called. As an experiment for Level/community#58. But I'm running into an inconsistency that's particularly problematic in subleveldown (which has to deal with two instances of deferred-leveldown and levelup, and is a test bed for API parity).

deferred-leveldown and levelup are inconsistent in their deferredOpen behavior: the former allows operations while the db has any status, the latter only if 'opening' or 'open'. For example, if the status is 'closed', deferred-leveldown will put operations in its queue. But in practice we don't get there because levelup will yield a new ReadError('Database is not open').

@ralphtheninja @juliangruber Given that the order of initialization in levelup is:

• Wrap input db with deferred-leveldown
• Start opening it (changing its status from 'new' to 'opening')
• Start accepting operations (after returning from constructor)

I propose the following changes:

• deferred-leveldown will only defer operations if the status of its underlying db is 'opening'
• Otherwise it will yield new Error('Database is not open').

This is a breaking change for deferred-leveldown, but semver-patch for levelup.

See also Level/levelup#710 (comment).

[juliangruber]

• deferred-leveldown will only defer operations if the status of its underlying db is 'opening'

I think there are some more cases we could discuss. Say the db was closed, should another operation cause it to reopen? Or it could even be currently closing and a consumer could expect the next operation to reopen it.

If after closing a db the user wants to use the deferred-leveldown behavior again, they need to ensure they called db.open(), so that it's in the opening state, right? I think that makes sense to me. Just want to check it with you

[vweevers]

If after closing a db the user wants to use the deferred-leveldown behavior again, they need to ensure they called db.open(), so that it's in the opening state, right? I think that makes sense to me. Just want to check it with you

Right. The way I see it, deferredOpen is a convenience to avoid having to open the db. It goes away if the user explicitly closes the db. It then becomes the user's responsibility to explicitly open the db again. After which the convenience is back.

I reckon in most if not all cases, if the user is closing the db, they're not expecting to do further work on the db, and scheduling operations should be an error.

[vweevers]

Which is to say, yielding new Error('Database is not open') is also a convenience :)

[ralphtheninja]

deferred-leveldown feels a bit hacky to me. I just want to entertain the idea of removing it in the long run. My gut feeling tells me we're having to fight it all the time and it's difficult to see what's going on. Might be a lot of work. I'm not saying we should. Just to think about it 😄

[vweevers]

Oh, I definitely want to remove it. Just gradually.

[juliangruber]

I want to remove it as well. I think we mostly had it because of callback inconvenience, now that we can await db.open() there's no big advantage any more

[juliangruber]

I reckon in most if not all cases, if the user is closing the db, they're not expecting to do further work on the db, and scheduling operations should be an error.

I think you're right. The easiest path for them is to dispose of the db completely after closing it, and then starting fresh. So only treating the opening state makes sense 👍

[vweevers]

To clarify, I want to remove deferred-leveldown, but not necessarily deferredOpen (as a feature). It does have a major benefit on sublevels for example. Don't want to have to do:

await db.open()
const sub = require('subleveldown')(db)
await sub.open()

That, plus widespread usage of deferredOpen, and that we may not be seeing other benefits, is why I want to remove it gradually. We may find a new place for it, or that opening sublevels isn't necessary, or... We'll see.

[juliangruber]

Why don't you want to have to do that? That code looks fine to me. Async operations are cheap now, I think we can remove deferredOpen altogether

[vweevers]

Asynchronicity is viral. Let's say a module takes a db as input, for example function myIndexer(db) which internally creates sublevels to store indexed data - or just to have a fresh db instance that can safely be monkeypatched. And it can't write to those sublevels until a await sub.open(). Assume that this module wants to intercept writes on the input db, in order to update indexes. Then, because of the required async initialization, it must change its signature to async function myIndexer(db).

[juliangruber]

Pseudo code:

const myIndexer = db => {
  db.put = async (key, value) => {
    await sub.open()
    // ...
  }
}

Wouldn't something like this work? By moving the async operation into the db hooks

[vweevers]

That just moves the problem. I think in userland you shouldn't have to worry about open/close state; saves boilerplate and prevents race issues.

deferredOpen isn't necessarily a complex feature (that should therefore be removed). The problem I have with the current implementation is that the feature is incomplete without levelup. Because it's levelup that auto-opens the db (and emits the events that are needed for logic like db.once('open', cb) if we wanted to implement it elsewhere). So:

require('levelup')(db).supports.deferredOpen // true
require('deferred-leveldown')(db).supports.deferredOpen // false!