Use process.kill instead of process manager killPid

[gspencergoog]

Description

In my last change, I had switched to processManager.killPid(process.pid) to kill a process because it was easier to test, but that doesn't appear to actually work. It works fine in tests, which is why I didn't catch it, but it doesn't actually kill the process in the real app.

Seems like a bug in ProcessManager, but maybe there's something subtle happening here. In any case, this now kills processes properly.

Tests

• Removed the test for what processes were killed by the process manager, since it's no longer doing that. I don't see a good way to test if process.kill() was called...

[github-actions]

PR Health

Changelog Entry ✔️

Package	Changed Files

Changes to files need to be accounted for in their respective changelogs.

This check can be disabled by tagging the PR with skip-changelog-check.

[jakemac53]

@gspencergoog I looked at the implementation and it just calls Process.killPid from dart:io so that seems very odd indeed... https://github.com/dart-lang/tools/blob/main/pkgs/process/lib/src/interface/local_process_manager.dart#L128

[jakemac53]

Fwiw, are you sure flutter isn't expecting two sigterm signals in order to shut down?

[jakemac53]

You should be able to look for the mocked processes here and then check if they were killed

[gspencergoog]

I agree with all of that... except it still doesn't kill the app even with process.kill(), which is why I converted back to draft. It seems to kill only the flutter shell, and not the child processes.

[gspencergoog]

When I do a kill -HUP or kill -TERM on the flutter process (on Linux) from the command line, it is exiting, and it's taking down one child, but it leaves the dart develop command and the app running. It kills the flutter command (the main process) and the frontend_server_aot.dart.snapshot.

[gspencergoog]

It works just fine on macOS, even with using the process manager to kill it.

[gspencergoog]

Okay, this works, but I'm not sure if it's too much of a hack. It should work on all the Linux systems that Flutter supports.

[jakemac53]

flutter run also supports sending a q character to exit, maybe we should try just sending that?

[gspencergoog]

flutter run also supports sending a q character to exit, maybe we should try just sending that?

I thought about that, but it doesn't listen to stdin with --machine. And the DTD URI isn't printed in web without --machine (this is a temporary oversight, but currently the case).

[jakemac53]

https://github.com/flutter/flutter/blob/efc4d23cf0af636dd4997ad56d784a00ec206e0b/packages/flutter_tools/doc/daemon.md?plain=1#L13

Looks like it actually supports a whole protocol, so we can send a shutdown message?

We may actually want to consider running using flutter daemon in general here? Then we can use app.start and app.stop etc.

[gspencergoog]

We may actually want to consider running using flutter daemon in general here? Then we can use app.start and app.stop etc.

Oooh, yeah, that's probably the right call. Okay, I'll look at that.