Daemon App Domain #206

grouma · 2019-03-13T20:29:02Z

Depend on SSE v 2.0.0 as it fixes an issue with multiple listeners on the connection stream
- Note this requires me to recompile the client.js
Create DevWorkflow abstraction
- This handles all things related to web development, e.g. launching chrome
Update serve and daemon command to make use of DevWorkflow
Create AppDomain for the daemon command
- Note that this domain depends on the ServerManager
Move _WebdevClient out of DevHandler and into WebdevVmClient
- Note this will eventually be used heavily in the AppDomain.
Update DevHandler so that we can easily create a DebugService
- I might tweak this in the future
Make minor tweaks to the logging so that it is properly handled in the daemon command
Add corresponding tests for the new App Domain events

webdev/lib/src/command/daemon_command.dart

webdev/lib/src/daemon/app_domain.dart

jakemac53 · 2019-03-13T21:20:31Z

webdev/lib/src/daemon/app_domain.dart

+  }
+
+  Future<String> _restart(Map<String, dynamic> args) async {
+    throw UnimplementedError();


you could implement this as _callServiceExtension('hotRestart') now if you wanted in this PR (that is ultimately probably what it should be)

Gonna wait for a follow up PR as I'd like to add some tests.

webdev/lib/src/command/daemon_command.dart

webdev/lib/src/serve/controller.dart

webdev/lib/src/serve/debugger/webdev_vm_client.dart

webdev/lib/src/serve/handlers/dev_handler.dart

jakemac53 · 2019-03-13T21:46:22Z

webdev/test/daemon/utils.dart

+  webdev.stdin.add(utf8.encode('[{"method":"daemon.shutdown","id":0}]\n'));
+  // Try to shutdown webdev cleanly before killing it.
+  await webdev.exitCode.timeout(const Duration(seconds: 5), onTimeout: () {
+    webdev.kill();


Hmm putting a timeout like this in the test seems like it will just hide actual issues. Presumably intellij etc won't be killing us like this.

I suppose we can can listen for kill signals and try to gracefully close in webdev. The issue I'm trying to solve is that Chrome doesn't properly shutdown if webdev is killed. I can rework some stuff to see if there is a better pattern.

No luck. I've experienced this in the past. If you kill the parent process of Chrome it will live on sometimes. This simply prevents random Chrome instances from staying open after doing a local test run. It should not have any impact on the tests.

Presumably intellij etc won't be killing us like this.

FWIW, in VS Code we do something similar to this:

https://github.com/Dart-Code/Dart-Code/blob/master/src/debug/flutter_debug_impl.ts#L176-L177
https://github.com/Dart-Code/Dart-Code/blob/master/src/debug/dart_debug_impl.ts#L420-L423

To avoid having processes hang around, we:

Send a message using the API to stop/detach

Wait a seconds

Call .kill() (which sends SIGTERM)

Wait two seconds

Call .kill(sigkill)

Calling SigKill is bad, because it won't let the process clean up any of its child processes, but sometimes sigterm either doesn't work quickly and we don't know if the process will terminate, so we have to gamble with a sigkill.

That said - I do also wonder if doing it here might mask issues (for example, if shutdown doesn't work at all, won't this just silently kill it after 5 seconds and it could go unnoticed?).

If you kill the parent process of Chrome it will live on sometimes.

I had a similar issue with flutter_tester (see flutter/flutter#20949 (comment)) and I was never able to find conclusive information on what the mechanism is for child processes terminating when their parent does (I couldn't find whether the child was supposed to notice the parent die and terminate, or whether the parent should terminate the child, or whether the OS should do it, etc.).

In Dart-Code, we now have "additionalPidsToTerminate" in some places, where we just collect up the PIDs we can and terminate them all.

I would like to continue to push back on this - it seems as if sending the shutdown signal is not actually causing the daemon to shut down which isn't correct behavior.

If we are having issues actually shutting down webdev due to some streams being open or something then we should add a hard exit when we get the shutdown command, so that IDE's don't experience the same issue as the tests (and file an issue to diagnose the root cause).

If we want the test to clean itself up better after failures, then we could leave a timeout like this in but also throw an exception so the test will still fail and not silently kill webdev and succeed.

Thanks for the details!

We used to do something similar in package:test. Dartium was especially hard to kill. You could kill the main process of Dartium and its child processes would be promoted. Really weird. We ended up going with a hack where we would explicitly call kill on each child process of Dartium before killing the main process.

Normally when you hit ctrl+c from WebDev the launched Chrome instance is also killed. However if you kill WebDev right after Chrome launches (as in these test cases) then for some reason Chrome isn't properly "attached" and lives on. All other child processes of WebDev are correctly cleaned up. I'm also extremely confident that the build daemon is properly shutdown as I spent weeks ensuring that was the case. We also have tests for it as well.

Finally, I'm confident that WebDev properly shutsdown with the daemon exit command as we have a test for this functionality in daemon_domain_test.dart. It does not exercise this exitWebDev code path.

jakemac53 · 2019-03-14T13:47:34Z

webdev/lib/src/daemon/daemon.dart

@@ -19,10 +19,10 @@ class Daemon {
  Daemon(
    Stream<Map<String, dynamic>> commandStream,
    this._sendCommand,
-    Future<ServeController> futureServeController,
+    Future<ServerManager> futureServerManagers,


futureServerManager

jakemac53 · 2019-03-14T16:00:35Z

webdev/lib/src/serve/handlers/dev_handler.dart

-      chrome.chromeConnection,
+  Future<DebugService> startDebugService(
+      ChromeConnection chromeConnection, String appUrl) async {
+    return await DebugService.start(


nit: you don't need the await here

jakemac53 · 2019-03-14T16:10:54Z

webdev/test/daemon/utils.dart

+  webdev.stdin.add(utf8.encode('[{"method":"daemon.shutdown","id":0}]\n'));
+  // Try to shutdown webdev cleanly before killing it.
+  await webdev.exitCode.timeout(const Duration(seconds: 5), onTimeout: () {
+    webdev.kill();


I would like to continue to push back on this - it seems as if sending the shutdown signal is not actually causing the daemon to shut down which isn't correct behavior.

If we are having issues actually shutting down webdev due to some streams being open or something then we should add a hard exit when we get the shutdown command, so that IDE's don't experience the same issue as the tests (and file an issue to diagnose the root cause).

If we want the test to clean itself up better after failures, then we could leave a timeout like this in but also throw an exception so the test will still fail and not silently kill webdev and succeed.

grouma · 2019-03-14T16:16:32Z

With regards to #206 (comment) see my discussion here #206 (comment).

We do have a test that the WebDev process is properly killed and it does not exercise the exitWebDev code path. That code path simply ensures instances of chrome are not left around after doing a local run.

grouma added 2 commits March 13, 2019 12:50

Daemon App Domain

09a49e7

add tests

7221637

grouma requested a review from jakemac53 March 13, 2019 20:29

grouma changed the title ~~App start~~ Daemon App Domain Mar 13, 2019

fix lint

71225fe

jakemac53 reviewed Mar 13, 2019

View reviewed changes

webdev/lib/src/command/daemon_command.dart Outdated Show resolved Hide resolved

jakemac53 reviewed Mar 13, 2019

View reviewed changes

grouma added 12 commits March 13, 2019 15:26

Merge branch 'master' into app-start

a0c2d48

refactor futures

ed5908a

rename and more cleanup

c3776e0

break into methods

3635b89

fix tests

c1c1c42

more minor cleanup

0369df8

increase wait for slow travis

3927058

mark tests as webdriver reduce timeout

a7ea72e

maybe a longer timeout

6e4183b

reduce timeout and better filter

a46c5fd

debug logging

72c9513

this will fix it

53e69f5

jakemac53 reviewed Mar 14, 2019

View reviewed changes

tweak tests and explicitly handle shutdown

9894bf5

jakemac53 approved these changes Mar 14, 2019

View reviewed changes

fix test

b144c51

grouma merged commit 71880b3 into master Mar 14, 2019

grouma deleted the app-start branch March 14, 2019 18:58

grouma mentioned this pull request Mar 15, 2019

Enable Daemon mode #191

Closed

Daemon App Domain #206

Daemon App Domain #206

Uh oh!

Conversation

grouma commented Mar 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grouma commented Mar 14, 2019

Uh oh!

Uh oh!

grouma commented Mar 13, 2019 •

edited

Loading