ipaclient: schema cache: Write all schema files in concurrent-safe way

[ghost]

https://fedorahosted.org/freeipa/ticket/6668

[tiran]

EnvironmentError is wrong on Python 3:

>>> os.rename('foo', 'bar')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
FileNotFoundError: [Errno 2] No such file or directory: 'foo' -> 'bar'
>>> FileNotFoundError.__mro__
(<class 'FileNotFoundError'>, <class 'OSError'>, <class 'Exception'>, <class 'BaseException'>, <class 'object'>)

[ghost]

AFAIK EnvironmentError in python 2 is super class for IOError and OSError, in python3 EnvironmentError and IOError are aliases for OSError. So I believe this is correct way of catching any OS/FS-related error in both python 2 and python 3.

[tiran]

OSError is sufficient. os.rename() can never raise IOError.

[ghost]

Since I've first read PEP 3151 (expecially section about Confusing OS-related exceptions [1]) I like to play it safely and always catch EnvironmentError because (like to PEP says):
In fact, it is hard to think of any situation where OSError should be caught but not IOError, or the reverse.

[1] https://www.python.org/dev/peps/pep-3151/#confusing-set-of-os-related-exceptions

[tiran]

I find path confusing because path it is not clear if you talk about a directory or file name. How about filename?

[ghost]

build-in open uses 'name' in python 2 and 'file' in python 3. So why not? I can combine it to 'filename'.

[tiran]

prefix is not enough. You have to include the dir arg to ensure that the file is created in the same directory as the target dir. Otherwise both files may end up on two diferent file systems and os.rename() is no longer atomic.

https://docs.python.org/3/library/tempfile.html#tempfile.mkstemp

You also can't just yield the temp file or its file attribute, because with concurrent_open(...) will call TemporaryFile.__exit__, which closes the fd and breaks fsync().

class RenameContext(object):
    def __init__(self, temp_file, destpath):
        self.temp_file = temp_file
        self.destpath = destpath

    def __enter__(self):
        return self.temp_file.file  # raw file

    def __exit__(self, exc, value, tb):
        f = self.temp_file
        f.flush()
        os.fsync(f.fileno())
        f.close()
        try:
            os.rename(f.name, self.destpath)
        except OSError:
            os.unlink(f.name)  # only Windows developers call this remove :)
            raise

And then

filename = os.path.abspath(filename)
directory, prefix = os.path.splitpath(filename)
with tempfile.NamedTemporaryFile(mode=mode, dir=directory, prefix=prefix, delete=False) as f:
    return RenameContext(f, filename)

[ghost]

Regarding the prefix you're right, I haven't thought about relative names. I know that rename works only inside single FS otherwise raises error.

Regarding yielding NamedTemporaryFile instance from concurrent_open I'm not sure I understand you. Do you want to say that NamedTemporaryFile.__exit__ is called when yield temp_file is executed on line 300? I don't agree with that because the context haven't finished yet. The problem I see not with line 300 is that I should enclose it with try: finally:.

[tiran]

Ah, I didn't see that you wrapped concurrent_open in a @contextmanager. In that case the __exit__ function of NamedTemporaryFile is not called because the function doesn't return the NamedTemporaryFile instance directly but rather a wrapped instance.

try/except OSError is fine. You don't want to call os.unlink() when os.rename() was successfully called.

[tiran]

PS: Without dir argument, the tempfile module uses the system's temp file directory. (TMPDIR, TEMP, TMP, /tmp, /var/tmp). These days temp dir are on a different file system, partly for performance, partly for security reasons.

[tiran]

The new try/finally also overrides the destination file when there is an exception. How about:

            try:
                yield temp_file
            except Exception:
                raise
            else:
                temp_file.flush()
                os.fsync(temp_file.fileno())
                temp_file.close()
                os.link(temp_file.name, filename)
            finally:
                os.unlink(temp_file.name)

The os.unlink() function is always executed as last statement, https://www.python.org/dev/peps/pep-0341/

[tiran]

correction, unlink doesn't override destination.

            try:
                yield temp_file
            except Exception:
                raise
            else:
                temp_file.flush()
                os.fsync(temp_file.fileno())
                temp_file.close()
                os.rename(temp_file.name, filename)
            finally:
                try:
                    os.unlink(temp_file.name)
                except OSError as e:
                    if e.errno != errno.ENOENT:
                        raise

[ghost]

superseded by #488