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

Fork to post_stat #4170

Merged
merged 6 commits into from Jan 7, 2017

Conversation

Projects
None yet
2 participants
@wisechengyi
Contributor

wisechengyi commented Jan 6, 2017

Problem

Earlier thread to post_stat may leave the pants process hanging from exiting until timeout. Although demonizing then joining the thread may help, post_stat procedure is already at the very end of the pants run, so there is not much time to save if we eventually want to wait for it to finish.

Solution

Fork a process to do the post.

@wisechengyi wisechengyi requested review from baroquebobcat, stuhood and mateor Jan 6, 2017

@wisechengyi wisechengyi referenced this pull request Jan 6, 2017

Merged

Make post_stat async #4157

@wisechengyi wisechengyi changed the title from Daemonize post_stat thread to Fork to post_stat Jan 7, 2017

@wisechengyi

This comment has been minimized.

Show comment
Hide comment
@wisechengyi

wisechengyi Jan 7, 2017

Contributor

changed the behavior to fork

Contributor

wisechengyi commented Jan 7, 2017

changed the behavior to fork

@wisechengyi wisechengyi requested a review from benjyw Jan 7, 2017

Show outdated Hide outdated src/python/pants/goal/run_tracker.py
t.start()
pid = os.fork()
if pid == 0:
self.post_stats(stats_url, stats, timeout=self.get_options().stats_upload_timeout)

This comment has been minimized.

@baroquebobcat

baroquebobcat Jan 7, 2017

Contributor

We'd probably want to do some kind of proper exit here, since the child would continue to execute after the fork. You could do something like the async clean-all patch did c06ac5d#diff-214714c04a4aa1e4879b7773ba163ae6R47

@baroquebobcat

baroquebobcat Jan 7, 2017

Contributor

We'd probably want to do some kind of proper exit here, since the child would continue to execute after the fork. You could do something like the async clean-all patch did c06ac5d#diff-214714c04a4aa1e4879b7773ba163ae6R47

This comment has been minimized.

@wisechengyi

wisechengyi Jan 7, 2017

Contributor

thanks! corrected

@wisechengyi

wisechengyi Jan 7, 2017

Contributor

thanks! corrected

This comment has been minimized.

@baroquebobcat

baroquebobcat Jan 7, 2017

Contributor

Probably should wrap with try...finally too.

@baroquebobcat

baroquebobcat Jan 7, 2017

Contributor

Probably should wrap with try...finally too.

This comment has been minimized.

@wisechengyi
@wisechengyi

This comment has been minimized.

@wisechengyi

wisechengyi Jan 7, 2017

Contributor

or is it a convention to wrap it no matter what's in that function?

@wisechengyi

wisechengyi Jan 7, 2017

Contributor

or is it a convention to wrap it no matter what's in that function?

This comment has been minimized.

@baroquebobcat

baroquebobcat Jan 7, 2017

Contributor

It's more that without the try finally, the child could potentially throw an exception that would trigger other behavior we don't want.

@baroquebobcat

baroquebobcat Jan 7, 2017

Contributor

It's more that without the try finally, the child could potentially throw an exception that would trigger other behavior we don't want.

@baroquebobcat

LGTM, modulo CI

@wisechengyi wisechengyi merged commit 10f0be3 into pantsbuild:master Jan 7, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@wisechengyi

This comment has been minimized.

Show comment
Hide comment
@wisechengyi

wisechengyi Jan 7, 2017

Contributor

thanks gents!

Contributor

wisechengyi commented Jan 7, 2017

thanks gents!

lenucksi added a commit to lenucksi/pants that referenced this pull request Apr 25, 2017

Fork to post_stat (#4170)
Problem:

Earlier thread to post_stat may leave the pants process hanging from exiting until timeout. Although demonizing then joining the thread may help, post_stat procedure is already at the very end of the pants run, so there is not much time to save if we eventually want to wait for it to finish.

Solution:
Fork a process to do the post.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment