Add unit test to verify fletch quit's graceful shutdown works.

[madsager]

We have both 'fletch shutdown' and 'fletch quit':

quit      Quits the fletch background process. Warning: will terminate all
          fletch sessions currently running

shutdown  Shut down the background Fletch process

It is unclear what the difference is and it seems that only quit actually works:

$ ./out/ReleaseIA32/fletch create session foo

$ ps aux | grep dart
ager      6840 30.5  0.2 702364 133928 ?       Ssl  10:06   0:01 /usr/local/google/home/ager/ssd/fletch-repo/fletch/out/ReleaseIA32/dart -c -Dfletch-vm=/usr/local/google/home/ager/ssd/fletch-repo/fletch/out/ReleaseIA32/fletch-vm --packages=/usr/local/google/home/ager/ssd/fletch-repo/fletch/pkg/fletchc/.packages -Dfletch.version=0.1.0-edge.81f381858b3af18d1493c914f1ad05244c1c2e18 -Dfletchc-library-root=../../third_party/dart/sdk -Dfletch-patch-root=../.. package:fletchc/src/driver/driver_main.dart /usr/local/google/home/ager/.fletch
ager      6861  0.0  0.0  23752   932 pts/4    S+   10:06   0:00 grep --color=auto dart

$ ./out/ReleaseIA32/fletch shutdown
Shutting down Fletch background process 6840: /usr/local/google/home/ager/ssd/fletch-repo/fletch/out/ReleaseIA32/dart -c -Dfletch-vm=/usr/local/google/home/ager/ssd/fletch-repo/fletch/out/ReleaseIA32/fletch-vm --packages=/usr/local/google/home/ager/ssd/fletch-repo/fletch/pkg/fletchc/.packages -Dfletch.version=0.1.0-edge.81f381858b3af18d1493c914f1ad05244c1c2e18 -Dfletchc-library-root=../../third_party/dart/sdk -Dfletch-patch-root=../.. package:fletchc/src/driver/driver_main.dart /usr/local/google/home/ager/.fletch

$ ps aux | grep dart                                                                                                                                                                                                
ager      6840 14.1  0.2 732576 136280 ?       Ssl  10:06   0:02 /usr/local/google/home/ager/ssd/fletch-repo/fletch/out/ReleaseIA32/dart -c -Dfletch-vm=/usr/local/google/home/ager/ssd/fletch-repo/fletch/out/ReleaseIA32/fletch-vm --packages=/usr/local/google/home/ager/ssd/fletch-repo/fletch/pkg/fletchc/.packages -Dfletch.version=0.1.0-edge.81f381858b3af18d1493c914f1ad05244c1c2e18 -Dfletchc-library-root=../../third_party/dart/sdk -Dfletch-patch-root=../.. package:fletchc/src/driver/driver_main.dart /usr/local/google/home/ager/.fletch
ager      6907  0.0  0.0  23756   932 pts/4    S+   10:06   0:00 grep --color=auto dart

$ ./out/ReleaseIA32/fletch create session foo

$ ps aux | grep dart
ager      6840 10.6  0.2 732576 136288 ?       Ssl  10:06   0:02 /usr/local/google/home/ager/ssd/fletch-repo/fletch/out/ReleaseIA32/dart -c -Dfletch-vm=/usr/local/google/home/ager/ssd/fletch-repo/fletch/out/ReleaseIA32/fletch-vm --packages=/usr/local/google/home/ager/ssd/fletch-repo/fletch/pkg/fletchc/.packages -Dfletch.version=0.1.0-edge.81f381858b3af18d1493c914f1ad05244c1c2e18 -Dfletchc-library-root=../../third_party/dart/sdk -Dfletch-patch-root=../.. package:fletchc/src/driver/driver_main.dart /usr/local/google/home/ager/.fletch
ager      6921 45.0  0.2 702364 135968 ?       Ssl  10:06   0:01 /usr/local/google/home/ager/ssd/fletch-repo/fletch/out/ReleaseIA32/dart -c -Dfletch-vm=/usr/local/google/home/ager/ssd/fletch-repo/fletch/out/ReleaseIA32/fletch-vm --packages=/usr/local/google/home/ager/ssd/fletch-repo/fletch/pkg/fletchc/.packages -Dfletch.version=0.1.0-edge.81f381858b3af18d1493c914f1ad05244c1c2e18 -Dfletchc-library-root=../../third_party/dart/sdk -Dfletch-patch-root=../.. package:fletchc/src/driver/driver_main.dart /usr/local/google/home/ager/.fletch
ager      6969  0.0  0.0  23756   932 pts/4    S+   10:06   0:00 grep --color=auto dart

$ ./out/ReleaseIA32/fletch shutdown
Shutting down Fletch background process 6921: /usr/local/google/home/ager/ssd/fletch-repo/fletch/out/ReleaseIA32/dart -c -Dfletch-vm=/usr/local/google/home/ager/ssd/fletch-repo/fletch/out/ReleaseIA32/fletch-vm --packages=/usr/local/google/home/ager/ssd/fletch-repo/fletch/pkg/fletchc/.packages -Dfletch.version=0.1.0-edge.81f381858b3af18d1493c914f1ad05244c1c2e18 -Dfletchc-library-root=../../third_party/dart/sdk -Dfletch-patch-root=../.. package:fletchc/src/driver/driver_main.dart /usr/local/google/home/ager/.fletch

$ ./out/ReleaseIA32/fletch create session foo

$ ps aux | grep dart
ager      6840  7.6  0.2 732576 136288 ?       Ssl  10:06   0:02 /usr/local/google/home/ager/ssd/fletch-repo/fletch/out/ReleaseIA32/dart -c -Dfletch-vm=/usr/local/google/home/ager/ssd/fletch-repo/fletch/out/ReleaseIA32/fletch-vm --packages=/usr/local/google/home/ager/ssd/fletch-repo/fletch/pkg/fletchc/.packages -Dfletch.version=0.1.0-edge.81f381858b3af18d1493c914f1ad05244c1c2e18 -Dfletchc-library-root=../../third_party/dart/sdk -Dfletch-patch-root=../.. package:fletchc/src/driver/driver_main.dart /usr/local/google/home/ager/.fletch
ager      6921 14.2  0.2 732576 138448 ?       Ssl  10:06   0:02 /usr/local/google/home/ager/ssd/fletch-repo/fletch/out/ReleaseIA32/dart -c -Dfletch-vm=/usr/local/google/home/ager/ssd/fletch-repo/fletch/out/ReleaseIA32/fletch-vm --packages=/usr/local/google/home/ager/ssd/fletch-repo/fletch/pkg/fletchc/.packages -Dfletch.version=0.1.0-edge.81f381858b3af18d1493c914f1ad05244c1c2e18 -Dfletchc-library-root=../../third_party/dart/sdk -Dfletch-patch-root=../.. package:fletchc/src/driver/driver_main.dart /usr/local/google/home/ager/.fletch
ager      7084 44.0  0.1 635800 129372 ?       Ssl  10:06   0:01 /usr/local/google/home/ager/ssd/fletch-repo/fletch/out/ReleaseIA32/dart -c -Dfletch-vm=/usr/local/google/home/ager/ssd/fletch-repo/fletch/out/ReleaseIA32/fletch-vm --packages=/usr/local/google/home/ager/ssd/fletch-repo/fletch/pkg/fletchc/.packages -Dfletch.version=0.1.0-edge.81f381858b3af18d1493c914f1ad05244c1c2e18 -Dfletchc-library-root=../../third_party/dart/sdk -Dfletch-patch-root=../.. package:fletchc/src/driver/driver_main.dart /usr/local/google/home/ager/.fletch
ager      7114  0.0  0.0  23756   932 pts/4    S+   10:06   0:00 grep --color=auto dart

$ ./out/ReleaseIA32/fletch quit
Background process exited

$ ps aux | grep dart
ager      7139  0.0  0.0  23752   932 pts/4    S+   10:06   0:00 grep --color=auto dart

Should we just get rid of fletch shutdown? If not, we should clearly document the differences between them.

@peter-ahe-google

[peter-ahe-google]

See my comment in https://github.com/dart-lang/fletch/issues/172#issuecomment-145214186.

I guess I forgot to remove shutdown. However, it is actually a bug if shutdown doesn't work as this is an indication that the process has a open ports. So I think the proper fix of this issue is:

• Remove fletch shutdown.
• Implement a unit test that ensures that graceful shutdown works.

I may need to split this up in two tasks if the latter takes me too long to think up a solution.

[wibling]

For start lets just remove fletch shutdown. I can take a stab at that. I will leave the bug open afterwards to track that a test is added.

[wibling]

I have removed the verb. Renaming this bug to be about adding a unit tests as described above.

[wibling]

Soren suggested to check this as part of shutting down the persistent process when running tests. Ie. make it a build bot step that checks the shutdown is graceful rather than a specific test.

[peter-ahe-google]

I have a couple of concerns about adding a build bot step:

1. It makes the build bot more complicated, and testing modifications to the buildbot is complicated (not something that most team members do regularly).
2. Build-bot-only steps are known to cause problems because people have a hard time testing them locally. You run all the tests locally, and only after code review and committing your changes do you realize that you made a mistake. Then people have a tendency to start submitting "speculative" fixes for build bot redness.
3. There's no status file to modify if this test needs to be disabled.

[ricowind]

If we made the driver capable of taking something similar to chrome/drt --user-data-dir then we could test all kind of stuff regarding the persistent process/driver shutdown sequence/failure scenarios (maybe this is already possible). If we made sure that the temporary directory for the data-dir is including the test name, then when somebody makes a wrong assumption in one test and we have a leftover hanging process you can always see exactly what test (since the data-dir is part of the command line that the kill step on the bots will show).

The only downside to the above is if we have a bug that actually takes down the main persistent process instead (the one started by the testing scripts/bots).

[peter-ahe-google]

The fletch binary looks for an environment variable named FLETCH_SOCKET_FILE.

[ricowind]

That should work as well, we then just need to cat /proc/PID/environ in the taskkill script to make sure that we know that file for a process that we kill (although, I would prefer a flag instead)

[peter-ahe-google]

@ricowind I don't understand why this relates to taskkill.