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

Use CommandExt::exec for cargo run on Unix (#2343) #2818

Merged
merged 1 commit into from
Oct 20, 2016

Conversation

robinst
Copy link
Contributor

@robinst robinst commented Jul 1, 2016

Before, we would spawn a child process for the program. One of the
problems with that is when killing the cargo process, the program
continues running.

With this change, the cargo process is replaced by the program, and
killing it works.

Before (cargo run is the parent):

502 7615 7614 0 2:26PM ttys012 0:00.12 /Users/rstocker/.multirust/toolchains/stable-x86_64-apple-darwin/bin/cargo run
502 7620 7615 0 2:26PM ttys012 0:00.01 target/debug/program

After (the shell is the parent):

502 81649 81648 0 5:27PM ttys012 0:00.69 -zsh
502 7739 81649 0 2:26PM ttys012 0:01.27 target/debug/program

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@alexcrichton
Copy link
Member

Thanks for the PR @robinst! The implementation looks good to me.

cc @brson, do you have thoughts on this? This causes the behavior on Windows and Unix to diverge, but in a way that makes sense to me because this is arguably the "more correct" thing to do and allows native tools like gdb and such to work if you want to debug the target process. I also suspect that we may want rustup to start taking a similar tactic where it uses exec on Unix and the same behavior as today on Windows.

@brson
Copy link
Contributor

brson commented Jul 1, 2016

@alexcrichton I recall that at least in the rustup case we decided to move back to fork/exec for a reason more than consistency with windows ... telemetry. Can't report anything if your process doesn't exist.

The problem of killing cargo transitively killing rustc can be solved in some other way too right?

@alexcrichton
Copy link
Member

Ah right, forgot about the telemetry. In Cargo's case it can't report things like termination-via-signal on Unix where it can do more fancy reporting on Windows if we implement this.

It's true that transitively killing rustc is possible on Unix with process groups (you kill the entire process group, not just the Cargo process). It's somewhat of a pain to set up though and it also doesn't allow things like debuggers/profilers to work nicely, so I don't necessarily see process killing as the only motivation here.

I'm somewhat inclined to do this as this seems a little more low level than what rustup is doing, but it also would likely have far less utility unless rustup mirrored this strategy (as cargo run by default wouldn't exec to the final process if using rustup).

I guess there's no other way to collect telemetry in rustup?

@alexcrichton
Copy link
Member

ping @brson, although this is deviating Cargo and rustup, I feel like for Cargo this is still the right choice, how do you feel about that?

@brson
Copy link
Contributor

brson commented Jul 27, 2016

I'm kinda confused on the issues here. Doesn't killing cargo today kill the child too? I thought that was standard behavior - on windows isn't that why we have the explicit group setup - so that killing cargo kills rustc?

In Cargo's case it can't report things like termination-via-signal on Unix where it can do more fancy reporting on Windows if we implement this.

Why does this patch affect the reporting on Windows? It's a Unix-specific patch.

@alexcrichton
Copy link
Member

@brson on Unix if you kill Cargo's process group you will kill the child too, and if you want to kill Cargo then you should always do that as we're never, for example, going to exec the compiler. My main desire for this is to assist debuggers where you can just run gdb --args cargo run ... and it'll fall into the relevant program eventually.

Why does this patch affect the reporting on Windows? It's a Unix-specific patch.

Today, Cargo will print out whether the child abnormally terminated, e.g. a segfault. This happens for Windows as well. If, however, Cargo were to exec, then it wouldn't have a chance to print that out. This means that cargo run which ran a program which printed no output and segfaulted would be up to the programmer to figure out that the segfault happened.

@brson
Copy link
Contributor

brson commented Jul 28, 2016

Today, Cargo will print out whether the child abnormally terminated, e.g. a segfault. This happens for Windows as well. If, however, Cargo were to exec, then it wouldn't have a chance to print that out. This means that cargo run which ran a program which printed no output and segfaulted would be up to the programmer to figure out that the segfault happened.

On Unix. Are you just suggesting that if we made this change on Unix we would also remove the reporting behavior on Windows so they have feature-parity?

@alexcrichton
Copy link
Member

Oh sorry nah I was actually just pointing out how this will introduce an inconsistency between Unix/Windows. It's probably not worth it to actively avoid printing such information on Windows when we otherwise could.

@bors
Copy link
Contributor

bors commented Sep 26, 2016

☔ The latest upstream changes (presumably #3091) made this pull request unmergeable. Please resolve the merge conflicts.

@robinst
Copy link
Contributor Author

robinst commented Sep 28, 2016

Should I look into fixing the conflicts for this PR?

I probably should have explained why I created this PR: I was looking for nice way to have auto-reloading when developing server programs (e.g. an iron server). cargo watch run looks perfect for this, but it has a problem (see this issue). Without this PR, the following happens:

% cargo watch run
Waiting for changes... Hit Ctrl-C to stop.

$ cargo run
   Compiling foo-service v0.1.0
     Running `target/debug/foo-service`

# made changes to the code here, which are detected by cargo-watch

$ cargo run
   Compiling foo-service v0.1.0
     Running `target/debug/foo-service`
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Io(Error { repr: Os { code: 48, message: "Address already in use" } })', ../src/libcore/result.rs:788
note: Run with `RUST_BACKTRACE=1` for a backtrace.
error: Process didn't exit successfully: `target/debug/foo-service` (exit code: 101)
-> exit code: 101

What happens is that cargo-watch calls cargo run. It detects that files were changes, so it kills cargo run. But because cargo run uses a child process, the program still continues running. Then it invokes another cargo run, which fails because the port is already in use.

With this PR, it works nicely as expected.

So, what I currently use to get the auto reloading is entr (it manages to kill both cargo and the child process): find src Cargo.toml | entr -r cargo run

@alexcrichton
Copy link
Member

@robinst thanks for the ping! Sorry this PR has been a bit slow :(

In that specific case cargo-watch wouldn't actually want to benefit from a feature like this because it wouldn't be as robust as it could be. For example this still wouldn't work on Windows (e.g. cargo-watch would exhibit the same bad behavior there). Additionally, if the process itself spawned more children they also wouldn't be killed.

For a tool like cargo watch the processes it spawns on Unix want to be part of their own process group (e.g. setsid) and then when it kills a child it kills the process group to ensure that everything dies. For Windows it'd want to use job objects which have similar semantics to killing entire process trees.

Basically the tl;dr; is that I don't think this feature is appropriate for solving the cargo-watch problem, but I believe that this is still useful independently of that. For example it allows cargo run to work better by proxying signals to the main process. If you're developing a shell, this means that cargo run followed by ctrl-c would "do the right thing". I think this also helps debuggers by avoiding process spawns and instead sticking with the same pid.

The drawback is that while this functionality would work on Unix, none of it would work on Windows (due to the lack of exec). This means that you could have an inconsistent development experience across platforms, which is generally something we try to avoid where possible.

I'm still a bit on the fence about whether or not it's worth it here. I believe @brson felt that it may have not been, but I'm not sure whether I agree yet. So I guess to answer your question about whether this should be rebased or not, first off do you agree with what I mentioned about cargo-watch? Next, would you still be interested in rebasing for the other features this earns?

@robinst
Copy link
Contributor Author

robinst commented Oct 20, 2016

Apparently cargo watch already works as expected on Windows (haven't checked myself though): watchexec/cargo-watch#25 (comment)

I'll rebase this PR, and maybe look into doing the process group thing in cargo watch.

Also note that just recently the watchexec tool was released, and it has the same problem: watchexec/watchexec#16

@robinst robinst force-pushed the use-exec-for-cargo-run branch from 4f6b278 to f301522 Compare October 20, 2016 01:18
Before, we would spawn a child process for the program. One of the
problems with that is when killing the cargo process, the program
continues running.

With this change, the cargo process is replaced by the program, and
killing it works.
@robinst robinst force-pushed the use-exec-for-cargo-run branch from f301522 to ed5cea5 Compare October 20, 2016 02:08
@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Oct 20, 2016

📌 Commit ed5cea5 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Oct 20, 2016

⌛ Testing commit ed5cea5 with merge 9a0801d...

bors added a commit that referenced this pull request Oct 20, 2016
Use CommandExt::exec for `cargo run` on Unix (#2343)

Before, we would spawn a child process for the program. One of the
problems with that is when killing the cargo process, the program
continues running.

With this change, the cargo process is replaced by the program, and
killing it works.

Before (`cargo run` is the parent):

> 502  7615  7614   0  2:26PM ttys012    0:00.12 /Users/rstocker/.multirust/toolchains/stable-x86_64-apple-darwin/bin/cargo run
> 502  7620  7615   0  2:26PM ttys012    0:00.01 target/debug/program

After (the shell is the parent):

> 502 81649 81648   0  5:27PM ttys012    0:00.69 -zsh
> 502  7739 81649   0  2:26PM ttys012    0:01.27 target/debug/program
@alexcrichton alexcrichton added the relnotes Release-note worthy label Oct 20, 2016
@alexcrichton
Copy link
Member

Ok, I think this is worth it for the debugging and "least surprise" case, so r+'ing. Thanks again!

@bors
Copy link
Contributor

bors commented Oct 20, 2016

☀️ Test successful - cargo-cross-linux, cargo-linux-32, cargo-linux-64, cargo-mac-32, cargo-mac-64, cargo-win-gnu-32, cargo-win-gnu-64, cargo-win-msvc-32, cargo-win-msvc-64
Approved by: alexcrichton
Pushing 9a0801d to master...

@bors bors merged commit ed5cea5 into rust-lang:master Oct 20, 2016
@robinst robinst deleted the use-exec-for-cargo-run branch October 21, 2016 00:57
@robinst
Copy link
Contributor Author

robinst commented Oct 31, 2016

Thanks for merging this! Saw that the 2017 roadmap includes "Rust should have a pleasant edit-compile-debug cycle", and that is exactly what motivated this PR.

I now use watchexec like so for server development:

$ watchexec --run-initially --restart "cargo run"

It still takes 7 seconds to compile after I've made changes, but it's already better than manually restarting (and here's hoping that incremental compilation will bring down those 7 seconds in the future) :). Ideally I'd want cargo to provide the "automatically rerun after changes" functionality, but I'm not sure how that fits in with cargo's vision.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Release-note worthy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants