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

test: add test for reused AbortController with execfile() #36644

Merged
merged 4 commits into from Dec 30, 2020

Conversation

@Trott
Copy link
Member

@Trott Trott commented Dec 27, 2020

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
@Trott
Copy link
Member Author

@Trott Trott commented Dec 27, 2020

@benjamingr Is the behavior seen here as expected?

@Trott Trott requested a review from benjamingr Dec 27, 2020
@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Dec 27, 2020

@benjamingr
Copy link
Member

@benjamingr benjamingr commented Dec 27, 2020

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

@Trott
Copy link
Member Author

@Trott Trott commented Dec 27, 2020

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!

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Dec 27, 2020

@benjamingr
Copy link
Member

@benjamingr benjamingr commented Dec 27, 2020

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

@Trott
Copy link
Member Author

@Trott Trott commented Dec 28, 2020

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.

@benjamingr
Copy link
Member

@benjamingr benjamingr commented Dec 28, 2020

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)

@Trott
Copy link
Member Author

@Trott Trott commented Dec 28, 2020

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?

@benjamingr
Copy link
Member

@benjamingr benjamingr commented Dec 28, 2020

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

That sounds like the preferable alternative to me.

@Trott
Copy link
Member Author

@Trott Trott commented Dec 28, 2020

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?

@benjamingr
Copy link
Member

@benjamingr benjamingr commented Dec 28, 2020

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

@Trott
Copy link
Member Author

@Trott Trott commented Dec 29, 2020

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.

lib/child_process.js Outdated Show resolved Hide resolved
@Trott
Copy link
Member Author

@Trott Trott commented Dec 29, 2020

OK, I think this is ready for review.

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Dec 29, 2020

Trott added 4 commits Dec 30, 2020
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>
@Trott
Copy link
Member Author

@Trott Trott commented Dec 30, 2020

Landed in faee739...37acaf6

@Trott Trott merged commit 37acaf6 into nodejs:master Dec 30, 2020
16 checks passed
@Trott Trott deleted the reused-ac branch Dec 30, 2020
@benjamingr
Copy link
Member

@benjamingr benjamingr commented Dec 30, 2020

I didn't realize the GitHub labels automated Rich 😮

danielleadams added a commit that referenced this issue Jan 12, 2021
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>
danielleadams added a commit that referenced this issue Jan 12, 2021
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>
danielleadams added a commit that referenced this issue Jan 12, 2021
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>
danielleadams added a commit that referenced this issue Jan 12, 2021
PR-URL: #36644
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
@danielleadams danielleadams mentioned this pull request Jan 12, 2021
targos added a commit that referenced this issue May 25, 2021
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>
targos added a commit that referenced this issue May 25, 2021
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>
targos added a commit that referenced this issue May 25, 2021
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>
targos added a commit that referenced this issue May 25, 2021
PR-URL: #36644
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
targos added a commit that referenced this issue Jun 5, 2021
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>
targos added a commit that referenced this issue Jun 5, 2021
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>
targos added a commit that referenced this issue Jun 5, 2021
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>
targos added a commit that referenced this issue Jun 5, 2021
PR-URL: #36644
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
targos added a commit that referenced this issue Jun 11, 2021
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>
targos added a commit that referenced this issue Jun 11, 2021
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>
targos added a commit that referenced this issue Jun 11, 2021
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>
targos added a commit that referenced this issue Jun 11, 2021
PR-URL: #36644
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants