Callback swallowed leading to Mutex unlock error

[1j01]

Whew! This took some debugging.

I got Uncaught Error: unlock of a non-locked mutex, and at first I thought it was the readdir failing (and it kind of is), then I noticed that I had typo'd "file" instead of another variable, and I was trying to hunt down where that variable came from, assuming it was some sort of getter that was causing the error, but it turned out that any error in my callback would cause the mutex error.
So I continued debugging, and here's what I found:

• It's swallowing errors when it should check for ENOENT, in OverlayFS's readdir, here (unrelatedly it should probably also check for ENOENT here)
• It's wrapping my callback in XmlHttpRequest.prototype.readdir, and then
• It's calling my callback twice! the second time to handle the error from the first callback!

BrowserFS/src/backend/HTTPRequest.ts

Lines 341 to 347 in 73e4eca

	public readdir(path: string, cb: BFSCallback<string[]>): void {
	try {
	cb(null, this.readdirSync(path));
	} catch (e) {
	cb(e);
	}
	}

The combined result of this is that swallows any errors in my callback, and gives that mutex error instead of any reasonable error.

I'll send a PR for this.

[jvilk]

Good catch! I should do an audit of the codebase, looking for potential re-applications of callbacks.

Is the OverlayFS issue that stat is swallowing an error code that it should return?

The stat code is doing a mkdir -p to find the parent directory that exists before creating all needed subdirectories; what error code is it swallowing that you'd expect it to return?

Similarly, readdir may operate on directories that only exist in writable, so errors from readable.readdir are expected. Is there a particular error code it's swallowing that it should return instead? Note that this code may also receive a ENOTDIR error if readable contains a file that was later deleted, so it's a bit more subtle than just checking for ENOENT.

[1j01]

Is the OverlayFS issue that stat is swallowing an error code that it should return?

I don't think the statDone one affected me, I was only using readdir (I haven't created any directories yet)

what error code is it swallowing that you'd expect it to return?

Possibly EPERM, or an unexpected type of error like ended up getting passed to the callback in readdir (which should be an ApiError) this._readable.readdir(p, (err: ApiError, rFiles: string[]) => {
But I guess maybe not.

The main thing was the double callback. I'll send a PR for that at least. And the callback being part of a try block at all.

I should do an audit of the codebase, looking for potential re-applications of callbacks.

I've started doing this audit now, and there sure are a lot of cases, but I'm on a roll. 😄

[jvilk]

Ignoring EPERM should be fine; it could be a directory that you didn't have permission to, a chmod caused it to move to writable (because you updated the directory/file), and now you do have access to it. Keep in mind readable is a read-only snapshot of some file system, and writable stores the changes. Data in readable may be out-of-sync with reality.

[1j01]

Yeah, that makes sense.

[dr-vortex]

@1j01 What is the status of this issue? I've made a ton of improvements to BFS in recent months so this issue may not be relevant or present anymore.

[dr-vortex]

Please use https://github.com/browser-fs/core/issues/14