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

Obscure error in getPathforOid #279

Open
ale-rt opened this issue Sep 2, 2019 · 2 comments
Open

Obscure error in getPathforOid #279

ale-rt opened this issue Sep 2, 2019 · 2 comments

Comments

@ale-rt
Copy link
Member

ale-rt commented Sep 2, 2019

While debugging an issue with @pysailor we found that a try/except in getPathForOID is masking an OSError with an AssertionError:

ZODB/src/ZODB/blob.py

Lines 432 to 435 in 8f5ac63

except OSError:
# We might have lost a race. If so, the directory
# must exist now
assert os.path.exists(path)

At least logging the OSError would help understanding what is wrong (in our case it was related to the fact that we had no more inode available in the filesystem).

@ale-rt
Copy link
Member Author

ale-rt commented Sep 2, 2019

Any suggestion for a future PR?

Given that assertion are stripped when the -O flag is set is that code safe at all? See:

[ale@emily ~]$ cat tmp/a.py
assert False
print("Hello world!")
[ale@emily ~]$ python3 tmp/a.py
Traceback (most recent call last):
  File "tmp/a.py", line 1, in <module>
    assert False
AssertionError
[ale@emily ~]$ python3 -O tmp/a.py
Hello world!

Should not we just raise the OSError if the directory does not exist?

@tseaver
Copy link
Member

tseaver commented Sep 2, 2019

Should not we just raise the OSError if the directory does not exist?

I'm not sure: the point of the try: ... except OSError: block is to ensure that the directory exists: if another thread / process has created it, then the function can return normally. I guess we could check the errno of the exception, e.g.:

        if create and not os.path.exists(path):
            try:
                os.makedirs(path)
            except OSError as exc:
                # We might have lost a race.  If so, the directory
                # must exist now
                if exc.errno != errno.EEXIST:
                    raise
                assert os.path.exists(path)
        return path

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

No branches or pull requests

2 participants