Microbenchmarks process.kill() does not work

(splitting this from investigation in #19096 into a more specific issue...)

In microbenchmarks we have some code that tries to kill the flutter run process after it gets its json results:

flutter/dev/devicelab/lib/tasks/microbenchmarks.dart

Line 128 in b931640

process.kill(ProcessSignal.SIGINT); // flutter run doesn't quit automatically

This does not work; nothing happens (for my Mac, and on some of the CIs, at least). My suspicion is that this is because the flutter executable is a shell script, so this process is the shell and not the VM. The shell doesn't seem to pass SIGINT on to the VM correctly - this could be the same issue I wrestled with in Dart Code for a while (I originally thought it was Windows-only).

Assuming it is that, the fix in Dart Code was to read the pid of the VM process via observatory and send kill to that too. It's also possible we can kill the whole set of processes with a negative ID, but I think this may require spawning it "detached" which may introduce other issues (on Windows I think it spawns a visible console window - not important in tests, but why I couldn't do it with Dart Code).

@jcollins-g

@jcollins-g Devon mentioned you might be handy with shell scripts; is this something you've seen before or might have ideas about? (using the VM's pid is probably a most foolproof way, but we might have to do some gymnastics to get it in the tests here).

TL;DR: Try adding an 'exec' at the beginning of this line and see if your problem just magically goes away:

flutter/bin/flutter

Line 172 in 127d1f8

"$DART" $FLUTTER_TOOL_ARGS "$SNAPSHOT_PATH" "$@"

There are three general approaches that can work to solve problems like this, assuming this is the simple case. (One shell script running one long-running executable, which when it exits also ends the shell script -- and you're not trying to kill off entire trees of fork/execed things which gets more complicated).

Use 'exec' in the shell script when running the real process (the dart binary at the end).
Make use of process groups and kill the process group containing the shell script and the executable. Programs can set their own process groups so this doesn't always work.
Read system process information and discover (like with pstree) the children of your process and kill them. This doesn't catch daemonization but will catch process group manipulation. Other techniques like asking the observatory fall into this bucket. A patchwork of methods here can eventually catch almost everything with the cost of having really complicated code just to kill a process..

IMO option 1 is the best option and Flutter should probably do that unless it breaks the world somehow, because I think using exec is what most shell-savvy people would expect it to do. It replaces the executing shell with what you're execing so there's no PID change.

A way in which it might break things is if there is a trap that must run after the dart process is complete (doesn't look like it -- the only trap defined is limited to a subshell) or if we've managed to introduce behavior that depends on the dart binary not dying when we kill the shell script...

Sadly the exec option looks like it won't work since we actually do need to do things after the execution of Dart if it exits with a particular code.

That seems suboptimal. I'd try really hard to avoid that in the flutter script, maybe by asking the VM folks if there is a way you can detect whether the snapshot is invalid without running it.

@devoncarew

TL;DR: Try adding an 'exec'
It replaces the executing shell with what you're execing so there's no PID change.

Interesting; I didn't know about that. Though I think this would leave the same issue on Windows and I'm not sure if there's an equiv there (though I haven't confirmed the same issue exists on Windows yet).

Having a way to validate the snapshot is valid (or even just do what that code does - re-generate it if it's not valid, then run it) doesn't seem like a bad idea to me though (cc @devoncarew - wdyt?)

Flutter does have a --pid-file switch that writes its pid to a file; so that could be an easy fix (similar to getting the pid from observatory) and would be isolated to the tests; however it does mean if there is other code (now or future) doing something similar, it would need to do something similar.

I might investigate the getting the pid via observatory/pid-file as they seem like the easiest fixes, and I might try testing the exec fix (by just removing that snapshot check temporarily) and see if something similar is possible on Windows as a more global fix.

@DanTup

@DanTup sounds like a reasonable approach. I thought this morning about adding some traps to forward signals from the shell to the binary, so that's another way of working around this. (The most direct path to me seems like the one you mentioned -- having the VM validate whether the snapshot is valid and rebuild it itself if necessary. It would be really nice if we could do that). I know almost nothing about Windows process management so I can't speculate on whether this is a problem there or how to solve it, though I'm interested to learn what you find.

@devoncarew

Having a way to validate the snapshot is valid (or even just do what that code does - re-generate it if it's not valid, then run it) doesn't seem like a bad idea to me though (cc @devoncarew - wdyt?)

I think the only current way is to try and execute it and check for a specific failure code. We'd need changes to snapshot execution to support anything more here; either something like a --check-snapshot flag, or having another arg to not actually run the snapshot if it turns out to be valid.

This would likely be generally useful for tools that execute snapshots, like pub. /cc @natebosch @grouma

@rmacnak-google

And, /cc @rmacnak-google and @a-siva for VM support for snapshot validity detection.

#!/bin/bash -e                                                                                                               
VM=$(which dart)
if [ $VM -nt hello.snapshot ]; then
   $VM --snapshot=hello.snapshot hello.dart
fi
exec $VM hello.snapshot $@

@rmacnak-google

@rmacnak-google That seems unlikely to work in the case where the dart executable is reverted to an older version.

Whichever tool is downloading the SDK should use the current time instead of the archive time, as Git does.

Whichever tool is downloading the SDK should use the current time instead of the archive time, as Git does.

AFAIK that's not the way apt works. brew does seem to use install time rather than archive time. Many folks also use manual approaches (such as dvm) which are most likely going to use the default arguments when extracting the archive which I believe is usually to use archive times.

I don't think we have scoping to move everyone to some team owned dart installer so I don't believe that solution is feasible.

Note that checking the timestamp on the dart binary also won't work for anyone with multiple SDKs switched by updating $PATH.

I think we should have tools in the cli dart vm to validate that a particular snapshot file is supported by that vm, esp. given that the snapshot format doesn't have compatibility guarantees between vm versions.

I think this might be the same request? dart-lang/sdk#20802

I'll have a quick look at whether we can fix with the pid or something for now since that issue has been around a while (almost 4 years); though it did have comments a few days ago.

In microbenchmarks, ensure flutter quits by sending `q` to stdin (#19400

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

Navigation Menu

Repository navigation

Microbenchmarks process.kill() does not work #19208

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Microbenchmarks process.kill() does not work #19208

Description

Activity

DanTup commented on Jul 10, 2018

jcollins-g commented on Jul 10, 2018

jcollins-g commented on Jul 10, 2018

DanTup commented on Jul 10, 2018

jcollins-g commented on Jul 11, 2018

devoncarew commented on Jul 11, 2018

devoncarew commented on Jul 11, 2018

rmacnak-google commented on Jul 11, 2018

jcollins-g commented on Jul 11, 2018

rmacnak-google commented on Jul 12, 2018

natebosch commented on Jul 12, 2018

natebosch commented on Jul 13, 2018

devoncarew commented on Jul 13, 2018

DanTup commented on Jul 16, 2018

github-actions commented on Sep 2, 2021

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions