Skip to content
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

The readdirSync and readdir functions both have a bug when one filesystem has an empty directory #742

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

williamstein
Copy link

The readdirSync and readdir functions both have a bug when there's 2 filesystems with this.fss[1] having the
directory but it is empty and this.fss[0] not having the directory. In that case checking the
size doesn't work to know if the directory exists. In particular, the fix at
#348 is incorrect. This PR fixes both bugs and adds examples to the test suite.

@G-Rath
Copy link
Collaborator

G-Rath commented Aug 19, 2022

@magarcia do you have any thoughts on this?

…ystems and a file exists but has the wrong property (e.g., directory versus file); hit this when fixing failures in cpython test suite
@williamstein
Copy link
Author

I'm working on getting the cpython test suite to pass (as part of https://github.com/sagemathinc/python-wasm which uses unionfs), and this revealed another bug in unionfs, which I've also fixed here (and added tests).

This test in the python test suite:

import os; os.listdir('test_exceptions.py')

was supposed to raise

NotADirectoryError: [Errno 54] Not a directory: 'test_exceptions.py''

but was instead raising

FileNotFoundError: [Errno 44] No such file or directory: 'test_exceptions.py'

I traced this (through WebAssembly, WASI, etc.) to how exceptions are raised in unionfs. Basically, unionfs assume that if there is any exception doing anything then it is because of ENOENT. However, there are other reasons to get an error, e.g., ENOTDIR is thrown when you try to open a file as a directory (as is the case in the test above). unionfs is just treating that as a file not found situation and looking further. I think that's the wrong behavior -- instead if an error other than ENOENT is raised, then immediately stop searching through the union and throw that exception. At least that addresses the above problem. I implemented this for readdir, readdirSync, and also asyncMethod and syncMethod. I don't know if that's enough to fully do this right.

Anyway, I hope this is helpful. I case this never gets looked at and somebody else hits this, I'm maintaining a temporary fork here with these fixes: https://www.npmjs.com/package/@wapython/unionfs

-- William

@williamstein
Copy link
Author

I pushed another commit that limits the scope of this change to only ENOTDIR errors. Otherwise, the latest change would cause other problems.

@williamstein
Copy link
Author

And also EEXIST errors, as I found when running the cpython test_gzip.py part of their test suite...

@G-Rath
Copy link
Collaborator

G-Rath commented Oct 27, 2022

Just want to let you know I've not forgotten about this, but it looks like a potentially significant change and we're blocked on doing releases due to #741 so I feel its a bit moot to dive into until that is resolved.

I'm going to try and reach out to @streamich on other channels to see if I can get us unblocked for all the *fs packages.

@williamstein
Copy link
Author

Thanks. I understand, and am fine using my slight fork in the meantime.

@G-Rath
Copy link
Collaborator

G-Rath commented Jun 2, 2023

@williamstein we've able to do releases again, if you want to pick this back up :)

@dy-dx
Copy link
Contributor

dy-dx commented Mar 19, 2024

A simplified version of this fix, without ENOTDIR handling, was merged: https://github.com/streamich/unionfs/pull/787/files

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants