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

Expand conditions for recognizing main process #252

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

DWesl
Copy link

@DWesl DWesl commented Aug 23, 2020

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.

Another option is to change it to

if getattr(self, "pid", None) is not None:

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.
@bluetech
Copy link
Member

Hi @DWesl,

Am I correct that this is a bug report by way of pytest-forked, which uses py.process? I'm asking because, given that the py library is deprecated, a better outcome would be to update pytest-forked and other projects to not use py.process. The usual process is to copy/paste the relevant py code, and simplify it for the given use case.

Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its certainly a acceptable bugfix
it should be accompanied by a test

Co-authored-by: Ronny Pfannschmidt <opensource@ronnypfannschmidt.de>
@DWesl
Copy link
Author

DWesl commented Aug 24, 2020

I think testing this would require mocking os.fork to raise an exception, which is not something I know how to do. I'll need a bit to research that.

This was uncovered while running pytest-forked. I'm preparing another PR for them and can move this file to pytest-forked as part of that.

Forgot to put the `not` back in last time.
Make sure fork fails, then check that the destructor does not raise an exception and cleans up after itself.
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.
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.
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

Successfully merging this pull request may close these issues.

None yet

3 participants