test: add test for reused AbortController with execfile() by Trott · Pull Request #36644 · nodejs/node · GitHub

Test that reusing an aborted AbortController with execfile() results in
immediate SIGTERM.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

@benjamingr Is the behavior seen here as expected?

CI: https://ci.nodejs.org/job/node-test-pull-request/35097/

github makes the PR very hard to read - but the behaviour seems correct yes :]

github makes the PR very hard to read - but the behaviour seems correct yes :]

Should be easier to read now that I've removed the lint errors. Whoops!

CI: https://ci.nodejs.org/job/node-test-pull-request/35098/

What do you think about the behaviour? I think an AbortError might be nicer to users? Not sure

What do you think about the behaviour? I think an AbortError might be nicer to users? Not sure

That's what I was expecting, but I'm not familiar with the spec or general usage TBH.

That's what I was expecting, but I'm not familiar with the spec or general usage TBH.

I think that makes more sense yeah - want to fix the code to reject with that? Should be pretty straightforward.

(If you don't feel like it lmk and I'll make a separate PR or push another commit to this one whatever you prefer)

The other possible reasonable-seeming options are:

throw an error if the AbortController is already in an aborted state when execfile() is called
ignore an AbortController that is already in an aborted state when execfile() is called

Are we (you, I guess) sure those aren't better options?

throw an error if the AbortController is already in an aborted state when execfile() is called

That sounds like the preferable alternative to me.

throw an error if the AbortController is already in an aborted state when execfile() is called

That sounds like the preferable alternative to me.

Would it be appropriate to Do Whatever Fetch Does In That SItuation?

Would it be appropriate to Do Whatever Fetch Does In That SItuation?

const ac = new AbortController();
const { signal } = ac;
ac.abort();
fetch('./', { signal }).catch(() => {});
fetch('./', { signal }).catch(console.log); // Logs an AbortError

AbortError it is I think

AbortError it is I think

OK, done! Guess I better go and check the behavior of everything else that uses AbortController in child_process for consistency.

OK, I think this is ready for review.

CI: https://ci.nodejs.org/job/node-test-pull-request/35132/

Test that reusing an aborted AbortController with execfile() results in immediate SIGTERM. PR-URL: nodejs#36644 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>

If an AbortController passed to execfile() is already aborted, use the same behavior as if the controller was aborted after calling execfile(). This mimics the behavior of fetch in the browser. PR-URL: nodejs#36644 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>

Move duplicate abort handler logic into a separate function. PR-URL: nodejs#36644 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>

PR-URL: nodejs#36644 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>

Landed in faee739...37acaf6

I didn't realize the GitHub labels automated Rich 😮

Test that reusing an aborted AbortController with execfile() results in immediate SIGTERM. PR-URL: #36644 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>

If an AbortController passed to execfile() is already aborted, use the same behavior as if the controller was aborted after calling execfile(). This mimics the behavior of fetch in the browser. PR-URL: #36644 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>

Move duplicate abort handler logic into a separate function. PR-URL: #36644 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>

PR-URL: #36644 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>