diff --git a/newsfragments/+inplace-setup.bugfix.rst b/newsfragments/+inplace-setup.bugfix.rst new file mode 100644 index 0000000..b7bcf9c --- /dev/null +++ b/newsfragments/+inplace-setup.bugfix.rst @@ -0,0 +1 @@ +Restore the original file and close opened streams when ``Path.in_place()`` fails during setup. diff --git a/path/__init__.py b/path/__init__.py index 731e88e..2053802 100644 --- a/path/__init__.py +++ b/path/__init__.py @@ -1750,43 +1750,42 @@ def in_place( backup_fn = self + (backup_extension or os.extsep + 'bak') backup_fn.remove_p() self.rename(backup_fn) - # closed explicitly below, after the caller's block exits - readable = open( # noqa: SIM115 - backup_fn, - mode, - buffering=buffering, - encoding=encoding, - errors=errors, - newline=newline, - ) - - perm = os.stat(readable.fileno()).st_mode - os_mode = os.O_CREAT | os.O_WRONLY | os.O_TRUNC - os_mode |= getattr(os, 'O_BINARY', 0) - fd = os.open(self, os_mode, perm) - writable = open( # noqa: SIM115 - fd, - "w" + mode.replace('r', ''), - buffering=buffering, - encoding=encoding, - errors=errors, - newline=newline, - ) - with contextlib.suppress(OSError, AttributeError): - self.chmod(perm) - try: - yield readable, writable + with contextlib.ExitStack() as streams: + readable = streams.enter_context( + open( + backup_fn, + mode, + buffering=buffering, + encoding=encoding, + errors=errors, + newline=newline, + ) + ) + perm = os.stat(readable.fileno()).st_mode + os_mode = os.O_CREAT | os.O_WRONLY | os.O_TRUNC + os_mode |= getattr(os, 'O_BINARY', 0) + fd = os.open(self, os_mode, perm) + streams.callback(os.close, fd) + writable = streams.enter_context( + open( + fd, + "w" + mode.replace('r', ''), + buffering=buffering, + encoding=encoding, + errors=errors, + newline=newline, + closefd=False, + ) + ) + with contextlib.suppress(OSError, AttributeError): + self.chmod(perm) + yield readable, writable except Exception: - # move backup back - readable.close() - writable.close() + # move backup back after closing any streams opened during setup self.remove_p() backup_fn.rename(self) raise - else: - readable.close() - writable.close() finally: backup_fn.remove_p() diff --git a/tests/test_path.py b/tests/test_path.py index 55d733c..76f569f 100644 --- a/tests/test_path.py +++ b/tests/test_path.py @@ -1229,6 +1229,51 @@ def test_exception_in_context(self, tmpdir): assert 'Lorem' not in data assert 'lazy dog' in data + @pytest.mark.parametrize( + ('options', 'error'), + [({'encoding': 'missing-codec'}, LookupError), ({'mode': 'q'}, ValueError)], + ) + def test_reader_setup_failure_restores_file(self, tmpdir, options, error): + doc = self.create_reference(tmpdir) + with pytest.raises(error), doc.in_place(**options): + pytest.fail('setup must fail before yielding') + assert doc.read_text(encoding='utf-8') == self.reference_content + assert not (doc + '.bak').exists() + + def test_writer_setup_failure_restores_file(self, tmpdir, monkeypatch): + doc = self.create_reference(tmpdir) + + def fail_open(*args, **kwargs): + raise PermissionError('cannot create replacement') + + monkeypatch.setattr(os, 'open', fail_open) + with pytest.raises(PermissionError), doc.in_place(encoding='utf-8'): + pytest.fail('setup must fail before yielding') + assert doc.read_text(encoding='utf-8') == self.reference_content + assert not (doc + '.bak').exists() + + def test_writer_wrapping_failure_closes_resources(self, tmpdir, monkeypatch): + doc = self.create_reference(tmpdir) + opened = [] + + def fail_writer(file, *args, **kwargs): + if isinstance(file, int): + opened.append(file) + raise OSError('cannot wrap replacement descriptor') + reader = open(file, *args, **kwargs) # noqa: SIM115 - caller owns stream + opened.append(reader) + return reader + + monkeypatch.setattr(path, 'open', fail_writer, raising=False) + with pytest.raises(OSError), doc.in_place(encoding='utf-8'): + pytest.fail('setup must fail before yielding') + reader, fd = opened + assert reader.closed + with pytest.raises(OSError): + os.fstat(fd) + assert doc.read_text(encoding='utf-8') == self.reference_content + assert not (doc + '.bak').exists() + def test_write_mode_invalid(self, tmpdir): with ( pytest.raises(ValueError),