WIP: Introduce Snapshot Isolation OCC to DBTransaction

Derived from #18
Description
This implements the snapshot isolation DB transaction.
This means DBTransaction will be automatically snapshot isolated, which means most locking will be unnecessary.
Instead when performing a transaction, there's a chance for a ErrorDBTransactionConflict exception which means there was a write conflict with another transaction.
Users can then decide on their discretion to retry the operation if they need to (assuming any non-DB side-effects are idempotent, noops or can be compensated). This should reduce the amount of locking overhead we need to do in Polykey. We may bubble up the conflict exception to the user, so the user can re-run their command, or in some cases, in-code we will automatically perform a retry. The user in this case can be the PK client, or the another PK agent or the PK GUI.
There is still one situation where user/application locks are needed, and that's where there may be a write-skew. See snapshot isolation https://en.wikipedia.org/wiki/Snapshot_isolation for more details and also https://www.cockroachlabs.com/blog/what-write-skew-looks-like/.
In the future we may upgrade to SSI (serializable snapshot isolation) which will eliminate this write-skew possibility.
~Additionally this PR will also enable the keyAsBuffer and valueAsBuffer options on the iterators, enabling easier usage of the iterators without having to use dbUtils.deserialize<T>(value) where it can be configured ahead of time.~ - already merged
See this https://www.fluentcpp.com/2019/08/30/how-to-disable-a-warning-in-cpp/ as to how to disable warnings in C++ cross platform.
Also see: https://nodejs.github.io/node-addon-examples/special-topics/context-awareness/
Issues Fixed

Fixes #17
Fixes #11 - we won't bother with abstract-level anymore since we are forking classic-level
Related https://github.com/MatrixAI/js-db/pull/22
Related https://github.com/facebook/jest/issues/12814

Tasks

~[x] 1. Added additional overloads to iterator for keyAsBuffer and valueAsBuffer~ - already merged in staging
[x] 2. Added transaction snapshot iterator and tie iterator lifecycle with the DBTransaction lifecycle
[x] 3. Add snapshotLock to ensure mutual exclusion when using the snapshot iterator
[x] 4. Implemented getSnapshot as the last-resort getter for DBTransaction.get
[ ] 5. Change DBTransaction.iterator to use the snapshot iterator
~[x] 6. Upgraded dependencies to match TypeScript-Demo-Lib (excluding node v16 upgrade)~ - already merged in staging
~[x] 7. Add tests for keyAsBuffer and valueAsBuffer usage on the iterator, expect string and V types. Existing tests should still work. Since by default iterator returns buffers. This is different from get which by default does not return raw buffers.~ - already merged into staging
~[ ] 8. Maintain the last updated timestamp for each key being updated in transactions and also in the main DB~ - relying on rocksdb SI directly now
[ ] 9. Detect write conflicts and throw ErrorDBTransactionConflict
[ ] 10. Use the sync option when committing DBTransaction by default, allow it to be set off
~[ ] 11. See if db_destroy can replace our removal of the db path~ - no need for this, because we may have more interesting state in the db path
[ ] 12. Catch CORRUPTION error code and attempt automatic repair and reopen, or at least provide users the ability to call the command with a special exception for this - https://github.com/MatrixAI/js-db/pull/19#issuecomment-1143114335
[x] 13. Added the native code for leveldb and corresponding deps
[x] 14. Added TS interfaces for the native module LevelDB
[x] 15. Promisified native module into LevelDBP
[x] 16. Port over native methods into DB.ts directly
[ ] 17. Update the CI/CD to auto-build the prebuilds, and to do auto-release as part of prerelease and release
[x] 18. Create DBIterator to maintain async lifecycle of the iterator - https://github.com/MatrixAI/js-db/pull/19#issuecomment-1145852220 (it was a good thing we caught this during benchmarking, a serious performance regression that led us to discover a resource leak)
[ ] 19. Use ErrorDBLiveReference to mean that an iterator or transaction object is still alive when db.stop() is called, and therefore it must prevent any stopping, we must there maintain a weakset for every transaction/iterator objects that we create. This can also be used for subdatabases, where you create a prefixed DB in the future.

the WeakSet does not support size or length, there's no way to know how many of these objects are still alive
instead subobjects will need to maintain reference back to the DB and subtract an allocation counter instead, or just maintain reference to objects and remove them
instead we end up using Set then, and destroy removes them from the set

[ ] 20. Create the transaction_* native functions:

[x] transactionInit
[x] transactionCommit
[x] transactionRollback
[x] transactionGet
[x] transationGetForUpdate
[x] transactionPut
[x] transactionDel
[ ] transactionMultiGet
[ ] transactionClear
[x] transactionIteratorInit
[x] transactionSnapshot

[x] 21. Prove that native binding to leveldb works
[x] 22. Switched over to rocksdb native bindings entirely
[x] 23. Modularized rocksdb C++ code for separate compilation, and make it easier to maintian
[x] 24. Investigate the releasing of the transaction snapshot if it is used
[x] 25. Solve write skew problems with GetForUpdate, this "materializes the conflict" in a write skew so that it becomes a write write conflict. Although rocksdb calls this a read write conflcit. SSI is not provided by rocksdb, it is however available in cockroachdb and badgerdb, but hopefully someone backports that to rocksdb.
[ ] 26. Provide JS-level locks because rocksdb optimistic transaction does not work with pessimistic locks atm, no lock manager required for now, since if the C++ code eventually provides it, then we would just switch to what C++ provides - https://groups.google.com/g/rocksdb/c/5v64dTxYKEw

Final checklist

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

Bringing in the new changes from TypeScript-Demo-Lib-Native. But without the application builds because this is a pure library. And also removing deployment jobs.

Ok it's time to finally bring in the leveldb source code and start hacking C++.

I'm also going to solve the problem with key path here and probably prepare it for merging by cherry picking into staging, this can go along with a number of other CI/CD changes too.

@emmacasolin I'm changing the DBIterator type to be like this and introducing DBIteratorOptions:
/**
 * Iterator options
 * The `keyAsBuffer` property controls
 * whether DBIterator returns KeyPath as buffers or as strings
 * It should be considered to default to true
 * The `valueAsBuffer` property controls value type
 * It should be considered to default to true
 */
type DBIteratorOptions = {
  gt?: KeyPath | Buffer | string;
  gte?: KeyPath | Buffer | string;
  lt?: KeyPath | Buffer | string;
  lte?: KeyPath | Buffer | string;
  limit?: number;
  keys?: boolean;
  values?: boolean;
  keyAsBuffer?: boolean;
  valueAsBuffer?: boolean
};

/**
 * Iterator
 */
type DBIterator<V> = {
  seek: (keyPath: KeyPath | Buffer | string) => void;
  next: () => Promise<[KeyPath, V] | undefined>;
  end: () => Promise<void>;
  [Symbol.asyncIterator]: () => AsyncGenerator<[KeyPath, V]>;
};

This means now KeyPath becomes pre-eminent, and anywhere I have KeyPath | Buffer | string, the Buffer or string is intepreted as a singleton KeyPath.

This also allows the key returned by the iterator to be later used by seek or the range options.
This will impact downstream EFS and PK usage though. But find and replace should be sufficient.

If the problem is double encoding then couldn't this be solved by having clear barriers to where and when the encoding is applied? We need the encoded form internally but the the user needs the un-encoded form. Doesn't this mean encoding conversion only needs to happen when we pass to and from the user?

Something I recently discovered the effect of the empty key.
That is if you have a KeyPath of [] this becomes [''].
Therefore if you use the empty key, in master I believe you would not be able to see this key when iterating.
To solve this, I had to change the default iterator option to instead of using gt to use gte.
For example:
    if (options_.gt == null && options_.gte == null) {
      options_.gte = utils.levelPathToKey(levelPath);
    }

This now ensures that the empty key shows up within the level during iteration.

Extra test cases were added for empty keys now, and this actually resolves another bug involving empty keys.
describe('utils', () => {
  const keyPaths: Array<KeyPath> = [
    // Normal keys
    ['foo'],
    ['foo', 'bar'],
    // Empty keys are possible
    [''],
    ['', ''],
    ['foo', ''],
    ['foo', '', ''],
    ['', 'foo', ''],
    ['', '', ''],
    ['', '', 'foo'],
    // Separator can be used in key part
    ['foo', 'bar', Buffer.concat([utils.sep, Buffer.from('key'), utils.sep])],
    [utils.sep],
    [Buffer.concat([utils.sep, Buffer.from('foobar')])],
    [Buffer.concat([Buffer.from('foobar'), utils.sep])],
    [Buffer.concat([utils.sep, Buffer.from('foobar'), utils.sep])],
    [Buffer.concat([utils.sep, Buffer.from('foobar'), utils.sep, Buffer.from('foobar')])],
    [Buffer.concat([Buffer.from('foobar'), utils.sep, Buffer.from('foobar'), utils.sep]),
    ],
    // Escape can be used in key part
    [utils.esc],
    [Buffer.concat([utils.esc, Buffer.from('foobar')])],
    [Buffer.concat([Buffer.from('foobar'), utils.esc])],
    [Buffer.concat([utils.esc, Buffer.from('foobar'), utils.esc])],
    [Buffer.concat([utils.esc, Buffer.from('foobar'), utils.esc, Buffer.from('foobar')])],
    [Buffer.concat([Buffer.from('foobar'), utils.esc, Buffer.from('foobar'), utils.esc])],
    // Separator can be used in level parts
    [Buffer.concat([utils.sep, Buffer.from('foobar')]), 'key'],
    [Buffer.concat([Buffer.from('foobar'), utils.sep]), 'key'],
    [Buffer.concat([utils.sep, Buffer.from('foobar'), utils.sep]), 'key'],
    [Buffer.concat([utils.sep, Buffer.from('foobar'), utils.sep, Buffer.from('foobar')]), 'key'],
    [Buffer.concat([Buffer.from('foobar'), utils.sep, Buffer.from('foobar'), utils.sep]), 'key'],
    // Escape can be used in level parts
    [Buffer.concat([utils.sep, utils.esc, utils.sep]), 'key'],
    [Buffer.concat([utils.esc, utils.esc, utils.esc]), 'key'],
  ];
  test.each(keyPaths.map(kP => [kP]))(
    'parse key paths %s',
    (keyPath: KeyPath) => {
      const key = utils.keyPathToKey(keyPath);
      const keyPath_ = utils.parseKey(key);
      expect(keyPath.map((b) => b.toString())).toStrictEqual(
        keyPath_.map((b) => b.toString()),
      );
    }
  );
});

Needs rebase on top of staging now.

Time to rebase.

I was looking at 2 codebases to understand how to integrate leveldb.

https://github.com/Level/classic-level
https://github.com/Level/rocksdb

It appears the classic-level is still using leveldb 1.20 which is 5 years old. The rocksdb is bit more recent.
The leveldb codebase has a bit more of a complicated build process.
The top level binding.gyp includes a lower level gyp file:
    "dependencies": [
      "<(module_root_dir)/deps/leveldb/leveldb.gyp:leveldb"
    ],

The deps/leveldb/leveldb.gyp file contains all the settings to actually compile the leveldb as a shared object.
Note that the binding.cc does import leveldb headers like:
#include <leveldb/db.h>

These headers are not specified by the binding.gyp, it's possible that by specifying the dependencies, the inclusion headers made available to the top-level target.
The leveldb.gyp also specifies a dependency in snappy:
    "dependencies": [
      "../snappy/snappy.gyp:snappy"
    ],

These are all organised under deps. These are not git submodules. Except for the snappy.
I think for us, we should just copy the structure of deps, as well as the submodule configuration. We can preserve the leveldb.gyp and snappy.gyp, and then just write our own binding.gyp that uses it. Then things should proceed as normal.

The presence of the snappy submodule means cloning has to be done now with git clone --recursive. If you have already cloned, setup the git submodule with git submodule update --init --recursive. It should bring in data into deps/snappy/snappy.

While porting over the binding.gyp from classic-level, we have to be aware of: https://github.com/MatrixAI/TypeScript-Demo-Lib/pull/38#issuecomment-1124494772
The cflags and cflags_cc both apply to g++ and g++ is used when the file is cpp.
However both c and cpp files may be used at the same time, so we should be setting relevant flags for both cflags and cflags_cc.
I'm not sure if this is true for non-linux platforms. The upstream classic-level does not bother with cflags_cc. However we will set it just so that we can get the proper standard checks.

I found out what cflags+ means. It is based on:

If the key ends with a plus sign (+), the policy is for the source list contents to be prepended to the destination list. Mnemonic: + for addition or concatenation.

So cflags+ will prepend rather than appending as normal. This sets the visibility=hidden to be true.
This is required due to: https://github.com/nodejs/node-addon-api/blob/main/doc/setup.md. The reason this is required is documented here: https://github.com/nodejs/node-addon-api/pull/460#issuecomment-544143695. This should go into TS-Demo-Lib-Native too.

I am using this .vscode/c_cpp_properties.json as configuration, so that vscode finds the right headers, there's no automatic way to generate this atm, it's based on paths that is inside the nix-shell:
{
    "configurations": [
        {
            "name": "Linux",
            "includePath": [
                "${workspaceFolder}/**",
                "/nix/store/zl4bvsqfxyx5vn9bbhnrmbmpfvzqj4gd-nodejs-16.14.2/include/**"
            ],
            "defines": [],
            "compilerPath": "/nix/store/58pwclg9yr437h0pfgrnbd0jis8fqasd-gcc-wrapper-11.2.0/bin/gcc",
            "cStandard": "c99",
            "cppStandard": "c++17",
            "intelliSenseMode": "${default}"
        }
    ],
    "version": 4
}

Paths are from:
which gcc
which node
echo $npm_config_nodedir

Noticed that the sync option for PutOptions and DelOptions should be switched to true. This ensures that when the transaction is committed, it is infact committed to disk. Which means if the OS crashes, we won't be losing data.
Probably makes sense that the DBTransaction should be using the internal write batch. I'm not sure if they are journaling the write batch, but it probably isn't, and it's in-memory. The only issue is that you cannot dump what's in the batch, which limits its debuggability.

It is possible or the DB to be corrupted in the case of power failure. In that case the repair_db should be called. There's no guarantee that the DB will return to normal state, but we should have this available in our pk agent start command and auto-repair in case we cannot open the DB. This is also why it is important to always backup PK agent state to other PK agents.

I believe all the types for src/leveldb is done.
I've also discovered a nice way of doing phantom types. It relies on the same mechanism as Opaque type constructor.
Note that I've updated the Opaque type to use a unique symbol as the differentiating key.
This only exists at compile-time and is elided at runtime.
Think of it as the "tag" in Tagged Unions used to implement algebraic data types.
So for example:
/**
 * Iterator object
 * A `napi_external` type
 */
type Iterator<
  K extends string | Buffer = string | Buffer,
  V extends string | Buffer = string | Buffer,
> = Opaque<'Iterator', object> & {
  readonly [brandIteratorK]: K;
  readonly [brandIteratorV]: V;
};
declare const brandIteratorK: unique symbol;
declare const brandIteratorV: unique symbol;

Now the K and V types are phantom types.
Phantom types don't work normally in TS due to structural typing system, but by telling TS that there would be a special key differentiating it, TS stops complaining about the fact that we aren't using the type parameter. But in reality we never use the type parameter and just use casting to create the specific Iterator type.

Should update the Opaque usage in PK to be:
/**
 * Opaque types are wrappers of existing types
 * that require smart constructors
 */
type Opaque<K, T> = T & { readonly [brand]: K };
declare const brand: unique symbol;

@emmacasolin

With this, we should be able to proceed with integrating src/leveldb and replace our usage of leveldown completely.
However the purpose of this is to also get snapshot isolation transactions. I'll do this after first prototyping substitution of leveldown for our internal src/leveldb.

LevelDB corruption errors are represented in JS as an exception with LEVEL_CORRUPTION code. It was introduced here: https://github.com/Level/classic-level/commit/39cad430930e1196a66197de04448392f7e40ef1.
This should mean we can detect if this is the exception and attempt an automatic repair.

It uses https://nodejs.org/api/n-api.html#napi_create_error, which should create an Error object with a code property of "LEVEL_CORRUPTION.
So we should be able to do:
if (e.code === 'LEVEL_CORRUPTION') {
  // attempt repair before opening
}

Not sure how this would be tested though.

The src/leveldb now exports both:

leveldb - default export of the module's methods
leveldbP - wrapped promisified version of leveldb

All leveldb types are also prefixed with LevelDB making it easier to differentiate them from our DB's types.

I've augmented the promisify function now with additional types that preserve typing info. It however still won't work well on overloaded function signatures.
/**
 * Convert callback-style to promise-style
 * If this is applied to overloaded function
 * it will only choose one of the function signatures to use
 */
function promisify<
  T extends Array<unknown>,
  P extends Array<unknown>,
  R extends (T extends [] ? void : (T extends [unknown] ? T[0] : T)),
>(
  f: (...args: [ ...params: P, callback: Callback<T> ]) => unknown
): (...params: P) => Promise<R> {
  // Uses a regular function so that `this` can be bound
  return function (...params: P): Promise<R> {
    return new Promise((resolve, reject) => {
      const callback = (error, ...values) => {
        if (error != null) {
          return reject(error);
        }
        if (values.length === 0) {
          (resolve as () => void)();
        } else if (values.length === 1) {
          resolve(values[0] as R);
        } else {
          resolve(values as R);
        }
        return;
      };
      params.push(callback);
      f.apply(this, params);
    });
  };
}

This can be ported to EFS and PK @tegefaulkes @emmacasolin but low priority.

The DB.ts is only using these functions:
  NAPI_EXPORT_FUNCTION(db_init);
  NAPI_EXPORT_FUNCTION(db_open);
  NAPI_EXPORT_FUNCTION(db_close);
  NAPI_EXPORT_FUNCTION(db_put);
  NAPI_EXPORT_FUNCTION(db_get);
  NAPI_EXPORT_FUNCTION(db_del);

  NAPI_EXPORT_FUNCTION(iterator_init);
  NAPI_EXPORT_FUNCTION(iterator_seek);
  NAPI_EXPORT_FUNCTION(iterator_close);
  NAPI_EXPORT_FUNCTION(iterator_nextv);

  NAPI_EXPORT_FUNCTION(batch_do);

I'm not currently using:
  NAPI_EXPORT_FUNCTION(db_clear);

Because in C++ it appears to be doing the same thing that I'm already doing by relying on the iterator.

I've ported all of those to DB.ts now.

Note that having ios/android builds of the native addon would only relevant if we intend to stay in the node ecosystem when on mobile. If node itself won't be embeddable (as a library or separate runtime) on mobile, then there's no need for ios/android builds of this addon.

Hit this problem again: https://github.com/facebook/jest/issues/12814.
So will need to use toEqual when comparing buffers coming out of the leveldb, not toStrictEqual.
I'm not sure but leveldown and classic level doesn't seem to have this problem, but I'm pretty sure they didn't wrap their buffers. So I wonder how I'm able to test like this.

The DB.test.ts is working now directly against the native addon.
However I discovered that boolean options that have a default value of false in the C++ code, if passed as undefined would instead actually be asserted to being true.
It appears that the BooleanProperty would:

If the property doesn't exist, use the default value.
If the property does exist, but is undefined ends up being true

This cause some confusion on the reverse option. This is because I've starting assigning the options even if they are not set, which would set it as undefined.
This would affect any option that is meant to default to false, and we would want to ensure that if passed as undefined, the option should be cast explicitly to true with !!(options?.[OPTION]).

If you're logging stuff out from C++, make sure to use STDERR otherwise it will clobber console.log outputs:
    fprintf(stderr, "BOOLEAN RESULT: %s %d\n", key, result);
    std::cerr << std::boolalpha << result << "\n";

Note that napi_get_value_bool will end up casting undefined values into a number. It's important to ensure that options are strictly defined with optionality. This means x?: boolean cannot be x: undefined when passed to the native addon.
I've created a filterUndefined utility function to remove any undefined properties to ensure that these properties are removed prior to passing into the native addon.

Before attempting the other tasks, let's see what happens if we use rocksdb with src/rocksdb as we may be able to get free SI if we just integrate at the C++ level.

I found a fork of leveldb's rocksdb that provides a guide on how to get SI and other advanced functionality out of rocksdb: https://github.com/lu4/rocksdb-ts. Would be worth investigating so potentially we can just use that for free.

Added a new test iterating sublevels with range it demonstrates how to use the range options which can be quite tricky.

I tried benchmarking the current code.
It looks like this:
[nix-shell:~/Projects/js-db]$ npm run bench

><EMAIL_ADDRESS>bench
> rimraf ./benches/results && ts-node -r tsconfig-paths/register ./benches

Running "DB1KiB" suite...
Progress: 100%

  get 1 KiB of data:
    43 276 ops/s, ±3.66%   | fastest

  put 1 KiB of data:
    28 117 ops/s, ±2.99%   | 35.03% slower

  put zero data:
    35 105 ops/s, ±2.27%   | 18.88% slower

  put zero data then del:
    18 127 ops/s, ±2.42%   | slowest, 58.11% slower

Finished 4 cases!
  Fastest: get 1 KiB of data
  Slowest: put zero data then del

Saved to: benches/results/DB1KiB.json

Saved to: benches/results/DB1KiB.chart.html
Running "DB1MiB" suite...
Progress: 100%

  get 1 MiB of data:
    1 932 ops/s, ±4.57%    | 93.21% slower

  put 1 MiB of data:
    19 ops/s, ±135.25%       | slowest, 99.93% slower

  put zero data:
    28 456 ops/s, ±4.25%   | fastest

  put zero data then del:
    13 221 ops/s, ±6.31%   | 53.54% slower

Finished 4 cases!
  Fastest: put zero data
  Slowest: put 1 MiB of data

Saved to: benches/results/DB1MiB.json

Saved to: benches/results/DB1MiB.chart.html

Everything looks similar to before EXCEPT the put 1 MiB of data.
That is now around 20 ops/s.
Whereas previously on staging branch that's currently around 467 ops/s.
How did it drop so much. Firstly I think it's because the sync option is true (which I set to false by default). But I've made it false, but it doesn't change anything.
This is quite strange since all other ops are basically the same.
Need to find out why my put is so slow.

I tried with classic level, it's not that. So now I need to think if it's now due to our promise implementation or something else. Very strange. Maybe there's optimisation flags that aren't turned on.

It's also not the src/leveldb code. Benching that gives us 471 ops/s. So it's something that's happening inside DB.ts.

It turns out its the iterator.
If the iterator is left open, it appears to cause performance issues.
We have a bug where the iterator is not being closed.
I need to refactor the next call inside the iterator so that it is actually being closed.

The await iterator.close() must not be called twice. If you call it twice, it's an error.

Ok so there's some missing logic in the iterator implementation atm:

We need to block calls to seek, next, and close. That is these calls cannot be run concurrently.
The ready decorator currently has a block parameter that enables blocking these calls during the async lifecycle. So we should use a lock.
Upstream classic-level and variants just throw exceptions if they are called at the same time, we can use our async-locks to ensure the methods are called.
The close should based on our CreateDestroy system.
The existence of the cache must be considered as well...

So I'm thinking we create a class called DBIterator to take care of this so it's clearer and fits within our existing async class design.
The DBTransaction is a bit more complicated since it has to maintain 2 iterators, however this might change once we bring in rocksdb since it might already have its own transaction state that we can get for free.

It is essential to run git submodule update --init --recursive now if we using submodules. Also use git submodule status to see what the status of the submodules are.
Currently:
[nix-shell:~/Projects/js-db]$ git submodule status
 09c7e96eac1ab983f97ce9e0406730b8014b3398 deps/rocksdb/rocksdb (v5.8-3265-g09c7e96ea)
 b02bfa754ebf27921d8da3bd2517eab445b84ff9 deps/snappy/snappy (1.1.7)

We now have DBIterator, this has taken over the functionality that was specified inside DB.ts before.
The benchmarks are back to how they were before, and we no longer have the iterator leak. We've made some changes to how the iterator next and seek calls work. Concurrent calls to next will block rather than throwing an error, but if a seek call is made while a next is in progress, that will result in ErrorDBIteratorBusy.

Although DBIterator is an CreateDestroy, it doesn't actually have an asynchronous creation at all, and it is just created from the constructor. This is because the iterator() call is supposed to just return the iterator object.

In trying out rocksdb, it appears there's differences at the C++ level.
In the new classic-level we have iterator_close and iterator_nextv, while in the current rocksdb, it's still using iterator_end and iterator_next.
Basically the same as leveldown, whereas classic-level upgraded their API. We should be able to change up the binding.cc and recompile.
Also note that rocksdb is 7.9 MiB, while leveldb was only half a megabyte. It's significantly longer to compile, which means I'm trying out multi-core builds for node-gyp build using npm_config_jobs=max.

The npm_config_jobs=max should be backported to TS-Demo-Lib-Native.

I had a thought that if we were developing solely on Nix, we could just use https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/libraries/rocksdb/default.nix and expect our binding.gyp to compile and at the end just link with whatever shared objects the rocksdb library has. It would be faster and less maintenance to keep rocksdb source code internally.
However that would preclude any ability to compile and build for windows and possibly other platforms. MacOS could still work with nix of course.
We don't have any nix integration with macos in the CI/CD yet, I did explore it but I wasn't sure if it was going to be more performance given that the macos runners don't have it.

I've tested with rocksdb, the only issue right now is that rocksdb is still using the old abstract-leveldown compatible, whereas I'm currently using the abstract-level API.
The classic-level already upgraded to using abstract-level, but rocksdb hasn't.
It's only a few minor changes stemming primarily from this commit: https://github.com/Level/classic-level/commit/b9ba3dd6c5e5b36387d1efb040ad69edb6045dc9
2 methods are missing from rocksdb/index.cpp: https://github.com/MatrixAI/js-db/pull/19#issuecomment-1146639422
So will need to first upgrade the rocksdb/index.cpp in a similar vein to how classic-level did it, and maintain API parity with leveldb/index.cpp.

This https://github.com/Level/classic-level/compare/ba729d223ec5ec54c5ad88e05c11e7bba71315f2...main shows us the diff from the point of forking from leveldown to classic-level main branch tip. We can use this to figure out what needs to be changed for rocksdb.

I've synced up the index.cpp API with the changes in classic-level, however, I'm hitting a segmentation fault during db_open call. Will need to debug this further.

Just a silly mistake it's working now.

All DB.test.ts and DBIterator.test.ts works with rocksdb.

Benchmarks for Matrix ML 1:
[nix-shell:~/Projects/js-db]$ npm run bench

><EMAIL_ADDRESS>bench
> rimraf ./benches/results && ts-node -r tsconfig-paths/register ./benches

Running "DB1KiB" suite...
Progress: 100%

  get 1 KiB of data:
    40 632 ops/s, ±3.33%   | fastest

  put 1 KiB of data:
    25 323 ops/s, ±1.62%   | 37.68% slower

  put zero data:
    28 292 ops/s, ±2.01%   | 30.37% slower

  put zero data then del:
    15 497 ops/s, ±2.21%   | slowest, 61.86% slower

Finished 4 cases!
  Fastest: get 1 KiB of data
  Slowest: put zero data then del

Saved to: benches/results/DB1KiB.json

Saved to: benches/results/DB1KiB.chart.html
Running "DB1MiB" suite...
Progress: 100%

  get 1 MiB of data:
    544 ops/s, ±1.92%      | 98.01% slower

  put 1 MiB of data:
    533 ops/s, ±1.22%      | slowest, 98.05% slower

  put zero data:
    27 299 ops/s, ±1.70%   | fastest

  put zero data then del:
    13 880 ops/s, ±1.65%   | 49.16% slower

Finished 4 cases!
  Fastest: put zero data
  Slowest: put 1 MiB of data

Saved to: benches/results/DB1MiB.json

Saved to: benches/results/DB1MiB.chart.html

The rocksdb's OptimisticTransactionDB is a class that extends StackableDB which extends DB. The current index.cpp is using just DB. Which means once we use OptimisticTransactionDB, this affects the entire js-db.
Furthermore rocksdb does not throw exceptions, they rely on the returned Status. This must be checked and then thrown as JS exception like we are doing right now.
It is possible to use the debugger on the native code when running with nodejs. You have to build a debug version of the native addon with node-gyp build --debug or node-gyp rebuild --debug.
The node-gyp-build loader actually prefers to use the release version rather than the debug version, so if both builds are available, the release version will be loaded in preference.
https://github.com/prebuild/node-gyp-build/blob/2e982977240368f8baed3975a0f3b048999af40e/index.js#L33-L39
The debug builds is quite heavy, it has to end up building all the dependencies with debug. And so the debug build is 130 MiB.
I reckon one should avoid deleting the release build when attempting to debug, and just move it somewhere else, and move it back in when you want to use the release build.
Once the debug build is available, one can use gdb on node. So it ends up doing the same thing with launch.json.
However with nodejs it's a bit smarter, since the debugger can auto-attach to any nodejs process, this means it's easy to debug nodejs code, because we are writing typescript, and upon using ts-node, the nodejs runs and the debugger is attached, and breakpoints... etc are used.
However the C/C++ seems to require a bit more configuration: https://code.visualstudio.com/docs/cpp/launch-json-reference, especially since we may be using ts-node in one case, jest in another case... The debugger has to attach to the node process.

So given there's no readonly mode, we can remove this readonly option.

This will be useful too https://www.fluentcpp.com/2019/08/30/how-to-disable-a-warning-in-cpp/.

Regarding the mixing of optimistic and pessimistic transactions, it seems this is not easily doable inside rocksdb. I've asked a question about this: https://groups.google.com/g/rocksdb/c/5v64dTxYKEw.
For now, I'll proceed with OptimisticTransactionDB, and if we need to do, serialisable locking could be done on the outside in JS-land, it would of course be subject to potential deadlocks and other possibilities until a lock manager can manage all of those.

The index.cpp maintains its own "classeswithstruct Database, struct Iterator... etc. I've created struct Transactionto act as the internal reference object that in JS-land, they must pass  as a reference intorocksdbP.transaction_*` calls.
Therefore there's multiple API layers:

In JS-land we have rocksdbP and rocksdb module objects - this is what the JS exposes to be called
In C++ we have transaction_open, transaction_commit, transaction_rollback as the top level calls.
The top level static calls interact against the struct Transaction object.
The struct Transaction object maintains an internal rocksdb::Transaction object, and performs calls against that internal object.
It also calls into struct Database because the database object also keeps references to the transactions in order to close them when closing the database.

I find it interesting that the C++ land is mirroring the JS land. Do we really need so many object layers? This is how classic-level was already designed with its struct Iterator and struct Database.
I imagine that in C++ land, one could potentially do away with its own Transaction objects, and instead pass rocksdb::Transaction objects directly as references usable from JS-land, like why bother creating another object wrapper encapsulating rocksdb objects?
Possibly if the native addon itself has to complicated stuff that is not related to the internal rocksdb.

Once we get around to creating iterators within the transaction, do we say that the Database still owns the Iterator, or is this now Database -> Transaction -> Iterator?

Would be good to also use clang-tidy. Integration requires all the relevant include paths. Will need to see: https://github.com/mapbox/node-cpp-skel/pull/64. It requires the clang-tools package.

Discovered a bug in iterator_close and therefore transaction_rollback and transaction_commit. This affected upstream too.  This causes the JS runtime to never continue executing.
The solution is to call the callback with a null indicating no error if we are conditionally calling the internal side effect.
I also checked the db_close if it had the same problem, and it doesn't. This is because it either queues the callback for execution, or assigns it as the database->pendingCloseWorker_ which will be executed at the end of each priority worker.
Note that most things are PriorityWorker except OpenWorker, CloseWorker, DestroyWorker, RepairWorker, CloseIteratorWorker, NextWorker.
I believe I should be making CommitTransactionWorker and RollbackTransactionWorker to be PriorityWorker as well. They seem to be important to be prioritised before the other workers if I'm interpreting this correctly.

It seems the PriorityWorker is basically intended for async work to always take priority over closing the DB.
Then I wonder why things like CloseIteratorWorker and NextWorker are not priority workers.

I've got transaction_init, transaction_commit, and transaction_rollback working.
I think it's a good idea to split up the C++ code, which is currently 2476 lines of code atm. It's getting difficult to constantly have 2/3 windows of the same file.
This should just be a matter of creating a few separate files, which are separately compiled and linked together. There will need to be headers to be included to allow forward declaration.

The napi-macros package doesn't export the function names when using NAPI_EXPORT_FUNCTION, so I've just redefined it here. Probably in the future, can just switch to using node-addon-api instead which is better maintained.
Here's what I've done:
/**
 * NAPI_EXPORT_FUNCTION does not export the name of the function
 */
#undef NAPI_EXPORT_FUNCTION
#define NAPI_EXPORT_FUNCTION(name) \
  { \
    napi_value name##_fn; \
    NAPI_STATUS_THROWS_VOID(napi_create_function(env, #name, NAPI_AUTO_LENGTH, name, NULL, &name##_fn)) \
    NAPI_STATUS_THROWS_VOID(napi_set_named_property(env, exports, #name, name##_fn)) \
  }

This means the functions being exported will have the name property, and we can see in the stack trace what function is called.
Furthermore the promisify is extended to preserve the function name when wrapping:
  Object.defineProperty(g, 'name', { value: f.name });

This means promisify should be updated in PK as well @emmacasolin:

/**
 * Convert callback-style to promise-style
 * If this is applied to overloaded function
 * it will only choose one of the function signatures to use
 */
function promisify<
  T extends Array<unknown>,
  P extends Array<unknown>,
  R extends T extends [] ? void : T extends [unknown] ? T[0] : T,
>(
  f: (...args: [...params: P, callback: Callback<T>]) => unknown,
): (...params: P) => Promise<R> {
  // Uses a regular function so that `this` can be bound
  const g = function (...params: P): Promise<R> {
    return new Promise((resolve, reject) => {
      const callback = (error, ...values) => {
        if (error != null) {
          return reject(error);
        }
        if (values.length === 0) {
          (resolve as () => void)();
        } else if (values.length === 1) {
          resolve(values[0] as R);
        } else {
          resolve(values as R);
        }
        return;
      };
      params.push(callback);
      f.apply(this, params);
    });
  };
  Object.defineProperty(g, 'name', { value: f.name });
  return g;
}

In relation to https://github.com/facebook/jest/issues/12814 it turns out that when running in Jest, even exceptions like Error created by napi_create_error are not instances of Error.
This means any tests against the native addons will need to be aware that strict type checks won't work.
This might mean that in the source code where it checks if the e is an instanceof Error, it just can't be done, and instead we must only be checking the properties like code.

Looks like I need some forward declarations here, right now there's an include loop between database.h, transaction.h and iterator.h.
I wonder though do these header need to declare forward declarations?

The warning about the loglevel is a legitimate warning. It is possible that the log level configuration is wrong, and if so, that would result in a JS exception being thrown. however this doesn't prevent the C++ constructor code from continuing.
The ideal case is for the C++ constructor to throw an exception if it fails to construct for whatever reason. The problem here is that the binding.gyp and related files was inherited from rocksdb in level. In there, it seems there is configuration that disables the existence of C++ exceptions, if not linux, at least windows.
So for now since we don't want to experiment with C++ exceptions right now, we can do a couple things:

Use a bare return to cancel computation, this means the object is still constructed, just rest of the code hasn't finished initialising. This creates a zombie object as explained here:  https://stackoverflow.com/a/737656/582917.
Use a named constructor or some static factory function to do the construction, then we can return some sort of status code.
Make it so that if the log level configuration parameter is wrong, it just defaults to doing the default thing, which is if the log level size is not greater than 0, it just uses the null logger.
Take the entire check on whether it is correct out of it and move it into db_open, thus throwing the JS exception there and not actually proceeding to construct the OpenWorker.

I'm going to try out solution 4.

Solution 4. should be working now. We figure out the rocksdb::InfoLogLevel and rocksdb::Logger ahead in db_open first, and then we pass that into the OpenWorker.
Note that the setting of the NullLogger only occurs when the infoLogLevel option is empty. Otherwise, it leaves it as whatever it is defaulted to.

No more major warnings. And the C++ code is no split out.

All tests pass on the split out code.
Also if an invlid log level will be used, then we get an JS exception in the callback with code DB_OPEN and the message Invalid log level.

Now to add in the rest of the transaction functions.

I have the basic transaction methods, it's time to refactor DBTransaction to support those methods, and reactivate some simple tests involving DBTransaction.

The rocksdb's OptimisticTransactionDB is a class that extends StackableDB which extends DB. The current index.cpp is using just DB. Which means once we use OptimisticTransactionDB, this affects the entire js-db.
Furthermore rocksdb does not throw exceptions, they rely on the returned Status. This must be checked and then thrown as JS exception like we are doing right now.
It is possible to use the debugger on the native code when running with nodejs. You have to build a debug version of the native addon with node-gyp build --debug or node-gyp rebuild --debug.
The node-gyp-build loader actually prefers to use the release version rather than the debug version, so if both builds are available, the release version will be loaded in preference.
https://github.com/prebuild/node-gyp-build/blob/2e982977240368f8baed3975a0f3b048999af40e/index.js#L33-L39
The debug builds is quite heavy, it has to end up building all the dependencies with debug. And so the debug build is 130 MiB.
I reckon one should avoid deleting the release build when attempting to debug, and just move it somewhere else, and move it back in when you want to use the release build.
Once the debug build is available, one can use gdb on node. So it ends up doing the same thing with launch.json.
However with nodejs it's a bit smarter, since the debugger can auto-attach to any nodejs process, this means it's easy to debug nodejs code, because we are writing typescript, and upon using ts-node, the nodejs runs and the debugger is attached, and breakpoints... etc are used.
However the C/C++ seems to require a bit more configuration: https://code.visualstudio.com/docs/cpp/launch-json-reference, especially since we may be using ts-node in one case, jest in another case... The debugger has to attach to the node process.
It could also just work with: gdb --args /usr/local/bin/node app.js. Not sure if that triggers vscode's breakpoints.
I believe that we never used launch.json at all, and only relied on the automatic attach system that vscode has. This made it easier with zero configuration. It would be nice if gdb can also be simultaneously auto-launched/attach for a given program, in this case it would also be nodejs. One might even be able to run both debuggers at the same time, and so you can add breakpoints/logpoints in TS and C/C++ code.

I had to just try this.
So the issue was that there was a segfault in the C++ code occurring in the Transaction::Put method call.
I narrowed it down to:
  rocksdb::Status s = dbTransaction_->Put(key, value);

But I couldn't see what the segfault was from.
First thing is to delete the build directory so that the old Release was removed, because node-gyp prefers using the Release instead of Debug build. I believe we should change this up by making the node-gyp-build prefer the debug build first, since this allows us to debug more easily, and then just delete the debug build when we're ready.
Then use node-gyp rebuild --debug we are rebuilding the debug binary.
Now the issue gets a bit complicated, normally we are using ts-node to run things, but ts-node isn't the binary we want to use gdb on. It's the node binary that we want to run it on. So for now I don't know how to use gdp on the ts-node, so I first just use tsc to compile all of the code like rimraf ./dist && tsc -p ./tsconfig.build.json.
Then I can write a simple JS script that I want to check, in this case a prototyping script test-rocksdb.js.
Finally I could use gdb node, and inside the GDB shell, we could use run ./test-rocksdb.js.
In this case I'm not even using a breakpoint at all. But I did have https://github.com/cyrus-and/gdb-dashboard integrated into my system.
What it showed me was that the this property in the stack was 0x0. That's a null pointer.
Subsequently confirming it by checking this against nullptr, also revealed that the this was null.
Note that subsequent runs/builds doesn't always show this, sometimes it ended up being 0x03. At any case, the this is not what it should be. It should be a pointer to the Transaction object.
That's when I checked the transaction_workers.cpp and noticed that I forgot to instantiate the tran_ in the other transaction workers that I created. Because TransactionCommitWorker and TransactionRollbackWorker had it properly setup.
Not instantiating properties means that they get instantiated to null pointer or possibly some undefined behaviour.
The usage of C++ constructor initialiser list is definitely error prone, since it has to be in-order of the declarations of the struct/class, and vscode doesn't tell you anything about the pointer not being initialised, because there are legitimate uses for doing so.

For further intensive dev on C/C++, definitely need to figure out how to integrate gdb into the vscode system while automatically compiling the typescript first.

Important to be aware that the rocksdb slices can be converted to C++ strings with key.toString(). To convert to C strings, further c_str() can be used. So key.toString().c_str(). Important if you want to use fprintf. But in C++, std::cerr is more idiomatic.
  std::cerr << key.ToString().c_str() << std::endl;
  std::cerr << value.ToString() << std::endl;

The clang-tidy does some semantic checks like. This is how you use it:
clang-tidy \
  --checks='*' \
  ./src/rocksdb/napi/worker.cpp \
  -- \
  -isystem /nix/store/zl4bvsqfxyx5vn9bbhnrmbmpfvzqj4gd-nodejs-16.14.2/include/node \
  -isystem /nix/store/hym1n0ygqp9wcm7pxn4sfrql3fg7xa09-python3-3.9.12/include \
  -isystem ./deps/rocksdb/rocksdb/include \
  -isystem ./node_modules/napi-macros

It can make use of .clang-tidy configuration file as well, but I haven't figured that out yet.
Note the usage of include/node. This is because we are currently including node-api.h, but not node/node-api.h. It seems node-gyp understands to go into the node directory too.
There are alot of checks and not all are relevant, so we would need to create our own standard like we do with .eslintrc.

Seems like to be compatible with the rest of the system, it would make sense to use #include<node/node_api.h> instead of just #include<node_api.h>. We can try and see what happens if it compiles well on the other platforms too.

We now have npm run lintnative and npm run lintnativefix. I disabled the include sorting because it's not able to know whether something is an external library atm, the clang-format is configured in .clang-format. The clang-tidy could be further improved like:
clang-tidy \
  --checks='*' \
  ./src/rocksdb/napi/worker.cpp \
  -- \
  $NIX_CFLAGS_COMPILE \
  -isystem ./deps/rocksdb/rocksdb/include \
  -isystem ./node_modules/napi-macros

Note that globbing doesn't work nicely in npm run scripts, so you must use find instead. The main problem is that compilation flags are necessary, and so node-gyp right now has all the configuration and configuring clang-tidy is extra work.
Anyway having clang-format working is great for now.

I've tested the transactionGetForUpdate, and this now works to solve write skew anomalies pretty easily. Basically it promotes reads into a same-value write. This explains my earlier question about the difference between software transactional memory and databases. STM seems to rely on value equality, but databases use logical timestamps instead. So writing the same value just means updating a logical timestamp. Databases have to deal with more sophisticated data, whereas STM can get away with a simpler model.

We can now proceed to bring in iterators for the transactions, and get onto fixing up DBTransaction.

In bringing in first class snapshots, we had to create Snapshot class that encapsulates rocksdb::Snapshot. The reason was the same reason for iterator/transaction, object lifecycle management, if we were to return a reference to a C++ object in a native method, JS expects them to be garbage collected as soon as the scope is finished. Because the object must live beyond the scope of the native method, we have to keep track of it.
This is done by incrementing the reference count by 1, but then also keeping a reference to it in the Database. This way now if the Database closes, it will end up releasing all of these snapshots.
Note that there are several ways the Database closes:

Directly through the dbClose native method, this does it asynchronously. But it also uses the snapshot_release_do.
By allowing the napi_env to expire or become invalid, which is triggered through a cleanup hook. This happens when the node runtime itself is closing. This ends up calling env_cleanup_hook. This does the cleanup functionality synchronously.

Now there's something additional one has to be concerned about when calling dbClose. Because it is asynchronous, the codebase tries to currently schedule these closing operations after any priority operations that is still running.
This is done through the PriorityWorker and a reference count on Database called Database::ref_, and with the count value put into Database::priorityWork_ property.
So if there is priority work to do, the closing operation is registered under Database::pendingCloseWorker_. This ends up being called when the reference count reaches 0.
This explains why CloseWorker is extending BaseWorker, and not PriorityWorker.
Now we have a similar situation for transactions. If we proceed to call dbClose() while there are still asynchronous operations in the transactions, we should be scheduling the transaction rollbacks after any priority work that's being done in the transaction.
This is what's still TBD in transaction_rollback_do, as well as transactionCommit.
  // TODO:
  // if other async ops, delay this operation

To solve this, we should be adding a reference count of priority work in our Transaction. Then the PriorityWorker that comes out of Transaction should be decrementing the count for Transaction, not just the Database.
For iterator this was done differently. Instead a DoFinally was registered for all the workers in the iterator, and so iteratorClose is always scheduled after any asynchronous concurrent NextWorker. This was easy to do because there was only 1 other asynchronous method.
For transaction it would be a bit more tedious to implement.
The reason is that the dbClose currently just immediately deletes the database if there is no priority work. Priority work isn't just whenever there's a PriorityWorker, it's also when there are Transaction, Snapshot or Iterator still live. We could create a second constructor for PriorityWorker that increments & decrements priority work for Transaction instead.

The new Snapshot class will when constructed with Transaction will do:
Snapshot::Snapshot(Transaction* transaction, const uint32_t id)
    : database_(transaction->database_),
      id_(id),
      isReleasing_(false),
      hasReleased_(false),
      ref_(NULL) {
  // This ensures that the transaction has consistent writes
  transaction->SetSnapshot();
  // Use this snapshot to get consistent reads
  dbSnapshot_ = transaction->GetSnapshot();
}

This ends up creating a consistent read and write when snapshots are used.
One just needs to call transctionSnapshotInit.
The DB has its own called dbSnapshotInit.
At the end, the snapshot should be released with snapshotRelease.

Transactions have their own priority count now. However I think I broke something there... need to verify that the ref counts make sense.

Transaction ref counts appear to be working.

I have a test for snapshots now, but I need to figure out what happens if you only call transactionSnapshotInit(). Because it does give you back a snapshot object, you can't just call it without dealing with it, otherwise resource leak.
However also the snapshot itself can't actually be released until after the transaction is committed or rollbacked. I would argue that if it is doing this, the snapshots should also be invalidated. But right now snapshot release has nothing to do with the transaction, but instead done as part of the database shutdown.
So one must first transactionCommit and then snapshotRelease the snapshot if it was used. This is based on the theory that the snapshot is needed to be cleaned up after the transaction where the snapshot was acquired is still live.
Perhaps one can release the snapshot before the transaction commits, but it doesn't affect the commit itself.

It's definitely kind of limiting not being able to use C++ exceptions, it makes it difficult to write procedures that terminate execution thread and have it bubble upwards. If we write functions we then need to check if those functions returned true, but this goes against my ideal writing procedures and functions separately so that it is clear that the former performs side-effects, and the latter doesn't.
So for now I've settled on macros.
Basically the issue is that we should be checking whether the transaction is ready to use. If it has already been committed or rollbacked, it should be usable afterwards, without an appropriate check, we end up with a segfault.
It seems common to reach a segfault whenver something that shouldn't be used is being used. Basically null pointer exception, which would be obvious in a managed language, whereas in C++ we don't see it at all, it's just a segfault.
So for now the C++ code isn't the best well abstracted atm, it would require more conditional checking and some further abstractions to clean up in the future, especially checking for valid states where something can be performed.

Now if I don't release the snapshot, the act of closing the database causes it to attempt to release the snapshot. So I suspect, perhaps if we commit/rollback the transaction we must not attempt to release that snapshot.
Actually it turns out, that if you were to release the snapshot earlier, this causes a bug in the transaction when it later attempts to close.
I think you never manually release the snapshot if the snapshot was created from the transaction. Because then the snapshot is part of the lifecycle of the transaction. Hence why it's called Transaction::GetSnapshot.
All the example code shows just setting the snapshot pointer to nullptr after the transaction is completed.

Anyway to solve the problem, we just have to ensure not to release the snapshot early. Or if we are releasing the snapshot early, we would then have to also probably clear the snapshot from the transaction before committing or rollbacking it.
So we should make snapshotRelease a dbSnapshotRelease instead, since it cannot be applied to snapshots acquired from a transaction.
If it is applied, we should add a function that throws an error as part of the release. The transaction snapshot is only applicable to the read calls. But any snapshot that is no longer relevant should be switched with a boolean, to avoid callers from re-using the snapshot.

You have to set the snapshot before getting it, if you get it before on the transaction, it will give back a null pointer.

We now have RocksDBTransactionSnapshot and RocksDBSnapshot 2 separate types, the former only for transaction snapshots, the latter exposing database snapshots. Don't mix them up!

Snapshot usage for transactions appear to be working. I've also changed dbGetMany to dbMultiGet which uses rocksdb's native MultiGet function.
Now I'm testing the snapshot usage for dbGet and dbGetMany. Once this works, we just need to add iterators to the transactions.

I've got snapshot usage working for all things except for iterators.
For the transactional iterator api, I'm thinking that iteratorInit and transactionIteratorInit are 2 constructors of the same iterator object.
This object depending on whether it is constructed from DB or Transaction has to update their reference count to DB or Transaction respectively. Basically DB currently maintains references to its iterators, transactions and snapshots. Transactions will need to maintain references to its iterators, but not snapshots. This is cause rocksdb transactions auto manage the snapshots, but they dont auto manage the iterators.
Additionally iterators originally created their own snapshots, and even if no snapshot is given, they use implicit snapshots. They should be changed so that they can take snapshot as a parameter. In such a case, the snapshot must be released if they were created from the DB, or must not be manually released if they were created from the transaction.
The iteratorNextv, iteratorSeek, iteratorClose should still work the same way. I think iteratorInit might be generic too so we don't need an additional method for transactionIteratorInit.

I've realised that writing JS to C++ bindings, you have to pick one of the 2 ways to do things:

Try to make C++ work like JS, such that the native calls work with JS idioms
Realise that the native calls are C++ calls, and wrap  those calls in JS idioms, and write C/C++ with C/C++ idioms

In the beginning I thought doing it style 1 was best, however I realise that this results in alot of code in C++ that ultimately will be harder to maintain, and requires advanced C++ expertise.
With style 2. we can assume that if you call the native methods incorrectly, you just get a segfault. Not very nice, but you shouldn't be calling them incorrectly. Then at the JS side, you don't just expose the native calls directly, but you wrap them in JS idiomatic objects and lambdas and exceptions... etc.
Basically the impedance mismatch between JS and C++ is so wide that it's just more efficient to have JS idiomatic behaviour encoded on the JS side, and wrap the sharp thorny bits of C++ with JS, rather than trying to get the C++ side to behave like you would expect a JS function to behave.
For example there's alot of functions on the native side that if you call it incorrectly, you end up with undefined behaviour which basically means a segfault. One example is like closing an object like the DB, Transaction... etc, if you close it twice, it will result in a segfault. So far I've tried to make so that the C++ code is idempotent and throws proper exceptions back, but I realise now that this is unnecessary, and I'm going to drop all these safeguards when calling into native code, and just specify the expected behaviour and make sure the JS side wraps the more "sharper" native behaviour.

Instead of having lots of runtime safeguards in C++, it makes more sense to just have loads of asserts to have preconditions and postconditions met.

The usage of assert for asserting compile time preconditions and postconditions is basically how we write TS these days by using the types as the compile time assertions. We got away from having to write runtime checks all over the place in JS, so we should replace my runtime safeguards with asserts.

Ok I think I got a spec for object lifecycle now. All objects that are going to be referenced from JS will have these (there are exceptions later):

Constructor (possibly multiple variants) that sets things like isClosing_ and hasClosed_ to false.
Destructor always asserts hasClosed_ to be true, inside it further proceeds to delete any "newed" objects in the constructor. It does not need to set nullptr for any pointer members, because the object itself is destroyed.
There is a Attach and Detach functions that set the ref_ property as well as optionally attaching themselves on a container object like Database with respect to Iterator. These 2 functions are idempotent, they will check if the ref_ is set or not set.
A closing function like Close. This is also idempotent and will set the hasClosed_ to true. It will proceed to do any closing/cleanup operations. What it won't do is delete member pointers. That can be left for the destructor to do. If for some reason it needs to delete a member pointer, then it must set that member pointer to a null pointer, which allows subsequent delete operations to pass through as a noop.

Now the order of creation is always:

Construct the object with new Object()
Call object->Attach(env, object_ref)

When you are doing an explicit/manual close like situation 2.:

Call object->Close()
Call object->Detach(env)

If the GC is triggered once the object falls out of scope in JS, the handle will:

Call object->Close()
Call object->Detach(env)
Perform delete object

Because Attach, Detach, Close are all idempotent, this should all work.
In situation 3 and 4 which are the same with respect to these objects, the container object will proceed to perform manual cleanup. Meaning they don't actually perform delete call, that's always reserved for the GC finalizer. So they will do:

Call object->Close()
Call object->Detach(env)

Furthermore situations 2, 3 are done asynchronously with workers, and they use the isClosing_ boolean.
Situation 4 is done synchronously because the node environment is shutting down.

There are 3 exceptions to the spec above.

Database - this is the "root" object of the native addon, the JS reference count starts at 0 here instead of 1, this is because if the database reference is no longer in-scope, then the database should be GCed completely, it should not stick around.
The BaseIterator is not an object managed by JS, only the extended Iterator is, and so the base object doesn't have Attach nor Detach methods
The BaseWorker object actually deletes itself in its DoFinally, this is because it is asynchronous, and there is no reference to in JS land

If there are child objects or pending work, situation 1 does not occur as the reference count will still be above or equal to 1.
This means that only situation 2 can lead to explicit destruction of the child objects. This is what dbClose and transactionRollback or transactionCommit has to do.
This destruction is still asynchronous of course.

Compilation succeeds, I realised now that both Database and Transaction needed forward declarations to ensure that they can compile. Forward declarations are useful to prevent include loops.

Instead of compile time LOG_DEBUG, I'm thinking we should make use of https://nodejs.org/api/cli.html#node_debug_nativemodule and just when the NODE_DEBUG=db, we would print out stderr logs from the native code. There was an existing issue in the js-logger to make use of that DEBUG environment variable too, but that's interesting cause it's more useful for showing debug messages and drilling down to specific debug messages compared to general logging messages. I think "debug logging" is a bit different from drill down. Right now js-logger has no way of being controlled by the outside via environment variables. It's just done all programmatically. I think it could be useful for js-logger to be able to receive instructions about enabling or disabling parts of the logger hierarchy dynamically. But native addons don't have this feature just yet until we incorporate runtime logging too here.

Confirmed that the napi_external finalizers actually run after the env_cleanup_hook. Which is surprising because that would mean that napi_env does still exist while the hook is executed.
Cleaning NAPI Environment
Cleaned NAPI Environment
Garbage Collecting Iterator
Garbage Collected Iterator
Garbage Collecting Database
Garbage Collected Database

However because the hook isn't being executed as part a function call, it does not have access to the napi_env env property. Which is why at that point we perform synchronous cleanup without making use of the base workers.
The finalizers can therefore run before the environment gets cleaned up, or after the environment is already cleaned up. I guess this makes sense since we are removing the cleanup hook inside GCDatabase.

Ok so basically our manual close operations have to also manually delete any of the internal pointers that has been constructed via new.
This is an all or nothing situation. If we leave the deletion of the objects to the destructors, which is symmetric with respect to the constructors, then it's possible that we don't release resources immediately during a call like dbClose. So if we are going to delete some objects during manual closes, then all objects require manual closes. In this case we must set the nullptr to ensure that subsequent delete ops are noops.

All tests as passing again. I have integrated the ability to use NODE_DEBUG_NATIVE to switch on debug logs at run time.
NODE_DEBUG_NATIVE=rocksdb npm test

Will show all the native logs on stderr.
This is just an experiment atm, later in the future we could integrate this into js-logger to allow specific drilldowns into the logger hierarchy. Especially given that we have log levels as well. I imagine that there's 2 dimensions of filters:

Vertically through the log priority level
Horizontally through what to log, which is how NODE_DEBUG and NODE_DEBUG_NATIVE works.

The * character may be used as a wildcard. Suppose for example your library has debuggers named "connect:bodyParser", "connect:compress", "connect:session", instead of listing all three with DEBUG=connect:bodyParser,connect:compress,connect:session, you may simply do DEBUG=connect:, or to run everything using this module simply use DEBUG=.
You can also exclude specific debuggers by prefixing them with a "-" character. For example, DEBUG=,-connect: would include all debuggers except those starting with "connect:".

The js-logger will probably not work on the basis of environment variables, but instead be configured directly through the root logger to allow dynamic forms of filtering. But I could imagine we augment the --verbose option to also have additional parameters on what to log like --verbose="module:submodule", it'd have to work with varargs of verbosity flags. And also -V should be for --version.

It turns out that because the GC* finalizers may be called after the env_cleanup_hook, then it's possible to have the Detach() be called when the detached target is already closed. So I've removed the precondition of Detach+ methods and DecrementPendingWork from both Database and Transaction classes.

One of the issue with testing right now is that if there's an error during a test, the afterEach hook is still called, and that can be problematic, because the GC finalizers are running at the same time, an await rocksdbP.dbClose(db);is being called. It appears this causes it to never finish its asynchronous work, we need to makedbClose()` a little more robust while concurrent GC is occurring.

A separate problem is that the transaction snapshot is not ensuring repeatable read for iterators created from the transaction. This is somewhat strange. I need to investigate this further.

One of the issue with testing right now is that if there's an error during a test, the afterEach hook is still called, and that can be problematic, because the GC finalizers are running at the same time, an await rocksdbP.dbClose(db); is being called. It appears this causes it to never finish its asynchronous work, we need to make dbClose() a little more robust while concurrent GC is occurring.

Fixed this, this turned out to be an off by 1 error. Well in 2 ways:

The database->isClosing_ wasn't being assigned during dbClose()
The Transaction::DecrementPendingWork was checking if pending work is 0 before queuing the close worker, but it should be checking for 1 instead because the pending work starts at 1 due to Transaction being a child object of Database, and the ref count starts at 1 before being copied to pendingWork_.

So it turns out that because iterators are by default consistent, then the clear worker is also consistent. Right now the dbClear doesn't take a snapshot, but we should allow for that the be case, because it's the same as using dbIteratorInit and then building up a batch to apply, and since dbIteratorInit can take a snapshot now, then dbClear can also take a snapshot.
Then the same principle apples to transactionClear. It would be a matter of getting the iterator from transaction and applying the ClearWorker to it.
At the same time, this convenience method is sort of unnecessary. It's much easier to just write this in JS, then messing around in C++.
The only benefit here is the higher speed of iteration in C++ so it avoids having to go back and forth between JS and C++.

Due to the lack of C++ exceptions, when during a constructor the construction fails, the only thing that can be done is to call return. You cannot return a status indicating the construction failed. You can set a status parameter indicating failure, and then expect the user to check if it is a zombie object however, and I this is sort of how the workers currently work. However it is quite tedious if that's the case.
This causes some problems, if during the construction it has failed, if we use NAPI_STATUS_THROWS_VOID expecting to get a JS exception, this doesn't always occur immediately because C++ execution thread continues, the caller of the construction just continues executing. Then it may reach a segfault at some point. When executing from node, we would only see a segfault, we wouldn't see the exception that should have been thrown.
I think if you're writing a C++ native addon, it makes sense to use C++ exceptions. If you're writing however a C library, or something to be bound into C, it makes sense not to use C++ exceptions as it's not really portable. Therefore rocksdb doesn't use exceptions.
I think in the future, when improving the native addon, we should use C++ exceptions and connect it to the JS exceptions somehow at the very end. But this can be done later in the future.
Anyway I came across this when I was trying to call transactionClear and forgot to wrap it in a promisify, which means no callback was passed in. It ended up as a silent error when attempting to create async work with the NAPI functions, and then eventually resulted in a segfault.

I've added in transactionMultiGetForUpdate as well, strangely missing from transactionGetForUpdate is that it lacks the exclusive option. I'm still not sure what that option means, and whether it is similar to the idea of SELECT FOR SHARE or SELECT FOR UPDATE in SQL.
Atm, I've continued to use the RocksDBGetForUpdateOptions even if the exclusive option has no effect.
My understanding of for share vs for update is only relevant to pessimistic transactions. In pessimistic transactions, this is basically how you "lock" keys between different transactions. For share means you get a read lock, for update means you get a write lock. However in optimistic transactions, this doesn't make any sense, since you're not locking each transaction anyway, therefore GetForUpdate only makes sense as a way to address write skews, that is materialising a write conflict, by writing to the key value with the same value. Therefore I don't think exclusive option makes any sense here, and I should remove it.

Ok I checked postgresql documentation.

REPEATABLE READ corresponds to Snapshot Isolation
SERIALIZABLE corresponds to Serializable Snapshot Isolation
READ COMMITTED is the default isolation level, and it is still working like how SQL stanardised it

In SERIALIZABLE:

Eliminate explicit locks, SELECT FOR UPDATE, and SELECT FOR SHARE where no longer needed due to the protections automatically provided by Serializable transactions.

You don't need any of GetForUpdate at all, and therefore no exclusive parameter.
So in our case as we using SI, we are similar to REPEATABLE READ. In this situation, there is the ability to use SELECT FOR UPDATE and SELECT FOR SHARE. The exclusive option is by default true.
In postgres, it seems to imply that this would provide the ability to do pessimistic locking.

UPDATE, DELETE, SELECT FOR UPDATE, and SELECT FOR SHARE commands behave the same as SELECT in terms of searching for target rows: they will only find target rows that were committed as of the transaction start time. However, such a target row might have already been updated (or deleted or locked) by another concurrent transaction by the time it is found. In this case, the repeatable read transaction will wait for the first updating transaction to commit or roll back (if it is still in progress). If the first updater rolls back, then its effects are negated and the repeatable read transaction can proceed with updating the originally found row. But if the first updater commits (and actually updated or deleted the row, not just locked it) then the repeatable read transaction will be rolled back with the message

However I don't think rocksdb blocks the transaction when using GetForUpdate. I will test this now though.
Furthermore given that MultiGetForUpdate doesn't have the exclusive option, I wonder if that means the option is still by default true?

Ok my tests show that exclusive option seems kind of useless. Because they don't actually block other transactions. If they behaved like it was described in postgres, then if T1 does FOR UPDATE on a key, and then T2 also does FOR UPDATE on a key, but within T1 transaction, then T2 should be blocked on T1 finishing what it should be doing.
In rocksdb, that doesn't occur. I think the exclusive option only has relevant to pessimistic transactions. So I'm going to remove the exclusive option.

It's a shame it seems rocksdb doesn't allow mixing pessimistic and optimistic transactions together.

All native addon functionality is done. Onto DBTransaction.

The DBIterator will need to be extended to be able to handle transaction iteration. I could override it with DBTransactionIterator. But I'm not sure because the constructor has to call super(). The issue being that DBIterator right now ends up calling rocksdbP.iteratorInit instead of rocksdbP.transactionIteratorInit.
Seems like what we need is to override the parent constructor. There is in fact a way to do it: https://stackoverflow.com/a/61541378/582917.
Alternatively I can change the iterator super class to take database or transaction in the same way I have done in C++.

But I do think it would be worthwhile to have either a separate type DBIterator vs TransactionIterator, or something like Iterator that is generic. I reckon we do the same as C++ and use a generic iterator type for both.

When debugging transactions, it can be useful to dump what the transaction is doing.
With this new transaction using rocksdb, we can't really dump the transaction state any more because we are no longer maintaining the transaction overlay state using tran.dump().
But there are a few functions we can use to show us what is happening:

GetId - https://github.com/facebook/rocksdb/blob/fac7a236856207b531342bc22e614aa5bb005a6a/include/rocksdb/utilities/transaction.h#L639-L645
GetID - https://github.com/facebook/rocksdb/blob/fac7a236856207b531342bc22e614aa5bb005a6a/include/rocksdb/utilities/transaction.h#L614
GetWriteBatch - https://github.com/facebook/rocksdb/blob/fac7a236856207b531342bc22e614aa5bb005a6a/include/rocksdb/utilities/transaction.h#L549-L555

In particular GetWriteBatch gives us a:
WriteBatchWithIndex*

We should iterate over that in C++ and return that as an object that could be used as a replacement for tran.dump().
Meaning that tran.dump() gets removed, and we end up having something like tran.ops() or something similar.
In a way this becomes better because it will also show things puts and deletes, and now just what the current state of the transaction is.

Actually since dump just uses an iterator, tran.dump() could still work. But we would need still want to be able to get the ops.

The DBIterator is now generic between DB and DBTransaction similar to how it is done in C++.
In addition, the iteratorRefs also exists in JS.
I'm thinking since the C++ side just autocloses iterators transactions when the root object closes, then perhaps when you commit, rollback or destroy the transaction it should also destroy the iterators. Same with the DB, this then eliminates the DBLiveReference exception.

I'm questioning the existence of tran.finalize().
It seems to only exist because when the commit succeeds.

The only issue now is that rollback originally didn't throw exceptions. But now it could because we discard the transaction on the C++ side.
So now we need to use finally.

The DBTransaction API is bit different from the C++ exposition of transactions. The C++ side provides a lot of flexibility. The DBTransaction wraps it all into a consistent read and consistent write transaction.
In the DB, we can also expose the snapshot feature, but in doing so we will need a DBSnapshot to be provided, which can then be inserted into the DBIterator during construction.
The types are getting a little complicated atm. DBIteratorOptions right now is propagating RocksDBSnapshot | RocksDBTransactionSnapshot type parameter. Need to look into simplification.

I have a problem right with root level iteration. If I don't have ['data', ...levelPath] it appears my iteration iterates nothing even though it should still show data. Marking this to be fixed later.

On top of this, I've found that calling iterator, clear and dump is more likely done with a levelPath as the first parameter, than options. So I'ved switched the parameters around.

I've found that Node NAPI actually allows directly wrapping C++ objects as JS objects with https://nodejs.org/api/n-api.html#object-wrap. The node-addon-api framework also exposes it like: https://github.com/nodejs/node-addon-api/blob/main/doc/object_wrap.md.
This could be useful in the future, since I inherited this architecture from leveldown, I'm basically recreating the JS class architecture encapsulating the C++ objects. I think it makes sense to do this as it would reduce some work, and I could even integrate the js-logger into the C++ side (but of course that would be specific to NAPI).

No DBSnapshot is necessary. If users want sync and snapshot and therefore consistent reads and consistent writes, they need to use the transaction. I'm simplifying this. However leaving this DBSnapshot for posterity:

import type DB from './DB';
import type { RocksDBSnapshot } from './rocksdb/types';
import Logger from '@matrixai/logger';
import { CreateDestroy, ready } from '@matrixai/async-init/dist/CreateDestroy';
import { rocksdbP } from './rocksdb';

@CreateDestroy()
class DBSnapshot {
  protected _db: DB;
  protected logger: Logger;
  protected _snapshot: RocksDBSnapshot;

  get db(): Readonly<DB> {
    return this._db;
  }

  get snapshot(): Readonly<RocksDBSnapshot> {
    return this._snapshot;
  }

  constructor({
    db,
    logger
  }: {
    db: DB,
    logger?: Logger
  }) {
    logger = logger ?? new Logger(this.constructor.name);
    logger.debug(`Constructing ${this.constructor.name}`);
    this.logger = logger;
    this._db = db;
    this._snapshot = rocksdbP.snapshotInit(db.db);
    logger.debug(`Constructed ${this.constructor.name}`);
  }

  async destroy() {
    this.logger.debug(
      `Destroying ${this.constructor.name}`,
    );
    this._db.snapshotRefs.delete(this);
    await rocksdbP.snapshotRelease(this._snapshot);
    this.logger.debug(
      `Destroyed ${this.constructor.name}`,
    );
  }
}

export default DBSnapshot;

When debugging transactions, it can be useful to dump what the transaction is doing.
With this new transaction using rocksdb, we can't really dump the transaction state any more because we are no longer maintaining the transaction overlay state using tran.dump().
But there are a few functions we can use to show us what is happening:

GetId - https://github.com/facebook/rocksdb/blob/fac7a236856207b531342bc22e614aa5bb005a6a/include/rocksdb/utilities/transaction.h#L639-L645
GetID - https://github.com/facebook/rocksdb/blob/fac7a236856207b531342bc22e614aa5bb005a6a/include/rocksdb/utilities/transaction.h#L614
GetWriteBatch - https://github.com/facebook/rocksdb/blob/fac7a236856207b531342bc22e614aa5bb005a6a/include/rocksdb/utilities/transaction.h#L549-L555

In particular GetWriteBatch gives us a:
WriteBatchWithIndex*

We should iterate over that in C++ and return that as an object that could be used as a replacement for tran.dump().
Meaning that tran.dump() gets removed, and we end up having something like tran.ops() or something similar.
In a way this becomes better because it will also show things puts and deletes, and now just what the current state of the transaction is.

The GetID and GetId don't appear to work, I tried them with 2 concurrent transactions, and they both appear to return 0. So I'm just using the id_ property instead, which is our own ID being used to keep track of the transaction.

A few things that can be improved in subsequent PRs:

Overall code architecture between JS classes, NAPI C++ classes, and rocksdb internal classes, and integrating C++ exceptions as well dealing with asynchronous errors properly.
The usage of GetWriteBatch returns a map structure called WriteBatchWithIndex which can be used to interrogate what is inside a transaction, memory management will need to be made more automatic by using shared pointers. And consider using the framework node-addon-api to make the code cleaner, but this should be held off until it is understood how to port this to Android/iOS with native code.
Retrying transactions is inefficient. If a transaction conflict occurs, it is currently necessary to recreate the entire transaction object. This is because the C++ code itself destroys the properties early if committing was not allowed. If we loosen this, then allow resetting the transaction snapshot, it should be possible to "retry" a transaction just be recalling the commit. (Actually I might try this now to see if it can be easily implemented after getting all the tests passing).

All tests pass again. The new SI transaction system is working quite well.
Need to integrate the getForUpdate calls to deal with write skew however for DBTransaction.
I think I'll leave task 12 to a later date, as a new issue.

I've added a test PCC locking to prevent thrashing for racing counters that demonstrates how to address racing counters atm. This is particularly relevant to @tegefaulkes.
Note that since the DBTransaction doesn't have any native locking yet as per task 26., this means locking has to be done outside of transaction construction. Let me know if this will cause problems. If it does, we have to address task 26, otherwise I can push that to be done later.
The main thing for implementing task 26, is to integrate LockBox into DBTransaction. It would need to ensure that locks are only released when the transaction is committed or rollbacked. Deadlock detection is an optional feature on top of that. Any implementation of this should also look into if we can also optimise the ability to retry transactions.

Task 17 can only be done after merging to staging.

Ok to solve task 26, we need to introduce the LockBox.
It has to be shared between all the transactions, which means it's going to be stored on DB.
Each transaction will then expose a Transaction.lock method.
So it may look like:
t1 = withF([db.transaction()], async ([tran]) => {
  await tran.lock(['counter', Lock], ['someotherkey', Lock]);
});

t2 = withF([db.transction()], async ([tran]) => {
  await tran.lock(['counter', Lock]);
});

await Promise.allSettled([t1, t2]);

Note that tran.lock would be the LockBox.lock.
However LockBox.lock takes Array<LockRequest>.
I'm wondering if its worth simplifying this. By convention we should be locking based on key paths, but really anything could be used. If we simplify things, one has to then be able to take just a string, and then the default lock constructor can be Lock to ensure mutex.
Furthermore if RWLock is used, then you get alot of flexibility with how you want to lock things.
Most important is that the locks are only released after you commit or rollback.
No deadlock detection, this can be addressed later.
As for re-entrant locking, that's something that should also be done, but can also be done later.
So future work:

Deadlock detection
Allow re-entrant locking, by tracking what has been locked within a transaction. Use a Set<ToString> to check for membership.
Allow lock upgrading - this is more for js-async-locks, to allow one to upgrade locks from read to write locks or downgrading

But I do want to add in default lock constructor if it is not specified, it should just be Lock. This will need update on js-async-locks.

Yea so LockBox.lock returns a ResourceAcquire. This means it's a bit clunky to use, requiring you to use it with the withF internally.
I reckon this is a bit incorrect, since locks are not a separate resource within the transaction, instead transaction locks are properties of the transaction itself. Therefore Transaction.lock will instead just return Promise<void>.
This would allow one to do things like:
await tran.lock('abc');
tran.unlock('abc');

In addition to this, the tran.lock('abc') may return a Promise<[ResourceRelease]>. Alloying you keep a function reference to the release.
To be able to do tran.unlock('abc') would require us to keep a reference to all releasers in a Map. If we need a Map to do re-entrant locking, we might as well make this possible.

Ok js-async-locks is queued for 3.0.0 update.
It's all done, PCC locking is now available in DBTransaction.

Subsequent work must take place on the staging branch in order to run the builds and test all the release jobs.

This should trigger v5 of js-db.


Bug Fixes

Attempts to address #1065 and #1066

Coverage decreased (-0.2%) to 89.04% when pulling 6684fdacbc79792ea48fa558d5ccff5aa230e591 on tgianos:issues/1066 into ccfe136cbe48a35abc20034f7966d2854f227688 on Netflix:3.3.x.

Coverage decreased (-0.2%) to 89.04% when pulling 6684fdacbc79792ea48fa558d5ccff5aa230e591 on tgianos:issues/1066 into ccfe136cbe48a35abc20034f7966d2854f227688 on Netflix:3.3.x.


trimTrailingWhitespace conflicts with insertFinalNewline

trimTrailingWhitespace deletes the line created by insertFinalNewline.

VSCode Version: Code 1.10.2 (8076a19fdcab7e1fc1707952d652f0bb6c6db331, 2017-03-08T13:56:35.908Z)
OS Version: Darwin x64 16.4.0
Extensions:

Extension
Author
Version

xml
DotJoshJohnson
1.7.0

html-css-class-completion
Zignd
1.0.3

html-snippets
abusaidm
0.1.0

path-intellisense
christian-kohler
1.2.0

gitignore
codezombiech
0.5.0

githistory
donjayamanne
0.2.0

vscode-html-css
ecmel
0.0.33

tslint
eg2
0.8.1

php-debug
felixfbecker
1.10.0

php-intellisense
felixfbecker
1.1.3

Angular2
johnpapa
2.0.3

vscode-JS-CSS-HTML-formatter
lonefy
0.2.2

Go
lukehoban
0.6.55

debugger-for-chrome
msjsdiag
2.6.0

vscode-icons
robertohuertasm
7.3.0

sass-indented
robinbentley
1.3.0

stylelint
shinnn
0.23.0

cordova-tools
vsmobile
1.2.7

bootstrap-3-snippets
wcwhitehead
0.0.9

JavaScriptSnippets
xabikos
1.4.0

Steps to Reproduce:

add "files.trimTrailingWhitespace": true to settings.json.
add "files.insertFinalNewline": true to settings.json.
reload.

@ir-ma3x i cannot reproduce this. i.e. this appears to be working correctly for me:
can you perhaps try with disabling all extensions (code --disable-extensions):

I'm seeing this as well. @alexandrudima, after saving go and cat the file involved, you will see it has no new line.

@c4milo
Does this issue occur in any file or only on .go files?
Can you reproduce when running code --disable-extensions ?

I was testing with a Vagrantfile and it was happening. I think files.insertFinalNewline is just not doing its thing because it happens even having files.trimTrailingWhitespace in false

@alexandrudima I just tested with --disable-extensions and it seems to work well.

@c4milo Have you found any steps / pattern for when this doesn't work ?
This works consistently as expected for me:

when running with code --disable-extensions.
when editing files in a fresh folder (i.e. no workspace settings), only files.insertFinalNewline

Looping in @bpasero , he knows all that is involved in the file save path.

@alexandrudima, yes, I basically go and edit the file in vscode and double check it with vim and hexdump. I never see the newline that vscode was supposed to append at the bottom. To work around it, I'm inserting two newlines in vscode to really get the result I want.

I can also not reproduce, sorry. @c4milo can you share your full settings file? Do you have workspace settings configured?

I was under the wrong assumption regarding POSIX's new lines. This works as expected. Thank you!

I can also not reproduce, sorry. @ir-ma3x can you share your full settings file? Do you have workspace settings configured?


Verifying that update_param in the paper=='all'

Hi,
I want to verify with the author that the model performance reported in the paper is always by updating the whole backbone parameters right? Otherwise, line 198 in unsupervise_adapt.py looks like a tiny bug to me.

We update all trainable parameters as we claimed in paper Sec 4.1 implementation.
Unlike [45, 66], all the trainable layers are updated and no special selection is required in our method.