ZODB icon indicating copy to clipboard operation
ZODB copied to clipboard

Obscure error in getPathforOid

Open ale-rt opened this issue 4 years ago • 2 comments

While debugging an issue with @pysailor we found that a try/except in getPathForOID is masking an OSError with an AssertionError: https://github.com/zopefoundation/ZODB/blob/8f5ac63a3d8b4611bcbcaa68f30b48e2c8389fd2/src/ZODB/blob.py#L432-L435

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 avatar Sep 02 '19 17:09 ale-rt

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?

ale-rt avatar Sep 02 '19 17:09 ale-rt

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

tseaver avatar Sep 02 '19 18:09 tseaver