Skip to content

Update project structure to align with node v18 and new async-locks #80

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

Merged
merged 3 commits into from
Jun 26, 2023

Conversation

CMCDragonkai
Copy link
Member

@CMCDragonkai CMCDragonkai commented Jun 26, 2023

Description

This is following up with the updates with respect to node v18 and macos updates and npm root over npm bin.

Issues Fixed

Tasks

  • 1. Fix test failures due to the new js-db and js-async-locks
  • 2. Rebuild and test with macos on CI/CD
  • 3. Release new patch version

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

We have a lot of test failures mostly related to concurrency, probably due to the new async-locks. Need to see if it due to similar problems as js-db.

@ghost
Copy link

ghost commented Jun 26, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@CMCDragonkai
Copy link
Member Author

Ok one of the big things is because we no longer support ToString in async-locks, this propagated to js-db which then propagates to js-encryptedfs.

This is actually a breaking change unfortunately. But we released it as 5.2.0. I think I just need to update all the usages of inode index and convert them to strings using toString().

@CMCDragonkai CMCDragonkai self-assigned this Jun 26, 2023
@CMCDragonkai
Copy link
Member Author

Converting all the inode indexes to strings solved a bunch of concurrency issues.

However there's another problem now... it seems to do with some encoding. Could it be the node v18 issue?

@CMCDragonkai
Copy link
Member Author

There seems to be some problem with the database persistence. I wrote a file called testfile, and I can read it again.

However if I stop EFS then start it again, I should be able to read the same thing again.

However I seem to get ErrorDBParseValue: Unexpected token e in JSON at position 0 which indicates it couldn't deserialise from JSON.

This is pretty strange, since the new DB shouldn't have this problem... but we have to see.

@CMCDragonkai
Copy link
Member Author

This error is definitely due to the upgrade in @matrixai/db. Changing to node v18 did not cause any problems, neither did any other of the dependencies.

@CMCDragonkai
Copy link
Member Author

Oh this error was due to 5.1.0 not 5.2.0. MatrixAI/js-db@a7053cb

We must repass back the same crypto object in when restarting the db.

@CMCDragonkai
Copy link
Member Author

This is problem for EFS because when we do createEncryptedFS, we don't actually end up with the same crypto object.

When we stop the EFS, we stop the DB, the DB must then must be started again with the same crypto object.

We will need to keep track of the crypto object between restarts of the EFS.

@CMCDragonkai
Copy link
Member Author

Another thing, is that the DB gets started/stopped even if the DB object was passed into the EFS from the outside, in that sense the EFS takes over the lifecycle of a passed in DB.

This is not quite correct. We need to only manage the lifecycle of the DB if the DB was created by EFS.

If the EFS did not create the DB, the DB's lifecycle should be independent.

@tegefaulkes

@CMCDragonkai
Copy link
Member Author

Ok changing that management of the DB instance now means that in PK's side, when giving a DB to EFS, we must then consider that the DB's own lifecycle must be managed outside of the EFS. This is a breaking change @tegefaulkes.

One thing I note is the complication of having to start the DB just to destroy things. This actually doesn't occur here in EFS because if it was our own encapsulated instance, the DB can just be destroyed after stopping and would clear all relevant state. Otherwise we just clear the inode manager state.

However in PK, we end up restarting the DB only to delete things. I wonder if some of that can be simplified if you can just destroy the DB and have expect all the state to be cleared. The only thing would be domains that leave state outside the DB... and may require the DB to do something in order to destroy. Unless we encapsulate all state within our local Polykey node path and just destroy that and it should be fine... but that does break the composition abstraction.

It just appears that the idea that stopping then destroying shouldn't actually stop the DB since destroying domains actually depend on the DB instance running. Still not sure how to elegantly express this.

@CMCDragonkai
Copy link
Member Author

Ok that fixed everything but now broke some of the chroot tests.

    ✕ should be able to access inodes inside chroot (145 ms)
    ✕ should not be able to access inodes outside chroot (176 ms)
    ✕ should not be able to access inodes outside chroot using symlink (174 ms)
    ✕ prevents users from changing current directory above the chroot (150 ms)
    ✓ can sustain a current directory inside a chroot (162 ms)
    ✕ can chroot, and then chroot again (125 ms)
    ✕ chroot returns a running efs instance (97 ms)
    ✕ chroot start & stop does not affect other efs instances (162 ms)
    ✕ root efs instance stops all chrooted instances (166 ms)
    ✓ destroying chroot is a noop (129 ms)

I think that may have relied on the DB lifecycle.

@CMCDragonkai
Copy link
Member Author

The solution involved a weird trick from https://stackoverflow.com/a/64638986/582917.

The constructor parameter type didn't work. But the type assertion did:

      const efsChrooted = new (this.constructor as new (...params: ConstructorParameters<typeof EncryptedFS>) => this)({
        db: this.db,
        iNodeMgr: this.iNodeMgr,
        fdMgr: this.fdMgr,
        rootIno: target,
        blockSize: this.blockSize,
        umask: this.umask,
        logger: this.logger,
        chroots: this.chroots,
        chrootParent: this,
      });

This is partly due to the experimental decorator situation in swc. Remember we had to use new this in our static creator, well our chroot method also constructs itself, and instead of using new EncryptedFS it had to refer to its own decorated self being new this.constructor().

In this case it also required a type assertion on the actual constructor function type.

I remember TSC didn't have this problem, but our tests now use SWC so this becomes an issue.

* updated `@matrixai/db` to 5.2.0 and `@matrixai/async-locks` to 4.0.0
* changed how EFS handles non-encapsulated DB lifecycles, it no longer manages injected DB instances
* due to `swc` testing, changed to using `new this.constructor` over `new EncryptedFS` for `chroot`
* changed to node v18
* changed to using `npm root` over `npm bin`
@CMCDragonkai
Copy link
Member Author

Tasks 2. and 3. are going to staging.

@CMCDragonkai CMCDragonkai changed the title WIP: Update project structure to align with node v18 and new async-locks Update project structure to align with node v18 and new async-locks Jun 26, 2023
@CMCDragonkai CMCDragonkai merged commit 4d4baea into staging Jun 26, 2023
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