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

[engine] strategy for interrupting engine execution to support user-driven interrupts #5285

Closed
kwlzn opened this issue Jan 6, 2018 · 6 comments

Comments

@kwlzn
Copy link
Member

kwlzn commented Jan 6, 2018

in order to support a saner fix for #5220, it seems like we'll want to have some way to gracefully interrupt an in-flight engine execution that can be called from a python signal handler in the daemon.

@kwlzn kwlzn added the engine label Jan 6, 2018
@kwlzn kwlzn added this to the 1.4.x milestone Jan 6, 2018
@kwlzn
Copy link
Member Author

kwlzn commented Jan 6, 2018

mod any potential concerns wrt partial request computation, on the surface this seems like approximately:

  1. switch from Future.wait() to Future.poll() in Scheduler::execute and check for some atomic flag/kill switch to short circuit (and drop the future to cancel the work?)
  2. plumb an api to set the kill switch
  3. call that from the signal handler in the daemon

@stuhood
Copy link
Sponsor Member

stuhood commented Jan 8, 2018

I think this sounds reasonable. One adjustment would be necessary:

Currently the main thread is the one actually doing the work by calling wait. Because rust futures are (mostly) "pull based", if we switched from wait to poll there, no thread would actually be working on the future. So I think that we'd additionally need to move the root futures into a CpuPool (or some other Future executor). Then those threads would drive them to completion, and rather than a future directly held by the Graph (a Shared future), Scheduler::execute would be holding a CpuFuture. Dropping it would probably cancel the work on the pool ... but this isn't precisely clear, because CpuPool might only be able to cancel before the work has started.

@stuhood
Copy link
Sponsor Member

stuhood commented Jan 10, 2018

Related: rust-lang/futures-rs#696

@stuhood stuhood modified the milestones: 1.4.x, 1.5.x Jan 12, 2018
@kwlzn kwlzn added this to the Daemon Post-Beta milestone Mar 1, 2018
@stuhood
Copy link
Sponsor Member

stuhood commented Aug 18, 2018

An update: it is no longer the case that the main thread is the one doing the work here. We've switched to launching all work on the tokio runtime, and so it should be the case that the main thread is able to wait in a loop and die for signals.

@stuhood stuhood self-assigned this Aug 18, 2018
@stuhood
Copy link
Sponsor Member

stuhood commented Aug 18, 2018

A first potential commit in this direction is here: master...twitter:stuhood/dont-block-the-main-thread

Will look into @kwlzn's suggestions about the kill switch on Monday.

@stuhood
Copy link
Sponsor Member

stuhood commented Nov 23, 2020

This has been fixed since we began respecting signals in the UI loop a while back.

@stuhood stuhood closed this as completed Nov 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants