classic-level icon indicating copy to clipboard operation
classic-level copied to clipboard

as far I see there is no working createIfMissing=false anymore.

Open axkibe opened this issue 9 months ago • 13 comments

See following minimal node.js example:

import { default as level } from 'level';
import timers from 'node:timers/promises';

const db = new level.Level( './dbxx', { passive: true, createIfMissing: false } );
await timers.setTimeout( 1000 );
console.log( 'nothing should be done' );

After this is finished, there will be a directory called "dbxx".

in classic-level/index.js it is forced to true, and I dont see the options overriding it anywhere. (also if testing I change this force to false the underlying layer implementation does create the directory anyway)

My use case: I want to offer the user a GUI, where they can point at a directory and then I'll check if this a level db in the format I expect (I tested for a specific key=value pair). Doing this naively will create unwanted level db instances all over the place..

Workaround I came up with: place a marker "MYAPP.meta" in the level db directory and check for that file along with CURRENT and the MANIFEST-? before trying to open level db.. (and thus always creating the database if not existent)

Would be nice tough if this functionality that existed as far I am aware with leveldown existed would be working again.

PS: level version 9.0.0 classic level 2.0.0 abstract level 2.0.2

axkibe avatar Apr 07 '25 15:04 axkibe

Does it merely create the directory or are there also LevelDB files inside? Which platform? We have unit tests for this with one known issue:

https://github.com/Level/classic-level/blob/15eb2893086ff52f6ce5b2b79a21ca6340b966fc/test/mkdir-test.js#L32-L33

vweevers avatar Apr 07 '25 15:04 vweevers

in classic-level/index.js it is forced to true

If you're referring to the following line - that's not the option. This is a so-called manifest that tells abstract-level that this implementation supports the createIfMissing option (because not all implementations do).

https://github.com/Level/classic-level/blob/15eb2893086ff52f6ce5b2b79a21ca6340b966fc/index.js#L25

vweevers avatar Apr 07 '25 16:04 vweevers

Sorry forgot to mention: Linux.

After running above minimal example

~/ls dbxx/
LOCK  LOG

Ah sorry, I misunderstood the "manifest" then (to be honest the code base is a little over my head)

using createIfMissing = true as option to the creator is not really in the documentation anyway, it used to be part of open() and still is, but with the (somewhat) recent API changes I/O already starts automatically after the constructor (as soon the main flow control releases)

axkibe avatar Apr 07 '25 16:04 axkibe

Might be caused by the combination with passive (just because that's not a use case I anticipated and may have some unintended side effect) though at quick glance I don't see why it wouldn't work.

vweevers avatar Apr 07 '25 16:04 vweevers

passive makes no difference, it was just additional defense I tried.

axkibe avatar Apr 07 '25 17:04 axkibe

In that case, yes, looks like a bug. I will need to debug, can't say when I'll have time.

vweevers avatar Apr 07 '25 17:04 vweevers

This is an expected behavior (also in leveldown) at least from the LevelDB side. It creates the directory and takes a lock before checking if the "database" exists:

https://github.com/Level/classic-level/blob/15eb2893086ff52f6ce5b2b79a21ca6340b966fc/deps/leveldb/leveldb-1.20/db/db_impl.cc#L280-L302

I quoted "database" because it's conceptually different from your expectation, which is that a database is a non-empty directory.

vweevers avatar Apr 12 '25 15:04 vweevers

"I quoted "database" because it's conceptually different from your expectation, which is that a database is a non-empty directory."

I don't understand, or I think there is a misunderstanding here, a "database" for me is something.. well how to say it, that is an initialized leveldb database (naively if it has at least one key/value pair this is certainly true, but technically even without).

I did look through the code, and it did puzzled me it would create the directory without even looking at the "createIfMissing" flag (which is looked at only further down in the code), which at least how I expect it shouldnt do.

Or said differently, I would expect "createIfMissing=false" do not alter the file system, unless there is already a created database right there.

Currently I'm working around this, by looking in my code if there is a CURRENT file and additionally a marker I place, right now I do this code, before any call to level:

async function getDbPathState(path) {
  let files;
  try {
    files = await fs.readdir(path);
  } catch (e) {
    switch (e.code) {
      case 'ENOENT':  return 'not-exist';
      case 'EACCES':  return 'no-access'; // Fixed typo: 'EACCESS' → 'EACCES'
      default:        return 'unknown';
    }
  }

  const hasPlotleMeta = files.includes('PLOTLE.meta');
  const hasCurrent = files.includes('CURRENT');

  if (hasPlotleMeta) {
    return hasCurrent ? 'ok' : 'broken';
  } else {
    return hasCurrent ? 'not-plotle' : 'exist';
  }
}

Right now it missing looking into the connect of my meta file, to verify version number.

What I would expect to do would be something like this (which I came first up with)

const db = new level.Level( path, { createIfMissing: false } );
try {
  await db.open( );
  let version = await db.get( 'version' );
  version = JSON.parse( version );
  if( version.id != 'plotle' ) return 'not-plotle';
} catch ( e ) {
  // error handling, deciding if its a general file io error, or it just isnt a valid level database.
}
return 'exist';

But i cannot because this will create directories and files, albeit the user UI just probed if there is a valid database there.

axkibe avatar Apr 13 '25 13:04 axkibe

Or said differently, I would expect "createIfMissing=false" do not alter the file system, unless there is already a created database right there.

That is a fair expectation, but not how LevelDB works. It uses the filesystem (specifically the LOCK file) to lock around the operation of creating the database. Database !== directory.

LevelDB does this because it doesn't support using the database from multiple processes simultaneously.

We could maybe handle this by having classic-level check if the directory exists beforehand - and if not, skip further work. We would have to accept the overhead and that this step won't have a lock (aka synchronization). Meaning, if 2 processes try to create the directory and database at the same time, they will both create the directory (so to speak) but only one process will succeed to create the LevelDB database.

vweevers avatar Apr 13 '25 14:04 vweevers

I know locking is a conceptional issue, otherwise I would have asked for readonly support, but I realize how this is not possible.

but if the LOCK file doesnt exist, then there is no valid database, right? And I guess the kernel API supports that opening exclusive, but failing if the file doesnt exist (O_EXCL without O_CREAT), no?

But I dont know how the level spec is, technically of course a database could be OK, just LOCK missing..

axkibe avatar Apr 13 '25 14:04 axkibe

but if the LOCK file doesnt exist, then there is no valid database, right?

I don't have a clear answer to that. LevelDB itself has no reason to delete the LOCK file as far as I know, but, it also won't complain if you manually delete the LOCK file (after closing the database). It will just recreate it.

vweevers avatar Apr 13 '25 14:04 vweevers

I just had a thought, how to approach this differently, I wonder what is the current use case of "createIfMissing=false", if it will create the directory, LOCK and LOG regardless?

I totally dig the minimalism and simplicity of level, but for this feature, either I am missing the imagination for this use case, or createIfMissing should either work as outlined (no alteration if not a valid db) or we can scrap it altogether and say users should manually check similar like I did as workaround, if they need to.

axkibe avatar Apr 13 '25 15:04 axkibe

We've had this option for more than 10 years, long before I joined, and personally I've never needed it. But I would guess it's for initialization, when combined with errorIfExists:

if (firstRun) {
  db = new ClassicLevel('./db', { errorIfExists: true })

  // Some form of initialization here
  await db.put('created', Date.now())
} else {
  db = new ClassicLevel('./db', { createIfMissing: false })
}

Here it does not matter that the else-path can create LOCK and LOG files. So createIfMissing works as intended for this use case.

vweevers avatar Apr 13 '25 16:04 vweevers