Fix remaining issues (#60)

* Update tests to Aff * Add stdin write test * Fix exit handler's string value * Update module import to include 'node:' * Fix path of stdin test * Add missing FFI * Drop `ipc` on sync fns (error); inline safeStdio * Fix `fromKillSignalImpl` FFI arg order * Update tests * Add changelog entry
purescript-node · Jul 26, 2023 · 8e031fe · 8e031fe
1 parent 4d27b66
commit 8e031fe
Show file tree

Hide file tree

Showing 7 changed files with 144 additions and 75 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -12,6 +12,22 @@ Bugfixes:
 
 Other improvements:
 
+## [v12.0.0](https://github.com/purescript-node/purescript-node-child-process/releases/tag/v12.0.0) - 2023-07-26
+
+Breaking changes:
+- Removed `safeStdio` (#60 by @JordanMartinez)
+
+  Turns out this isn't safe for `*Sync` functions. AFAIK, this isn't documented
+  in Node docs.
+
+Bugfixes:
+- Fixed `exitH`'s String value for listener (#60 by @JordanMartinez)
+- Added missing FFI for `execSync'` (#60 by @JordanMartinez)
+- Fixed `fromKillSignal`'s FFI's arg order (#60 by @JordanMartinez)
+
+Other improvements:
+- Update tests to actually throw if invalid state occurs (#60 by @JordanMartinez)
+
 ## [v11.0.0](https://github.com/purescript-node/purescript-node-child-process/releases/tag/v11.0.0) - 2023-07-25
 
 Breaking changes:

diff --git a/src/Node/ChildProcess.purs b/src/Node/ChildProcess.purs
@@ -86,7 +86,7 @@ module Node.ChildProcess
 
 import Prelude
 
-import Data.Maybe (Maybe(..), fromMaybe, maybe)
+import Data.Maybe (Maybe(..), fromMaybe)
 import Data.Nullable (Nullable, toMaybe, toNullable)
 import Data.Posix (Pid, Gid, Uid)
 import Data.Posix.Signal (Signal)
@@ -97,12 +97,11 @@ import Effect.Uncurried (EffectFn2)
 import Foreign (Foreign)
 import Foreign.Object (Object)
 import Node.Buffer (Buffer)
-import Node.ChildProcess.Types (Exit(..), Handle, KillSignal, Shell, StdIO, UnsafeChildProcess)
+import Node.ChildProcess.Types (Exit(..), Handle, KillSignal, Shell, StdIO, UnsafeChildProcess, ipc, pipe)
 import Node.Errors.SystemError (SystemError)
 import Node.EventEmitter (EventEmitter, EventHandle)
 import Node.EventEmitter.UtilTypes (EventHandle0, EventHandle1)
 import Node.Stream (Readable, Writable)
-import Node.UnsafeChildProcess.Safe (safeStdio)
 import Node.UnsafeChildProcess.Safe as SafeCP
 import Node.UnsafeChildProcess.Unsafe (unsafeSOBToBuffer)
 import Node.UnsafeChildProcess.Unsafe as UnsafeCP
@@ -288,7 +287,7 @@ spawnSync' command args buildOpts = (UnsafeCP.spawnSync' command args opts) <#>
   }
   where
   opts =
-    { stdio: maybe safeStdio (\rest -> safeStdio <> rest) o.appendStdio
+    { stdio: [ pipe, pipe, pipe ] <> fromMaybe [] o.appendStdio
     , encoding: "buffer"
     , cwd: fromMaybe undefined o.cwd
     , input: fromMaybe undefined o.input
@@ -328,7 +327,7 @@ spawn
   :: String
   -> Array String
   -> Effect ChildProcess
-spawn cmd args = coerce $ UnsafeCP.spawn' cmd args { stdio: safeStdio }
+spawn cmd args = coerce $ UnsafeCP.spawn cmd args
 
 -- | - `cwd` <string> | <URL> Current working directory of the child process.
 -- | - `env` <Object> Environment key-value pairs. Default: process.env.
@@ -367,7 +366,7 @@ spawn'
 spawn' cmd args buildOpts = coerce $ UnsafeCP.spawn' cmd args opts
   where
   opts =
-    { stdio: maybe safeStdio (\rest -> safeStdio <> rest) o.appendStdio
+    { stdio: [ pipe, pipe, pipe, ipc ] <> fromMaybe [] o.appendStdio
     , cwd: fromMaybe undefined o.cwd
     , env: fromMaybe undefined o.env
     , argv0: fromMaybe undefined o.argv0
@@ -452,7 +451,7 @@ execSync' cmd buildOpts = do
     , windowsHide: Nothing
     }
   opts =
-    { stdio: maybe safeStdio (\rest -> safeStdio <> rest) o.appendStdio
+    { stdio: [ pipe, pipe, pipe ] <> fromMaybe [] o.appendStdio
     , encoding: "buffer"
     , cwd: fromMaybe undefined o.cwd
     , input: fromMaybe undefined o.input
@@ -550,7 +549,7 @@ execFileSync
   -> Array String
   -> Effect Buffer
 execFileSync file args =
-  map unsafeSOBToBuffer $ UnsafeCP.execFileSync' file args { stdio: safeStdio, encoding: "buffer" }
+  map unsafeSOBToBuffer $ UnsafeCP.execFileSync' file args { encoding: "buffer" }
 
 -- | - `cwd` <string> | <URL> Current working directory of the child process.
 -- | - `input` <string> | <Buffer> | <TypedArray> | <DataView> The value which will be passed as stdin to the spawned process. Supplying this value will override stdio[0].
@@ -585,7 +584,7 @@ execFileSync' file args buildOpts =
   map unsafeSOBToBuffer $ UnsafeCP.execFileSync' file args opts
   where
   opts =
-    { stdio: maybe safeStdio (\rest -> safeStdio <> rest) o.appendStdio
+    { stdio: [ pipe, pipe, pipe ] <> fromMaybe [] o.appendStdio
     , encoding: "buffer"
     , cwd: fromMaybe undefined o.cwd
     , input: fromMaybe undefined o.input
@@ -685,7 +684,7 @@ fork
   :: String
   -> Array String
   -> Effect ChildProcess
-fork modulePath args = coerce $ UnsafeCP.fork' modulePath args { stdio: safeStdio }
+fork modulePath args = coerce $ UnsafeCP.fork' modulePath args { stdio: [ pipe, pipe, pipe, ipc ] }
 
 -- | - `cwd` <string> | <URL> Current working directory of the child process.
 -- | - `detached` <boolean> Prepare child to run independently of its parent process. Specific behavior depends on the platform, see options.detached).
@@ -724,7 +723,7 @@ fork'
 fork' modulePath args buildOpts = coerce $ UnsafeCP.fork' modulePath args opts
   where
   opts =
-    { stdio: maybe safeStdio (\rest -> safeStdio <> rest) o.appendStdio
+    { stdio: [ pipe, pipe, pipe, ipc ] <> fromMaybe [] o.appendStdio
     , cwd: fromMaybe undefined o.cwd
     , detached: fromMaybe undefined o.detached
     , env: fromMaybe undefined o.env

diff --git a/src/Node/ChildProcess/Types.js b/src/Node/ChildProcess/Types.js
@@ -1,8 +1,8 @@
 export const showKillSignal = (ks) => ks + "";
 export const showShell = (shell) => shell + "";
-export const fromKillSignalImpl = (left, right, sig) => {
+export const fromKillSignalImpl = (fromInt, fromStr, sig) => {
   const ty = typeof sig;
-  if (ty === "number") return right(sig | 0);
-  if (ty === "string") return left(sig);
+  if (ty === "number") return fromInt(sig | 0);
+  if (ty === "string") return fromStr(sig);
   throw new Error("Impossible. Got kill signal that was neither int nor string: " + sig);
 };
diff --git a/src/Node/UnsafeChildProcess/Safe.purs b/src/Node/UnsafeChildProcess/Safe.purs
@@ -24,7 +24,6 @@ module Node.UnsafeChildProcess.Safe
   , spawnFile
   , spawnArgs
   , stdio
-  , safeStdio
   ) where
 
 import Prelude
@@ -37,7 +36,7 @@ import Data.Posix.Signal as Signal
 import Effect (Effect)
 import Effect.Uncurried (EffectFn1, EffectFn2, mkEffectFn1, mkEffectFn2, runEffectFn1, runEffectFn2)
 import Foreign (Foreign)
-import Node.ChildProcess.Types (Exit(..), Handle, KillSignal, StdIO, UnsafeChildProcess, intSignal, ipc, pipe, stringSignal)
+import Node.ChildProcess.Types (Exit(..), Handle, KillSignal, StdIO, UnsafeChildProcess, intSignal, stringSignal)
 import Node.Errors.SystemError (SystemError)
 import Node.EventEmitter (EventEmitter, EventHandle(..))
 import Node.EventEmitter.UtilTypes (EventHandle0, EventHandle1)
@@ -61,7 +60,7 @@ errorH :: EventHandle1 UnsafeChildProcess SystemError
 errorH = EventHandle "error" mkEffectFn1
 
 exitH :: EventHandle UnsafeChildProcess (Exit -> Effect Unit) (EffectFn2 (Nullable Int) (Nullable KillSignal) Unit)
-exitH = EventHandle "exitH" \cb -> mkEffectFn2 \code signal ->
+exitH = EventHandle "exit" \cb -> mkEffectFn2 \code signal ->
   case toMaybe code, toMaybe signal of
     Just c, _ -> cb $ Normally c
     _, Just s -> cb $ BySignal s
@@ -149,10 +148,3 @@ foreign import spawnArgs :: UnsafeChildProcess -> Array String
 foreign import spawnFile :: UnsafeChildProcess -> String
 
 foreign import stdio :: UnsafeChildProcess -> Array StdIO
-
--- | Safe default configuration for an UnsafeChildProcess.
--- | `[ pipe, pipe, pipe, ipc ]`.
--- | Creates a new stream for `stdin`, `stdout`, and `stderr`
--- | Also adds an IPC channel, even if it's not used.
-safeStdio :: Array StdIO
-safeStdio = [ pipe, pipe, pipe, ipc ]
diff --git a/src/Node/UnsafeChildProcess/Unsafe.js b/src/Node/UnsafeChildProcess/Unsafe.js
@@ -10,13 +10,14 @@ export {
   spawn as spawnImpl, 
   spawn as spawnOptsImpl, 
   execSync as execSyncImpl,
+  execSync as execSyncOptsImpl,
   execFileSync as execFileSyncImpl,
   execFileSync as execFileSyncOptsImpl,
   spawnSync as spawnSyncImpl,
   spawnSync as spawnSyncOptsImpl,
   fork as forkImpl,
   fork as forkOptsImpl,
-} from "child_process";
+} from "node:child_process";
 
 export const unsafeStdin = (cp) => cp.stdin;
 export const unsafeStdout = (cp) => cp.stdout;

diff --git a/test/Main.purs b/test/Main.purs
@@ -2,76 +2,131 @@ module Test.Main where
 
 import Prelude
 
-import Data.Either (hush)
+import Data.Either (Either(..), hush)
 import Data.Maybe (Maybe(..))
 import Data.Posix.Signal (Signal(..))
 import Data.Posix.Signal as Signal
 import Effect (Effect)
-import Effect.Console (log)
+import Effect.Aff (Aff, effectCanceler, launchAff_, makeAff, nonCanceler)
+import Effect.Class (liftEffect)
+import Effect.Class.Console (log)
+import Effect.Exception (throw, throwException)
 import Node.Buffer as Buffer
-import Node.ChildProcess (errorH, exec', execSync', exitH, kill, spawn, stdout)
+import Node.ChildProcess (exec', execSync', kill, spawn, stdin)
+import Node.ChildProcess as CP
+import Node.ChildProcess.Aff (waitSpawned)
 import Node.ChildProcess.Types (Exit(..), fromKillSignal)
 import Node.Encoding (Encoding(..))
 import Node.Encoding as NE
-import Node.Errors.SystemError (code)
-import Node.EventEmitter (on_)
-import Node.Stream (dataH)
+import Node.EventEmitter (EventHandle, once, once_)
+import Node.Stream as Stream
+import Unsafe.Coerce (unsafeCoerce)
 
 main :: Effect Unit
-main = do
-  log "spawns processes ok"
+main = launchAff_ do
+  writingToStdinWorks
   spawnLs
+  nonExistentExecutable
+  noEffectsTooEarly
+  killsProcess
+  execLs
+  execSyncEcho "some value"
 
-  log "emits an error if executable does not exist"
-  nonExistentExecutable $ do
-    log "nonexistent executable: all good."
+until
+  :: forall emitter psCb jsCb a
+   . emitter
+  -> EventHandle emitter psCb jsCb
+  -> ((a -> Effect Unit) -> psCb)
+  -> Aff a
+until ee event cb = makeAff \done -> do
+  rm <- ee # once event (cb (done <<< Right))
+  pure $ effectCanceler rm
 
-  log "doesn't perform effects too early"
-  spawn "ls" [ "-la" ] >>= \ls -> do
-    let _ = kill ls
-    ls # on_ exitH \exit ->
-      case exit of
-        Normally 0 ->
-          log "All good!"
-        _ -> do
-          log ("Bad exit: expected `Normally 0`, got: " <> show exit)
+writingToStdinWorks :: Aff Unit
+writingToStdinWorks = do
+  log "\nwriting to stdin works"
+  sp <- liftEffect $ spawn "sh" [ "./test/sleep.sh" ]
+  liftEffect do
+    (stdin sp) # once_ Stream.errorH \err -> do
+      log "Error in stdin"
+      throwException $ unsafeCoerce err
+    buf <- Buffer.fromString "helllo" UTF8
+    void $ Stream.write (stdin sp) buf
+    sp # once_ CP.errorH \err -> do
+      log "Error in child process"
+      throwException $ unsafeCoerce err
+  exit <- until sp CP.closeH \completeAff -> \exit ->
+    completeAff exit
+  log $ "spawn sleep done " <> show exit
 
-  log "kills processes"
-  spawn "ls" [ "-la" ] >>= \ls -> do
-    _ <- kill ls
-    ls # on_ exitH \exit ->
-      case exit of
-        BySignal s | Just SIGTERM <- Signal.fromString =<< (hush $ fromKillSignal s) ->
-          log "All good!"
-        _ -> do
-          log ("Bad exit: expected `BySignal SIGTERM`, got: " <> show exit)
+spawnLs :: Aff Unit
+spawnLs = do
+  log "\nspawns processes ok"
+  ls <- liftEffect $ spawn "ls" [ "-la" ]
+  res <- waitSpawned ls
+  case res of
+    Right pid -> log $ "ls successfully spawned with PID: " <> show pid
+    Left err -> liftEffect $ throwException $ unsafeCoerce err
+  exit <- until ls CP.closeH \complete -> \exit -> complete exit
+  case exit of
+    Normally 0 -> log $ "ls exited with 0"
+    Normally i -> liftEffect $ throw $ "ls had non-zero exit: " <> show i
+    BySignal sig -> liftEffect $ throw $ "ls exited with sig: " <> show sig
 
-  log "exec"
-  execLs
+nonExistentExecutable :: Aff Unit
+nonExistentExecutable = do
+  log "\nemits an error if executable does not exist"
+  ch <- liftEffect $ spawn "this-does-not-exist" []
+  res <- waitSpawned ch
+  case res of
+    Left _ -> log "nonexistent executable: all good."
+    Right pid -> liftEffect $ throw $ "nonexistent executable started with PID: " <> show pid
 
-spawnLs :: Effect Unit
-spawnLs = do
-  ls <- spawn "ls" [ "-la" ]
-  ls # on_ exitH \exit ->
-    log $ "ls exited: " <> show exit
-  (stdout ls) # on_ dataH (Buffer.toString UTF8 >=> log)
+noEffectsTooEarly :: Aff Unit
+noEffectsTooEarly = do
+  log "\ndoesn't perform effects too early"
+  ls <- liftEffect $ spawn "ls" [ "-la" ]
+  let _ = kill ls
+  exit <- until ls CP.exitH \complete -> \exit -> complete exit
+  case exit of
+    Normally 0 ->
+      log "All good!"
+    _ ->
+      liftEffect $ throw $ "Bad exit: expected `Normally 0`, got: " <> show exit
 
-nonExistentExecutable :: Effect Unit -> Effect Unit
-nonExistentExecutable done = do
-  ch <- spawn "this-does-not-exist" []
-  ch # on_ errorH \err ->
-    log (code err) *> done
+killsProcess :: Aff Unit
+killsProcess = do
+  log "\nkills processes"
+  ls <- liftEffect $ spawn "ls" [ "-la" ]
+  _ <- liftEffect $ kill ls
+  exit <- until ls CP.exitH \complete -> \exit -> complete exit
+  case exit of
+    BySignal s | Just SIGTERM <- Signal.fromString =<< (hush $ fromKillSignal s) ->
+      log "All good!"
+    _ -> do
+      liftEffect $ throw $ "Bad exit: expected `BySignal SIGTERM`, got: " <> show exit
 
-execLs :: Effect Unit
+execLs :: Aff Unit
 execLs = do
-  -- returned ChildProcess is ignored here
-  _ <- exec' "ls >&2" identity \r ->
-    log "redirected to stderr:" *> (Buffer.toString UTF8 r.stderr >>= log)
-  pure unit
+  log "\nexec"
+  r <- makeAff \done -> do
+    -- returned ChildProcess is ignored here
+    void $ exec' "ls >&2" identity (done <<< Right)
+    pure nonCanceler
+  stdout' <- liftEffect $ Buffer.toString UTF8 r.stdout
+  stderr' <- liftEffect $ Buffer.toString UTF8 r.stderr
+  when (stdout' /= "") do
+    liftEffect $ throw $ "stdout should be redirected to stderr but had content: " <> show stdout'
+  when (stderr' == "") do
+    liftEffect $ throw $ "stderr should have content but was empty"
+  log "stdout was successfully redirected to stderr"
 
-execSyncEcho :: String -> Effect Unit
-execSyncEcho str = do
+execSyncEcho :: String -> Aff Unit
+execSyncEcho str = liftEffect do
+  log "\nexecSyncEcho"
   buf <- Buffer.fromString str UTF8
   resBuf <- execSync' "cat" (_ { input = Just buf })
   res <- Buffer.toString NE.UTF8 resBuf
-  log res
+  when (str /= res) do
+    throw $ "cat did not output its input. \nGot: " <> show res <> "\nExpected: " <> show str
+  log "cat successfully re-outputted its input"
diff --git a/test/sleep.sh b/test/sleep.sh
@@ -0,0 +1,6 @@
+#!/usr/bin/env sh
+
+sleep 2
+echo "$1"
+
+echo "Done"