spawk.spawn.exit needs await

[katlim-br]

Maybe this is just something to add to the documentation, but I was banging my head for several hours because of jest open handles when running spawk.

So my friend reviewed this further, and so he found that, instead of:

        const spawnMock = spawk
            .spawn("mypath", myargs)
            .stdout("OK")
            .exit(0);

it had to be:

        const spawnMock = await spawk
            .spawn("mypath", myargs)
            .stdout("OK")
            .exit(0);

[wraithgar]

Yes jest appears to be buggy in how it deals with events. It only listens for exit or message events which means the close event is not being properly handled. The only other issue in this repo so far, ironically had to do with the exact same bug.

I'm not familiar enough w/ jest to suggest a fix for them, but maybe the research in the other issue could help?

As far as documentation yes I agree it could be better. I'm not sure that the results of exit or any other spawk method need to be awaited though. They all return this so that they can be chained together. The spawnMock is just an object, not a promise. The code itself that calls spawn is what would need to be awaited (if it was async) before checking spawnMock.called for example. Nothing in the interceptor is async in and of itself.

[wraithgar]

The decision in spawk to fire the exit event BEFORE the stdout/stderr writes and the close event was 100% intentional in this module.

spawk/lib/interceptor.js

Lines 287 to 303 in 7700f03

	this.child.emit('exit', emitCode, emitSignal)
	if (this.child.stdout) {
	if (stdout) {
	this.child.stdout.write(stdout)
	}
	this.child.stdout.end()
	}
	if (this.child.stderr) {
	if (stderr) {
	this.child.stderr.write(stderr)
	}
	this.child.stderr.end()
	}
	process.nextTick(() => {
	this.child.emit('close', emitCode, emitSignal)
	})
	}

This is because the node documentation is very clear on the matter and I wanted spawk to be able to trigger bugs in code that wasn't properly following the spec: https://nodejs.org/api/child_process.html#event-close

The 'close' event is emitted after a process has ended and the stdio streams of a child process have been closed. This is distinct from the 'exit' event, since multiple processes might share the same stdio streams. The 'close' event will always emit after 'exit' was already emitted, or 'error' if the child failed to spawn.

At this point though I don't know if I can rely on jest to fix this, and may have to add a jest compatibility mode.

[wraithgar]

#27

[wraithgar]

If you can test this using the git source instead of the published version that'd be great. npm i wraithgar/spawk would do it.

I added a jest compatibility flag and would like to know it works before publishing.

[katlim-br]

so the test would be without the await right?

[wraithgar]

Yes you don't need to await the results of spawk.spawn. That's just the interceptor class, which is an object not a promise.

[katlim-br]

I tried it in my machine and it kept giving me open handles. It did not work.

Here is part of the package.json

    "devDependencies": {
        "spawk": "wraithgar/spawk"
    }

Here is the test (with obscure values)

    it("should run with arg string", async () => {
        spawk
            .spawn("mybin", ["myarg"])
            .jestCompatibilityMode()
            .stdout("OK")
            .exit(0);

        const stdout = await spawnProcessAndWait("mybin", "myarg");
        expect(stdout).toBe("OK");
    });

Here is the code (I had to change it from my original code, but it keeps the same essence)

async function spawnProcessAndWait(procPath, procArgs) {
  const promise = new Promise((resolve, reject) => {
    const child = spawn(procPath, procArgs, {
      timeout: 20 * 60 * 1000,
    });

    let stdOutStr = "";
    child.stdout.on("data", (data) => {
      stdOutStr += data.toString();
    });

    child.stderr.on("data", (data) => {
      reject(new Error(data.toString()));
    });

    child.on("error", (error) => {
      reject(new Error(error.message));
    });

    child.on("close", () => {
      if (!process.env.JEST_WORKER_ID) {
        console.debug(`child process finished`);
      }

      resolve(stdOutStr);
    });
  });

  return promise;
}

[wraithgar]

Thank you, that code helped me. The problem with your code is likely that you're requiring spawn from child_process before you require spawk. Spawk has to be loaded before anything that requires child_process loads.

Once I swapped the requires so spawk came first, your test worked.

Coincidentally it worked with and without the new compatibility mode. When I changed the code to listen for exit instead of close, I got the old expected error that stdout had nothing on it, and when I enabled the compatibility mode it worked.

Looking into the old issue more it had to do with how jest itself was spawning things. The person was testing the @actions/exec worker itself, not using spawk inside of jest.

Your code should work, and works for me locally provided spawk is required first. I am going to update the README to be more specific about the use case that compatibility mode is fixing, and cut a release.

However you don't need to wait on that release as far as I can tell. This code works for me, the only two things I have installed are spawk@latest and jest@latest. Also note that there was a bug in your example of not passing an array for procArgs.

const spawk = require('spawk')
const { spawn } = require('child_process')

it("should run with arg string", async () => {
  spawk.preventUnmatched()
  spawk
    .spawn("mybin", ["myarg"])
    .stdout("OK")
    .exit(0);

  const stdout = await spawnProcessAndWait("mybin", ["myarg"]);
  expect(stdout).toBe("OK");
});

async function spawnProcessAndWait(procPath, procArgs) {
  const promise = new Promise((resolve, reject) => {
    const child = spawn(procPath, procArgs, {
      timeout: 20 * 60 * 1000,
    });

    let stdOutStr = "";
    child.stdout.on("data", (data) => {
      stdOutStr += data.toString();
    });

    child.stderr.on("data", (data) => {
      reject(new Error(data.toString()));
    });

    child.on("error", (error) => {
      reject(new Error(error.message));
    });

    child.on("close", () => {
      if (!process.env.JEST_WORKER_ID) {
        console.debug(`child process finished`);
      }

      resolve(stdOutStr);
    });
  });

  return promise;
}

$ npx jest test.js 
 PASS  ./test.js
  ✓ should run with arg string (7 ms)

Test Suites: 1 passed, 1 total
Tests:       1 passed, 1 total
Snapshots:   0 total
Time:        0.191 s, estimated 1 s
Ran all test suites matching /test.js/i.

[wraithgar]

After reading the jest code again it appears to use fork not spawn which means spawk wouldn't work there. I'm going to remove this compatibility mode for now because it is not needed, and try to get around to writing a spawk.fork method so folks can mock that too.

[katlim-br]

Actually my code did import spawk BEFORE child_process therefore the await is still needed (I know it doesnt make sense but it works).

I dont know further details on jest to comment about the specifics of fork and spawn, but it does seem to me that shouldn't impact on your library right? unless jest is already hijacking spawn or fork.

[wraithgar]

I was able to use your example and write a test that passed in jest. Without something I can use that shows broken behavior I can't fix it. The exit function on the interceptor is not async, it sets a property and returns

  exit (code) {
    this.#interceptor.exitCode = code
    return this
  }

The only thing I can think of is that adding that await moves the result to the next event loop and you have a race condition somewhere. This is definitely not a spawk bug though, so I'm gonna close this.

If you have some code that doesn't work with spawk that I can use to reproduce this please make an issue or reopen this one and I'll look into it.

[katlim-br]

ok thats fair.