From ce3e539b09e229acc56b3e5199e50ea374f5ec86 Mon Sep 17 00:00:00 2001 From: DWesl <22566757+DWesl@users.noreply.github.com> Date: Sun, 23 Aug 2020 18:38:12 -0400 Subject: [PATCH 1/6] Expand conditions for recognizing main process Fork failures are not uncommon on Cygwin, which causes an exception in the constructor. Unfortunately, cleaning up the partially-initialized instance calls the `__del__` method, which then raises another exception because `self.pid` never got set. This change should prevent the second exception. --- py/_process/forkedfunc.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/py/_process/forkedfunc.py b/py/_process/forkedfunc.py index 1c285306..fdd6fa77 100644 --- a/py/_process/forkedfunc.py +++ b/py/_process/forkedfunc.py @@ -107,7 +107,7 @@ def _removetemp(self): self.tempdir.remove() def __del__(self): - if self.pid is not None: # only clean up in main process + if not (hasattr(self, "pid") and self.pid is None): # only clean up in main process self._removetemp() From 3348f6178d63a785b77e7bb17917f4b31d098afe Mon Sep 17 00:00:00 2001 From: DWesl <22566757+DWesl@users.noreply.github.com> Date: Mon, 24 Aug 2020 06:05:35 -0400 Subject: [PATCH 2/6] Change test to be more concise. Co-authored-by: Ronny Pfannschmidt --- py/_process/forkedfunc.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/py/_process/forkedfunc.py b/py/_process/forkedfunc.py index fdd6fa77..93ea5621 100644 --- a/py/_process/forkedfunc.py +++ b/py/_process/forkedfunc.py @@ -107,7 +107,7 @@ def _removetemp(self): self.tempdir.remove() def __del__(self): - if not (hasattr(self, "pid") and self.pid is None): # only clean up in main process + if getattr(self, "pid", None) is None: # only clean up in main process self._removetemp() From 05343bf60fcc64fbe34a10a0fdcb488bade258ee Mon Sep 17 00:00:00 2001 From: DWesl <22566757+DWesl@users.noreply.github.com> Date: Mon, 24 Aug 2020 06:21:51 -0400 Subject: [PATCH 3/6] Correct the sense of the test comparison. Forgot to put the `not` back in last time. --- py/_process/forkedfunc.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/py/_process/forkedfunc.py b/py/_process/forkedfunc.py index 93ea5621..8072f4df 100644 --- a/py/_process/forkedfunc.py +++ b/py/_process/forkedfunc.py @@ -107,7 +107,7 @@ def _removetemp(self): self.tempdir.remove() def __del__(self): - if getattr(self, "pid", None) is None: # only clean up in main process + if getattr(self, "pid", None) is not None: # only clean up in main process self._removetemp() From 9cebab1f80327103b1a6eae828ce7caae5839dad Mon Sep 17 00:00:00 2001 From: DWesl <22566757+DWesl@users.noreply.github.com> Date: Mon, 24 Aug 2020 06:42:24 -0400 Subject: [PATCH 4/6] Test ForkedFunc behavior on fork failure Make sure fork fails, then check that the destructor does not raise an exception and cleans up after itself. --- testing/process/test_forkedfunc.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/testing/process/test_forkedfunc.py b/testing/process/test_forkedfunc.py index ae0d9ab7..17e2f9b7 100644 --- a/testing/process/test_forkedfunc.py +++ b/testing/process/test_forkedfunc.py @@ -16,6 +16,16 @@ def test_tempdir_gets_gc_collected(monkeypatch): assert ff.tempdir.check() ff.__del__() assert not ff.tempdir.check() + +def test_no_second_exception_if_fork_fails(monkeypatch): + def raise_oserror(): + # BlockingIOError on py3k + raise OSError("Resource temporarily unavailable") + monkeypatch.setattr(os, "fork", raise_oserror) + with pytest.raises(OSError, "Resource temporarily unavailable"): + ff = py.process.ForkedFunc(boxf1) + ff.__del__() + assert not ff.tempdir.check() def test_basic_forkedfunc(): result = py.process.ForkedFunc(boxf1).waitfinish() From e2dd0bb4abc9748e0fc21d5d720cdf59b9f8bf01 Mon Sep 17 00:00:00 2001 From: DWesl <22566757+DWesl@users.noreply.github.com> Date: Mon, 24 Aug 2020 07:15:20 -0400 Subject: [PATCH 5/6] Fix exceptions in new test I forgot the keyword for a keyword-only arg, then referenced a variable that would have been assigned after an exception was raised. Add the keyword. Remove variable that will never be assigned due to exception. --- testing/process/test_forkedfunc.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/testing/process/test_forkedfunc.py b/testing/process/test_forkedfunc.py index 17e2f9b7..f672dc4c 100644 --- a/testing/process/test_forkedfunc.py +++ b/testing/process/test_forkedfunc.py @@ -22,10 +22,10 @@ def raise_oserror(): # BlockingIOError on py3k raise OSError("Resource temporarily unavailable") monkeypatch.setattr(os, "fork", raise_oserror) - with pytest.raises(OSError, "Resource temporarily unavailable"): - ff = py.process.ForkedFunc(boxf1) - ff.__del__() - assert not ff.tempdir.check() + with pytest.raises(OSError, match="Resource temporarily unavailable"): + py.process.ForkedFunc(boxf1) + # The second exception would be raised while leaving the with block + # Not marking it should be equivalent to assert_does_not_raise def test_basic_forkedfunc(): result = py.process.ForkedFunc(boxf1).waitfinish() From a318d8243224d25dd7fe1d8179f5e3b97c44b4a4 Mon Sep 17 00:00:00 2001 From: DWesl <22566757+DWesl@users.noreply.github.com> Date: Mon, 24 Aug 2020 09:27:06 -0400 Subject: [PATCH 6/6] Add gc.collect to ensure destructor triggered I think refcounting semantics would trigger the deconstructor call while exiting the context manager in CPython, but this makes the test less dependent on CPython implementation details. --- testing/process/test_forkedfunc.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/testing/process/test_forkedfunc.py b/testing/process/test_forkedfunc.py index f672dc4c..686d2c10 100644 --- a/testing/process/test_forkedfunc.py +++ b/testing/process/test_forkedfunc.py @@ -1,3 +1,5 @@ +import gc + import pytest import py, sys, os @@ -24,8 +26,10 @@ def raise_oserror(): monkeypatch.setattr(os, "fork", raise_oserror) with pytest.raises(OSError, match="Resource temporarily unavailable"): py.process.ForkedFunc(boxf1) - # The second exception would be raised while leaving the with block - # Not marking it should be equivalent to assert_does_not_raise + # Make sure the ForkedFunc is collected + # That may be triggered by refcounting while exiting the with statement, + # but gc.collect() should work even on PyPy. + gc.collect() def test_basic_forkedfunc(): result = py.process.ForkedFunc(boxf1).waitfinish()