-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Overlay index fetch issue #82
Conversation
As mentioned in #80, there were issues when initializing the I also found the bug which caused the double unlocking bug. This occurs if the program locks a path 3 times. |
@terryluan12 Some changes have been made. Primarily, the |
Ah, didn't even notice it, thanks for the heads up! I just merged + re-added the changes |
src/backends/index/index.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes relate to the issue you mentioned in #80, please refer to my other comment.
@@ -26,7 +26,7 @@ export class LockedFS<FS extends FileSystem> implements FileSystem { | |||
/** | |||
* The current locks | |||
*/ | |||
private locks: Map<string, MutexLock> = new Map(); | |||
private locks: Map<string, [MutexLock]> = new Map(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a tuple with a single MutexLock
. Do you mean an array, MutexLock[]
?
@@ -83,8 +88,8 @@ export class LockedFS<FS extends FileSystem> implements FileSystem { | |||
} | |||
|
|||
// Non-null assertion: we already checked locks has path | |||
this.locks.get(path)!.resolve(); | |||
this.locks.delete(path); | |||
const res = this.locks.get(path)!.shift(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, please inline: this.locks.get(path)!.shift().resolve();
This should have been resolved in ec86a6a. Please let me know if the issue still happens- you will need to run |
@terryluan12 Any updates on this? |
@terryluan12 Any updates on this? |
Hi @james-pre, so sorrry about the lack of update! Got busy with some real life stuff, and didn't see the comments. Just got a replacement for my previous laptop, and will get to work on zen-fs, and finish up this issue ASAP |
Hey @james-pre, I've been taking a look at the double unlocking issue, and I've gotten an idea of why it's bugging. Specifically, it occurs when you have two processes waiting on a locked promise. Once the promise resolves, both processes will continue at the same time. Thus they'll both call addLock, and one promise will override the others, causing the weird double unlocking. Thinking about it, I think one possible solution is to implement a queuing system. What are your thoughts? |
Thanks for your dedication to this. My thinking is that we should try to avoid a queue system. It should work like this:
I think e387889 may fix something since now the promise won't resolve until the lock is actually cleared. |
No worries at all, thanks for allowing me to help! I think there's been a misunderstanding. To clarify, the issue occurs in the following scenario, when there are 3 locks; for example A, B, and C. When A then releases, both B and C start again at the same time. I believe this is the unintended behaviour, where they both start again at the same time. |
Ah, this makes more sense now! Thanks for the clarification. Maybe we could look at a wrap and replace, so when B attempts the lock, it replaces the current lock with one that resolves when B is done. This wouldn't impact A since it holds the lock, and doesn't update the reference (i.e. the A's |
@terryluan12 Now that the mutex issue has been solved, can I close this PR? |
Yep, absolutely! |
Should resolve #78 and one of the additional bugs mentioned in #80