Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Awkward behavior on error with pipe #640

Closed
mighty1231 opened this issue Jun 11, 2023 · 4 comments · Fixed by #899
Closed

Awkward behavior on error with pipe #640

mighty1231 opened this issue Jun 11, 2023 · 4 comments · Fixed by #899
Assignees

Comments

@mighty1231
Copy link

mighty1231 commented Jun 11, 2023

single exit 1 process - throws error

exit 1 process with piped - does not throw error

Is it intended behavior?

async function checkPipe() {
  try {
    await $`exit 1`.pipe($`echo hello`);
    console.log("not error at first");
  } catch (e) {
    console.log("it does not reach here (1)");
  }

  try {
    await $`exit 1`;
    console.log("it does not reach here (2)");
  } catch (e) {
    console.log("error catched!");
  }
}

await checkPipe();

Expected Stdout

$ exit 1
$ echo hello
hello
it does not reach here (1)
error catched!

Actual Behavior

$ exit 1
$ echo hello
hello
not error at first
file:///wd/temp.ts:106
    await $`exit 1`.pipe($`echo hello`);
           ^
ProcessOutput [Error]: 
    at checkPipe (file:///wd/temp.ts:106:12)
    exit code: 1
    at ChildProcess.<anonymous> (file:///wd/node_modules/zx/build/core.js:146:26)
    at ChildProcess.emit (node:events:513:28)
    at ChildProcess.emit (node:domain:489:12)
    at maybeClose (node:internal/child_process:1100:16)
    at Socket.<anonymous> (node:internal/child_process:458:11)
    at Socket.emit (node:events:513:28)
    at Socket.emit (node:domain:489:12)
    at Pipe.<anonymous> (node:net:301:12)
    at Pipe.callbackTrampoline (node:internal/async_hooks:130:17) {
  _code: 1,
  _signal: null,
  _stdout: '',
  _stderr: '',
  _combined: ''
}

Specifications

  • Version: 7.2.2
  • Platform: Linux (Ubuntu 18.04)
@mighty1231 mighty1231 changed the title Cannot catch error when pipe is used Cannot catch error with pipe Jun 11, 2023
@mighty1231 mighty1231 changed the title Cannot catch error with pipe Awkward behavior on error with pipe Jun 11, 2023
@antonmedv
Copy link
Collaborator

@antongolub what do you think is a proper behavior here?

@antongolub
Copy link
Collaborator

antongolub commented Mar 26, 2024

Well, the queue is pretty clear:

  1. $`exit 1` creates the first delayed action (setImmediate)
  2. $`echo hello` creates the second
  3. The pipe method bounds processes via internal postrun / prerun hooks.
  4. Then the 1st deferred setImmediate triggers internal _run, it fails and drowns the bound dest._prerun = this.run.bind(this).

I think, this is fine. Just imagine a case, when some piped process awaits for the input and meets exit 1 (like cat). I'd expect it to fall.

@mighty1231
Copy link
Author

mighty1231 commented Mar 29, 2024

@antonmedv Is it fine that none of the log executed after the second $ call?

neither these logs shown

  • it does not reach here (2)
  • error catched!

Let's rewrite the issue in more detailed way with 3 examples.

case 1 (normal)

just a single $ statement handles exception in nice way

import { $ } from "zx/core";

async function checkPipe() {
  try {
    await $`exit 1`;
    console.log("it does not reach here (2)");
  } catch (e) {
    console.log("error catched!");
  }
}

await checkPipe();

result

$ node abc.mjs 2>/dev/null
error catched!

case 2 (weird)

import { $ } from "zx/core";

async function checkPipe() {
  // run a piped process that fails
  try {
    await $`exit 1`.pipe($`echo hello`);
  } catch (e) {
    // do nothing
  }

  console.log('no matter for intermediate log');

  // same as case 1 below
  try {
    await $`exit 1`;
    console.log("it does not reach here (2)");
  } catch (e) {
    console.log("error catched!");
  }
}

await checkPipe();

result

$ node abc.mjs 2>/dev/null
no matter for intermediate log

where does the log error catched! go?

case 3

How about pipe in bash instead zx's pipe?

import { $ } from "zx/core";

async function checkPipe() {
  try {
    // bash's pipe
    await $`exit 1 | echo hello`;
  } catch (e) {
    // do nothing
  }

  console.log('no matter for intermediate log');

  try {
    await $`exit 1`;
    console.log("it does not reach here (2)");
  } catch (e) {
    console.log("error catched!");
  }
}

await checkPipe();

result

$ node abc.mjs 2>/dev/null
no matter for intermediate log
error catched!

Hello error catched!

Specification

  • Version: 7.2.2
  • Platform: Linux (Ubuntu 22.04)
  • Node version: v16.20.2

@antonmedv antonmedv reopened this Mar 29, 2024
@antongolub antongolub self-assigned this Sep 16, 2024
@antongolub
Copy link
Collaborator

antongolub added a commit to antongolub/zx that referenced this issue Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants