Upgrading @matrixai/async-init, @matrixai/async-locks, @matrixai/db, @matrixai/errors, @matrixai/workers and integrating @matrixai/resources #63
Conversation
CMCDragonkai
changed the title
|
This is a big upgrade... it would result in |
|
The |
|
In integrating the If we remove the requirement, and that would mean That could simplify the code and remove all the |
|
Only issue is that some places the |
|
The |
|
When I removed the However I wanted to improve the API a little bit. Since we now know how to do So now we have: public async withTransactionF<T>(
...args: [
...inos: INodeIndex[],
f: (tran: DBTransaction) => Promise<T>
]
) {
const f = args.pop() as (tran: DBTransaction) => Promise<T>;
return withF(
[
this.db.transaction(),
this.getLocks(...args as Array<INodeIndex>)
],
([tran]) => f(tran)
);
}
public withTransactionG<T, TReturn, TNext>(
...args: [
...inos: INodeIndex[],
g: (tran: DBTransaction) => AsyncGenerator<T, TReturn, TNext>,
]
): AsyncGenerator<T, TReturn, TNext> {
const g = args.pop() as (tran: DBTransaction) => AsyncGenerator<T, TReturn, TNext>;
return withG(
[
this.db.transaction(),
this.getLocks(...args as Array<INodeIndex>)
],
([tran]) => g(tran)
);
}This actually conveniently replaces the This is nicer than having the inodes that you need to lock at the very end. Furthermore, notice that I "specialise" the resources to just the transaction, because the user won't have any use for the lock objects. The user can always still compose much more complex resource contexts by using the |
|
Also moving to selective imports so that tests can be run by themselves. To: |
|
I tried getting a vscode regex to do a big find and replace. Couldn't work due to matches braces requirements. |
|
There are no functions that are exposing a generator interface atm. Even streams are using the FD abstraction, and they don't actually hold lock/transaction during their lifetime. This is because every write is a single operation. So for now there's no use of |
|
I've found that the |
|
The And also for |
|
Everything is migrated over, however I got a few test failures in |
|
The canary check logic should be moved into DB during |
|
This is now done in |
|
Current tests state, all passes except 2: First: Both appear to involve concurrent writing/reading to file blocks which is the trickiest part, and that which is impacted by #63 (comment) |
|
Replacing |
|
No problems integrating In doing so I discovered that there's no need to remove entries from the lock collection since the lock entries are now removed when there is nothing holding them. This does mean that it has to construct the lock object each time someone is trying to lock it. Which can be a bit inefficient. But at the same time, not having to keep it around reduces programmer burden to figure out when to clear or delete a lock entry. In the future, the |
|
For the first test failure involving It appears the The data in teh I confirmed this with a normal |
|
The Need to compare with what is in |
|
In master, it doesn't go into that case at all. It instead goes to The reason is because: Is This is quite strange. Because in However in the current PR, it does exist. Now should it exist or not? If you already have a file that has Therefore it seems to make sense that the block would still exist, and the |
|
Reduced this problem to the simplest reproduction: import fs from 'fs/promises';
async function main () {
await fs.writeFile('./tmp/fdtest', 'abcdef');
const fd = await fs.open('./tmp/fdtest', 'r+');
await fd.truncate(4);
await fd.close();
// File is abcd
console.log(await fs.readFile('./tmp/fdtest', { encoding: 'utf-8' }));
}
main();vs: import os from 'os';
import fs from 'fs';
import pathNode from 'path';
import Logger, { StreamHandler, LogLevel } from '@matrixai/logger';
import { EncryptedFS, utils } from './src';
async function main () {
const logger = new Logger('EncryptedFS Files', LogLevel.WARN, [
new StreamHandler(),
]);
const dataDir = await fs.promises.mkdtemp(
pathNode.join(os.tmpdir(), 'encryptedfs-test-'),
);
const dbPath = `${dataDir}/db`;
const dbKey: Buffer = utils.generateKeySync(256);
const efs = await EncryptedFS.createEncryptedFS({
dbKey,
dbPath,
umask: 0o022,
logger,
});
await efs.writeFile('/fdtest', 'abcdef');
// File is abcdef
console.log(await efs.readFile('/fdtest', { encoding: 'utf-8'}));
const fd = await efs.open('/fdtest', 'r+');
// File is abcdef
console.log(await efs.readFile('/fdtest', { encoding: 'utf-8'}));
await efs.ftruncate(fd, 4);
await efs.close(fd);
// File should be abcd, but is instead abcdef
console.log(await efs.readFile('/fdtest', { encoding: 'utf-8'}));
}
main();In the current branch, the resulting file isn't truncated. It's still In the |
|
The main difference is in When looking up the block during truncation, in However in the current branch, it does find the block which means it doesn't write a new block being just The question is why does |
|
The This could mean that it is correctly acquiring the block in the current branch, because it is still I'm not sure if the |
|
I noticed that This would mean that it is intended that the block is However it appears that even though it is clearing the right key: |
|
In the case of |
|
The
I have debated with myself on whether to lock the directory then target, or target than directory. I'm not sure what the implications are atm. So you'll see through testing. |
task referencechecklist:
list of methods using transactions
|
|
There seems to be a bug when concurrently streaming writes to a file and truncating said file. Normally I'd expect that the size of a file when using This is likely a problem with the |
|
I've found another possible bug. When using But I'm seeing a stat size of 24 and the contents of the file just the 4 bytes written by |
|
General ETA for what is left. refering to the above reference. task 3. is hard to pin down. It's revealing small bugs as I progress so the overall time depends on things that are not fully known yet. But first blush i'm expecting up to 1 days to finish the tests and possibly 2 more to fix any bugs. For task 4. should be half a day if that at most. 5. and 6. are trivial and should take no time at all. So overall;
Should be done within 4 days. |
tegefaulkes
force-pushed
the
dbanderrors
branch
2 times, most recently
from
b5b5969 to
235f858
Compare
|
Cleaned up the commits |
We check after the navigation step aswell as after acquiring the lock. This Means we will still throw an `ENOENT` if something happened to the target before acquiring the lock. This needs to squash into `wip: checking target exists after lock`
Write was not zero filling bytes when seeking past the end of a file. This was only a problem in one specific case. That was when you created a new file, seeked ahead and then wrote to the file. Existing test only checked when the file already existed.
…arameters When you copied a file and got the stat the resulting blocks and size parameters would be 0. Just needed to set the parameters after copying.
|
The |
|
Note that |
CMCDragonkai
changed the title
Description
Several core libraries have been updated and js-encryptedfs needs to start using them.
@matrixai/async-init- no major changes@matrixai/async-locks- theINodeManageruses a collection ofMutexfor mutual exclusion of inodes. This means operations to individual inodes may be blocked. We can switch this withLock, and in the future optimise withRWLockWriterorRWLockReader@matrixai/db- the DB now supports proper DB transactions, we should switch theINodeManagerAPI to fully support it@matrixai/errors- we make all of our errors extendAbstractError<T>and also provide static descriptions to all of them, as well as use thecausechain@matrixai/workers- no major changes here@matrixai/resources- since the@matrixai/dbno longer does any locking, the acquisition of theDBTransactionandLockhas to be done together withwithForwithGIssues Fixed
Tasks
AbstractErrorand usecausechain and static descriptions@matrixai/dbintoINodeManager, remove sublevel objects and replace with full keypaths.@matrixai/async-locksto use theLockclass instead ofMutexfromasync-mutex@matrixai/resourceswithFandwithGto replace some of ourtransactAPI.FileDescriptor.writeandFileDescriptor.readshould be using a single transaction context for the entire operation: Upgrading @matrixai/async-init, @matrixai/async-locks, @matrixai/db, @matrixai/errors, @matrixai/workers and integrating @matrixai/resources #63 (comment)tests/inodes[ ] 9. Consider optimising- do this in Identify and Eliminate Unscalable Operations with Lazy GC #53EncryptedFS.ftruncateso that it does not need to iterate over the entire file blocks before truncating, this targets Identify and Eliminate Unscalable Operations with Lazy GC #53fileGetBlocksand see if it correctly implemented - infinite loop solved by incrementing the block count inside the while loop, and added new test toINodeManager.file.test.tsread sparse blocks from a fileto cover thistests/utils.ts#expectErrorhas been changed to take the exception class as well, and then theerrnoobject to be checkedftruncateFileDescriptorhaving its own lock. This prevents some stream concurrency errorsnavigateFromseems racy with multiple transaction contexts being used_openseems racy with the final FD creation with respect to inode creationmkdirseems to have strange behavour when the target already exists, need to compare behaviour with real filesystemmkdirFinal checklist