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

Properly manage the lifetime of Exiters in Daemon Runs #7996

Merged

Conversation

@blorente
Copy link
Contributor

commented Jul 2, 2019

Problem

There is a bug where, once the LocalPantsRunner has reset the global exiter to its own LocalExiter, it won't un-set it ever, and therefore unhandled exceptions will use that exiter to exit.

This has caused issues, around this line, because the LocalPantsRunner reset the global exiter and the DaemonPantsRunner didn't reset it.

There is a regression test to demonstrate this issue.

Solution

  • Following the discussion on #7606, implement a contextmanager that temporarily overrides the global exiter.
  • Remove the concept of "my exiter", and make classes (LocalPantsRunner, DaemonPantsRunner, PantsRunner and PantsDaemon) use the global exiter.

Result

The regression test passes, and _log_unhandled_exceptions_and_exit works as expected in DaemonPantsRunner.

@blorente blorente requested review from stuhood, ity and cosmicexplorer Jul 2, 2019

Show resolved Hide resolved src/python/pants/base/exception_sink.py Outdated
Show resolved Hide resolved src/python/pants/base/exception_sink.py Outdated
Show resolved Hide resolved src/python/pants/bin/local_pants_runner.py Outdated
Show resolved Hide resolved src/python/pants/bin/local_pants_runner.py Outdated
@ity

ity approved these changes Jul 2, 2019

Copy link
Contributor

left a comment

I understand the time constraint with this review, so if you want to create a ticket and circle back with the cosmetic changes suggested, that would be completely fine!

Show resolved Hide resolved src/python/pants/bin/local_pants_runner.py Outdated

@blorente blorente force-pushed the blorente:blorente/fix-exiters-and-exceptions branch from dee32d8 to 005c2dc Jul 24, 2019

@blorente blorente force-pushed the blorente:blorente/fix-exiters-and-exceptions branch from 005c2dc to d9529f8 Jul 25, 2019

@blorente

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

I have made many changes to the PR, so I'd like a fresh review if possible.
Every commit is independently reviewable.

@blorente blorente requested review from ity, cosmicexplorer and wisechengyi Jul 25, 2019

@blorente blorente changed the title Don't use the global exiters in daemon runs Properly manage the lifetime of Exiters in Daemon Runs Jul 25, 2019

@stuhood
Copy link
Member

left a comment

Thanks a lot! This looks good.

Show resolved Hide resolved src/python/pants/base/exception_sink.py
@blorente

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2019

Will merge on green.

@blorente blorente merged commit adc9898 into pantsbuild:master Jul 28, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.