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

Introduce Resettable #5770

Merged
merged 2 commits into from May 2, 2018

Conversation

Projects
None yet
3 participants
@illicitonion
Copy link
Contributor

illicitonion commented Apr 30, 2018

Because we fork without execing a lot, we often need to reset things
that have references to background threads. This provides a handy
wrapper for doing so.

Right now, our uses of grpcio::Environment aren't fork-safe. This fixes that.

If this looks good, I will also apply it to the
process_execution::remote::CommandRunner.

I can also apply it to the ResettablePool which we already have, for
re-use (it would add an additional Arc clone per CpuPool operation, but
this is probably fine because the CpuPool operations are already by
definition somewhat heavyweight).

@illicitonion illicitonion requested review from stuhood , ity and dotordogh Apr 30, 2018

Introduce Resettable
Because we fork without execing a lot, we often need to reset things
that have references to background threads. This provides a handy
wrapper for doing so.

If this looks good, I will also apply it to the
process_execution::remote::CommandRunner.

I can also apply it to the ResettablePool which we already have, for
re-use (it would add an additional Arc clone per CpuPool operation, but
this is probably fine because the CpuPool operations are already by
definition somewhat heavyweight).

@illicitonion illicitonion force-pushed the twitter:dwagnerhall/resettable branch from 70fae7a to f959c63 Apr 30, 2018

@ity

ity approved these changes May 1, 2018

Copy link
Member

ity left a comment

lgtm! I like this a lot - thank you for doing this.

Happy to apply this to CommandRunner if you like - feel free to create a ticket.

@illicitonion illicitonion merged commit 29bdaa6 into pantsbuild:master May 2, 2018

1 check passed

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

@illicitonion illicitonion deleted the twitter:dwagnerhall/resettable branch May 2, 2018

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented May 8, 2018

I think you could Box the Fn rather than Arc'ing it.

@illicitonion

This comment has been minimized.

Copy link
Contributor

illicitonion commented May 8, 2018

I think you could Box the Fn rather than Arc'ing it.

Sadly not; as far as I know, Box<Fn() -> T> can't be easily made Clone, which would mean Resettable couldn't be Clone.

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented May 8, 2018

Hm, ok. Then it sounds like Sized isn't the problem, per-se. Good news though, is that in 1.26, Fns with Clone captured variables will automatically be Clone! neat.

@illicitonion

This comment has been minimized.

Copy link
Contributor

illicitonion commented May 9, 2018

Yeah, it needs to be both Sized and Clone. Sounds like in 1.26 we'll be able to use Box instead, which will be nice! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment