Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added fresh parameter and readiness tests for EncryptedFS #58

Merged
merged 4 commits into from
Jan 13, 2022

Conversation

CMCDragonkai
Copy link
Member

@CMCDragonkai CMCDragonkai commented Jan 12, 2022

Description

The EFS didn't have a fresh parameter. Adding this in to match MatrixAI/js-db#6.

Also noticed that EFS doesn't have readiness testing. I'm adding this in to tests/EncryptedFS.test.ts.

Related to MatrixAI/Polykey#266 (comment)

Tasks

  1. Add fresh to EncryptedFS and maybe INodeManager
  2. Readiness testing
  3. Added promise rejection to subleveldown() and listening for open errors when subleveldown is called

Final checklist

  • Domain specific tests
  • Full tests
  • Updated inline-comment documentation
  • Lint fixed
  • Squash and rebased
  • Sanity check the final build

@CMCDragonkai
Copy link
Member Author

As discovered here, EFS is not passing a persistence test: MatrixAI/Polykey#266 (comment).

This may have something to do with INodeManager not having reset up everything because it is CD, and its setup is not redone during EncryptedFS.start.

@CMCDragonkai
Copy link
Member Author

A variant of this is:

    const efs1 = await EncryptedFS.createEncryptedFS({
      dbPath: dataDir,
      dbKey,
      logger,
    });
    await efs1.writeFile('testfile', 'hello world');
    await efs1.stop();
    await efs1.start();
    try {
      await efs1.readFile('testfile', { encoding: 'utf-8' });
    } catch (e) {
      console.log('OH NO', e);
    }
    await efs1.stop();

If the readFile is done on a separate file, we get ENOENT which is a correct error. But if we try to read testfile itself, we end up with the Inner database is not open internally.

That's super weird.

However it seems strange we cannot get the underlying exception of Inner database is not open.

@CMCDragonkai
Copy link
Member Author

Had to create an upstream issue about this error: Level/subleveldown#109

@CMCDragonkai
Copy link
Member Author

Managed to replicate the error.

import DB from './src/DB';

async function main () {
  const db = await DB.createDB({
    dbPath: './tmp/db',
  });
  const mgrDomain = ['INodeManager'];
  const dataDomain = [mgrDomain[0], 'data'];
  const mgrDb = await db.level(mgrDomain[0]);
  const dataDb = await db.level(dataDomain[1], mgrDb);
  await db.put(['INodeManager', 'inodes', '123'], 'a', 'b');
  await db.stop();
  await db.start();
  await db.level('123', dataDb);
  console.log(await db.get(['INodeManager', 'inodes', '123'], 'a'));
  await db.stop();
}

main();

Upon calling await db.level('123', dataDb); we get this error:

/home/cmcdragonkai/Projects/js-db/node_modules/subleveldown/leveldown.js:142
    if (self.leveldown.status !== 'open') return cb(new Error('Inner database is not open'))
                                                 ^
OpenError: Inner database is not open
    at /home/cmcdragonkai/Projects/js-db/node_modules/subleveldown/node_modules/levelup/lib/levelup.js:119:23
    at /home/cmcdragonkai/Projects/js-db/node_modules/deferred-leveldown/node_modules/abstract-leveldown/abstract-leveldown.js:38:14
    at /home/cmcdragonkai/Projects/js-db/node_modules/deferred-leveldown/deferred-leveldown.js:31:21
    at /home/cmcdragonkai/Projects/js-db/node_modules/encoding-down/node_modules/abstract-leveldown/abstract-leveldown.js:38:14
    at /home/cmcdragonkai/Projects/js-db/node_modules/subleveldown/node_modules/abstract-leveldown/abstract-leveldown.js:38:14
    at onopen (/home/cmcdragonkai/Projects/js-db/node_modules/subleveldown/leveldown.js:142:50)
    at processTicksAndRejections (internal/process/task_queues.js:77:11)

@CMCDragonkai
Copy link
Member Author

I believe this would known as an uncaught rejection. Because this is not actually received by the catch handler of the level call.

Furthermore I realised that I don't have the types for levelup, level, subleveldown and abstract-leveldown. So I have installed them now:

npm install --save-dev @types/subleveldown
npm install --save-dev @types/abstract-leveldown
npm install --save-dev @types/levelup
npm install --save-dev @types/level

Now at least I can see what kind of event types we can have. Maybe help get access to the open error.

@CMCDragonkai
Copy link
Member Author

CMCDragonkai commented Jan 12, 2022

Through some sleuthing I found a way to actually acquire the error. It's quite sneaky.

So basically subleveldown ends up "constructing" the AbstractLevelDOWN. This is done in: https://github.com/Level/subleveldown/blob/f132be81a01fd1260f3bc4e8801185707154a4a7/leveldown.js#L141

During this construction, it will call the abstract level down constructor. Which is actually not in abstract-leveldown, nor in leveldown, but in levelup.

This ends up calling this.open(). https://github.com/Level/levelup/blob/46d6e183d1d93404417c107f68698654ba6aca52/lib/levelup.js#L55-L57

Notice that the callback passed into that is either the callback passed by the user at the beginning of constructing a levelup instance. Or it is given a default value of a callback that just does this.emit('error', err);.

This is the callback that is actually being called with the error object. And this would normally be the callback passed in by us when we create the leveldb object. In our start method we have level() and we do have a callback there.

So it turns out that subleveldown actually calls the open call for the inner db.

Now because the error is emitted on the constructed sublevel instance, that instance is what we need to watch for errors, not the root leveldb instance.

        dbLevelNew.on('error', (e) => {
          console.log('GOT AN ERRROR!!!!!', e);
        });

@CMCDragonkai
Copy link
Member Author

Upstream should indicate that error is a legitimate event for the leveldb instances.

@CMCDragonkai
Copy link
Member Author

The fix for the open problem is that the DB levels must be recreated upon start.

This means INodeManager needs to be CDSS. Not CD pattern.

This is good opportunity to just integrate async-init into this as well.

@CMCDragonkai
Copy link
Member Author

The js-db has received a fix for being able to handle and report the error as part of promise rejection: MatrixAI/js-db#9

For this PR, I need to rework the INodeManager and EncryptedFS and then integrate async-init as well.

Will need to update to 1.1.5 of @matrixai/db.

@CMCDragonkai
Copy link
Member Author

Based on the idea of recreating the sublevels, my thinking is that just doing:

      // INodeManager needs to be recreated
      // It contains sublevels that needs to be re-established
      this.iNodeMgr = await INodeManager.createINodeManager({
        db: this.db,
        logger: this.logger.getChild(INodeManager.name),
        fresh,
      });

In the EncryptedFS.start should be enough to fix the problem.

However it doesn't seem to work. I'm still getting the Inner database is not open error.

@CMCDragonkai
Copy link
Member Author

If I recreate the sublevels right before I call await this.db.level, it ends up working again. But this should already work because the db domains were already recreated after restarting.

Furthermore I noticed that fileGetBlocks is called twice.


Ok I just realised why recreating INodeManager is not enough.

The problem is that this instance is also injected into FileDescriptorManager, and that is still using the old instance.

It appears that therefore, we need to represent INodeManager as a singleton. Therefore it should be StartStop pattern, and then we can just propagate the start stop instead.

@CMCDragonkai
Copy link
Member Author

The INodeManager is now CDSS.

One major change is that during creation/start, it doesn't gc dangling inodes. This was too difficult to do when CDSS and ready decorators are applied, because it could not call those destroy methods if the instance isn't yet started. However, instead the stop method can explicitly destroy dangling inodes.

However 2 other tests failed:

    ✕ cwd still works if the current directory is deleted (74 ms)
    ✕ deleted current directory can still use . and .. for traversal (135 ms)

@CMCDragonkai
Copy link
Member Author

Appears that the source the error comes from await efs.stop();.

It thinks that it's no longer running... need to investigate why.

@CMCDragonkai
Copy link
Member Author

Ok it turns out the error is the same problem with trying to do GC operations in start as well.

Basically the current async-init will say that it's not ready if we are in the middle of stopping because it checking the lock now.

Therefore we do have to do some code duplication and create a separate gcAll() that does the garbage collection fully. And if we do this, we can then enable it for start too for INodeManager.

@CMCDragonkai
Copy link
Member Author

Tests are passing now. Now finalising the linting.

Both the EncryptedFS and INodeManager is now following the CDSS pattern
@CMCDragonkai
Copy link
Member Author

Ok it's all done. Time to merge.

@CMCDragonkai CMCDragonkai merged commit 7c39549 into master Jan 13, 2022
@CMCDragonkai CMCDragonkai deleted the fresh branch January 13, 2022 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant