Skip to content

Avoid os.fork() prior to stats upload.#4919

Merged
kwlzn merged 1 commit into
pantsbuild:masterfrom
twitter:kwlzn/what_the_fork
Sep 30, 2017
Merged

Avoid os.fork() prior to stats upload.#4919
kwlzn merged 1 commit into
pantsbuild:masterfrom
twitter:kwlzn/what_the_fork

Conversation

@kwlzn

@kwlzn kwlzn commented Sep 30, 2017

Copy link
Copy Markdown
Member

Problem

Presently, we do an os.fork() just before posting build stats in order to reduce the time-until-timeout for users who aren't connected to the VPN (and thus can't reach the stats server).

For users on OSX 10.11 (and potentially others), this seems to have been causing a silent segmentation fault in the forked process due to https://bugs.python.org/issue13829 .

With the addition of faulthandler support to pants, we now see a full traceback to stderr when this segmentation fault happens in the forked process due to retaining handles to the original processes stdio descriptors. At first this appeared to be a new issue encountered during release handling, but after investigating it seems like it's just faulthandler's improved logging revealing a long-standing masked segfault.

Solution

Remove the os.fork() call prior to stats upload and revisit.

Result

Can no longer reproduce the segmentation fault.

@stuhood stuhood left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks Kris.

Maybe (re)open an issue about making stats posting async? As mentioned offline, doing something with a persistent queue or the sqlite db we already have, would be good.

@kwlzn

kwlzn commented Sep 30, 2017

Copy link
Copy Markdown
Member Author

sure, created #4920

@kwlzn kwlzn merged commit 9a6b204 into pantsbuild:master Sep 30, 2017
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.

2 participants