fixed mutex issue #80
Conversation
* fixed stat function in overlay.ts not throwing error properly
|
@terryluan12 If you don't mind, please add tests. Thanks. |
Yep absolutely! I'll probably get the tests done on Wednesday |
|
Sounds good. Thanks again for the PR. |
|
Hey @james-pre should be good to review again, sorry about the delay. I think the const mutex = new Mutex();
const foo = () => {
mutex.lock("path")
// critical section
mutex.unlock("path")
}
spawnThread(foo)
spawnThread(foo)in the scenario above, if the first thread were to be inside the critical section at the same time as the second thread were to start locking, and the second thread were to check the length of the queue, it would see it as 0. |
There was a problem hiding this comment.
This is why Mutex.lock is awaited every time it is called. Your example incorrectly uses the functions, so naturally it won't work correctly. Mutex.lock should await the previous lock.
const mutex = new Mutex();
async function foo(n: number): void {
console.log(`#${n} locking...`);
await mutex.lock('path');
console.log(`#${n} locked`);
// something important
console.log(`#${n} unlocking...`);
mutex.unlock('path');
}
foo(1);
foo(2);In essence, this means the above example should always behave like so:
#1 locking...
#2 locking... OR #1 locked
#1 locked OR #2 locking...
#1 unlocking...
#2 locked
#2 unlocking...
I'd prefer the code to only store one promise in the mutex for each path.
|
Sorry, I forgot to add the await keyword before To clarify, I was just attempting to justify why I thought the |
|
In any case, there should only ever be one lock on a path and therefore one promise. To be honest, my current implementation is not good and does not follow that correctly. I think ideally it would look something like this: interface MutexLock<T> {
promise: Promise<T>;
resolve(): T;
reject(): Error;
}
class Mutex {
protected locks: Map<string, MutexLock<void>> = new Map();
public async lock(path: string): Promise<void> {
if(this.locks.has(path)) {
await this.locks.get(path).promise;
}
let resolve: () => T;
let reject: () => Error;
const promise = new Promise<void>((_resolve, _reject) => {
resolve = _resolve;
reject = _reject;
});
this.locks.set(path, { promise, resolve, reject });
// Do not return the promise!
// It will be resolved on unlock()
}
public unlock(path: string): void {
if(!this.locks.has(path)) {
throw new ErrnoError(/* ... */);
}
// We have already checked that locks has path
this.locks.get(path)!.resolve();
}
}I had already some code similar to this about a month ago. I did not push it after you opened the PR since I didn't want to create any confusion. Please let me know what you think.
The "queue" is meant to track active locks. Since there should only ever be one lock at a time for a path, having an array doesn't really make sense. Does this clear up what the locks is meant to do? Thanks, |
|
Kind of? I took a look over the ideal code you had sent, and I'm a little confused on how the code should run. I'm assuming a similar setup to the test code up above with the Since the code awaits until the promise resolves, the execution freezes. However, within the |
Yes, it should be used like the example.
Please note the comment that says "do not return this promise". I was trying to explain that we create a new promise for the lock, but do not await it (that way code execution continues). Lock essentially works like so:
Also, This all means that from when |
|
Oh okay, I think that makes sense! In that case, would you like to implement it, or should I? |
NOTE: the implementation uses `Promise.withResolvers()` (see https://mdn.io/Promise.withResolvers)
Minor test syntax changes
|
@terryluan12 I went ahead and implemented it, I hope you don't mind. Note the new implementation uses |
Sounds good, thank you for your help! I'm trying to fix an issue with the I'm also still getting the hanging issue in the mutex input. I have updated the zen-fs example program I've been using here (https://github.com/terryluan12/zen-fs-example). |
Added documentation for `Mutex.unlock`
|
Just as an update, I figured out that the issue with the hanging wasn't a problem with the mutex, but with |
|
I think I have narrowed down the two bugs. From the This error only occurs when reactStrictMode is turned on. Error two: This error only occurs when you are calling Both seem to be race conditions, with the first one occurring due to the re-render of the component, and the second one occurring due to the async I think I'm a bit stumped as to good ways to resolve these race conditions though, and would love your help/feedback. Please let me know! |
|
In response to your first comment: Note this try/finally logic can be nicely abstracted away with using _ = await this.lock(path);
// ... ops ...Since the I digress, these are some future ideas that would really help the codebase. Thank you again for the PR, I really appreciate it. |
Of course!
For this error, the only advice I have is to check what other FS functions were called before. I will be out of town this weekend so can't do any in depth debugging, though I will try to help as much as I can.
This makes sense. This problem also hints at a potential bug with |
Sounds good, I believe I responded! I just made a change to hopefully improve readability? Let me know |
Sounds good, thanks for the knowledge! I hadn't heard of explicit resource management before! I'll give it a go a bit later (I may also put it in a new issue if that's alright). For now, I want to get all the bugs out of the zentest repo. |
No worries, thanks for your help so far! I'll focus on the second problem for now. You mentioned that |
This caching is unrelated to |
|
Why the change to using a regex in |
|
I think I understand the change made. I believe it is not needed because of the Also, I managed to get the |
|
Sounds good! In that case, everything looks good to me, feel free to merge! |
|
Thanks for all your help! |
Fixes #79