Shutdown scripts throw an exception in Windows

[mjordan]

Running shutdown scripts on Windows produces the following type of error:

Fatal error: Uncaught exception 'RuntimeException' with message 'Cocur\BackgroundProcess can only check if a process is running on *nix-
based systems, such as Unix, Linux or Mac OS X. You are running "WINNT".' in M:\mik_mark\vendor\cocur\background-process\src\BackgroundP
rocess.php:162
Stack trace:
#0 M:\mik_mark\vendor\cocur\background-process\src\BackgroundProcess.php(89): Cocur\BackgroundProcess\BackgroundProcess->checkSupporting
OS('Cocur\\Backgroun...')
#1 M:\mik_mark\mik(330): Cocur\BackgroundProcess\BackgroundProcess->isRunning()
#2 {main}
  thrown in M:\mik_mark\vendor\cocur\background-process\src\BackgroundProcess.php on line 162

The script ends up not running. This is an known issue with the Cocur library we are using.

This makes me wonder if we need to run shutdown scripts as background processes. It makes a lot of sense to run post-write hooks in the background so they don't slow down moving on to the next package, but that same logic doesn't apply to shutdown hooks. @jpeak5 and @MarcusBarnes, any thoughts? We can also build in logic such that if mik is running on Windows, it doesn't use Cocur, it just runs the scripts in the foreground. On Linux and OSX, it runs them in the background.

[jpeak5]

That sounds good to me!

[mjordan]

Looking back over the background to shutdown scripts, (e.g. #156 and #354) I see no rationale for having them run in the background other than a casual mention by me in #156:

One way of implementing this would be using register_shutdown_function(), having the hook scripts run as background processes.

It may actually be misleading to to have long-running shutdown scripts run in the background since the user would not see a clear indication that it had completed.

I just ran a version of the create_structure_files.php script as a foreground job and it worked fine.

So, can we think of any reason to not run shutdown scripts only as foreground jobs, or is there any reason we should build in OS-specific logic to do this only on Windows?

[jpeak5]

I favor foreground so that, as you say, the user will know when they have completed.

Sent from my iPhone

On Jun 1, 2017, at 1:38 PM, Mark Jordan notifications@github.com<mailto:notifications@github.com> wrote:

Looking back over the background to shutdown scripts, (e.g. #156https://github.com/MarcusBarnes/mik/issues/156 and #354https://github.com/MarcusBarnes/mik/issues/354) I see no rationale for having them run in the background other than a casual mention by me in #156https://github.com/MarcusBarnes/mik/issues/156:

One way of implementing this would be using register_shutdown_function(), having the hook scripts run as background processes.

It may actually be misleading to to have long-running shutdown scripts run in the background since the user would not see a clear indication that it had completed.

I just ran a version of the create_structure_files.php script as a foreground job and it worked fine.

So, can we think of any reason to not run shutdown scripts only as foreground jobs, or is there any reason we should build in OS-specific logic to do this only on Windows?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/MarcusBarnes/mik/issues/392#issuecomment-305566163, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ABEMrKGyPlQVsPE3klib-s9q8KREun9mks5r_vcogaJpZM4NsNev.

[mjordan]

Thanks for the feedback @jpeak5. I'll wait for @MarcusBarnes to weigh in before opening a PR.

[MarcusBarnes]

A second @jpeak5's take. Thank you.

[mjordan]

OK, sounds like we have consensus. I'm happy to open a PR for evaluation.

[MarcusBarnes]

Addressed in pull-request https://github.com/MarcusBarnes/mik/pull/396 (merged with commit https://github.com/MarcusBarnes/mik/commit/9d5eae6516610a3346c00c8d3be91d928ddcb3e8).

[mjordan]

Thanks @MarcusBarnes, I'll update https://github.com/MarcusBarnes/mik/wiki/Shutdown-hooks to reflect this change.