LauncherSpec: Check that all launched processes do exit #986
Conversation
rvl
force-pushed
the
rvl/703/windows-process-cleanup
branch
5 times, most recently
from
7ad8e91
to
313ce1f
Compare
- Fix backend process concurrency issue on Windows - Also get the LauncherSpec working on wine. It needs to detect Wine, then use commands which are available on wine. - LauncherSpec: Add an assertion that withCreateProcess does not block
rvl
force-pushed
the
rvl/703/windows-process-cleanup
branch
from
313ce1f
to
6a05496
Compare
| -> IO ExitCode | ||
| interruptibleWaitForProcess tr' ph = do | ||
| status <- newEmptyMVar | ||
| void $ forkIO $ waitThread status `onException` continue status |
There was a problem hiding this comment.
I think Duncan's variant used finally rather than onException here which has a slightly different meaning (finally will also execute the action in case of normal exits).
There was a problem hiding this comment.
This will likely be used only for starting the backend / server which should never actually exit normally, but we are creating a potential deadlock by doing this. Is it intentional @rvl ?
There was a problem hiding this comment.
I needed to change it to use onException because we are capturing exit status. Otherwise it would try to putMVar twice
There was a problem hiding this comment.
Hmmmm. I see okay, there's actually a putMVar in waitThread itself which I guess is safe (provided the launcherLog doesn't throw. But I am ready to accept anything at this stage).
There was a problem hiding this comment.
I think that because the putMVar is the last action in the block, either way there will be something put.
| setupTrace_ cfg "tests" | ||
| after (tr, sb) = do | ||
| logDebug tr "Logging shutdown." | ||
| shutdown sb |
| after <- getCurrentTime | ||
| ph <- takeMVar mvar | ||
| assertProcessesExited [ph] | ||
| diffUTCTime after before `shouldSatisfy` (< 2) |
There was a problem hiding this comment.
It is not obvious that this 2 (seconds) actually refers to 1000000 (microseconds) above with a safe margin (which is what I assume).
There was a problem hiding this comment.
True, fixed with a comment.
|
bors r+ |
986: LauncherSpec: Check that all launched processes do exit r=KtorZ a=rvl Relates to #703. # Overview It appears that the `jormungandr` node backend process was not getting cleaned up on Windows and that actions which are supposed to run concurrently with the backend were not running. These extra tests check that the [`withCreateProcess`](http://hackage.haskell.org/package/process-1.6.6.0/docs/System-Process.html#v:withCreateProcess) function takes care of terminating the process -- if it is given a chance (i.e. if the process is not killed with -9, or "End Task" on Windows). It also checks that the concurrent actions run while the backend process is running, and that the backend process is terminated when the other action completes. Finally, it implements a workaround for the unwanted behaviour of the `process` library on Windows where `waitForProcess` seems to block all concurrent async actions in the thread. - [x] Adds a test to `cardano-wallet-launcher:test:unit` for process clean up. - [x] Adjusts commands used so that the tests can be run under Wine. - [x] Adds an assertion to check that the process is killed if the action does not complete. - [x] Adds an assertion to check that the the process is killed promptly if the action completes. - [x] Fixes async blocking issue on Windows. ### Testing under Wine Use something like this: ``` wine $(nix-build release.nix -A x86_64-pc-mingw32.tests.cardano-wallet-launcher.unit.x86_64-linux -o launcher-unit-windows)/cardano-wallet-launcher-2019.11.7/unit.exe --match "Backend process" ``` Co-authored-by: Rodney Lorrimar <[email protected]>
Build succeeded |
Relates to #703.
Overview
It appears that the
jormungandrnode backend process was not getting cleaned up on Windows and that actions which are supposed to run concurrently with the backend were not running.These extra tests check that the
withCreateProcessfunction takes care of terminating the process -- if it is given a chance (i.e. if the process is not killed with -9, or "End Task" on Windows).It also checks that the concurrent actions run while the backend process is running, and that the backend process is terminated when the other action completes.
Finally, it implements a workaround for the unwanted behaviour of the
processlibrary on Windows wherewaitForProcessseems to block all concurrent async actions in the thread.cardano-wallet-launcher:test:unitfor process clean up.Testing under Wine
Use something like this: