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

Internal Assertion in AbortSignal #54601

Closed
RedYetiDev opened this issue Aug 28, 2024 · 6 comments
Closed

Internal Assertion in AbortSignal #54601

RedYetiDev opened this issue Aug 28, 2024 · 6 comments
Labels
abortcontroller Issues and PRs related to the AbortController API confirmed-bug Issues with confirmed bugs. web-standards Issues and PRs related to Web APIs

Comments

@RedYetiDev
Copy link
Member

RedYetiDev commented Aug 28, 2024

I am working on updating the Web Platform Tests in #54468, but ran into an issue occuring with AbortSignal. I narrowed down the issue to a minimal reproduction, with leads to an ERR_INTERNAL_ASSERTION:

const controller = new AbortController();
const signal1 = AbortSignal.any([controller.signal]);

controller.signal.addEventListener('abort', () => {
    AbortSignal.any([signal1]);
})

controller.abort();
node:internal/event_target:1090
  process.nextTick(() => { throw err; });
                           ^

Error [ERR_INTERNAL_ASSERTION]: This is caused by either a bug in Node.js or incorrect usage of Node.js internals.
Please open an issue with this stack trace at https://github.com/nodejs/node/issues

    at assert (node:internal/assert:14:11)
    at AbortSignal.any (node:internal/abort_controller:253:11)
    at AbortSignal.<anonymous> (/repro.js:5:17)
    at [nodejs.internal.kHybridDispatch] (node:internal/event_target:816:20)
    at AbortSignal.dispatchEvent (node:internal/event_target:751:26)
    at abortSignal (node:internal/abort_controller:374:10)
    at AbortController.abort (node:internal/abort_controller:396:5)
    at Object.<anonymous> (/repro.js:8:12)
    at Module._compile (node:internal/modules/cjs/loader:1546:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1691:10) {
  code: 'ERR_INTERNAL_ASSERTION'
}

Node.js v22.7.0

The failed assertion is

assert(!sourceSignalRef.aborted);


It seems to me that Node.js's current behavior is to fail the assertion when a signal has already aborted, however the WPT's expected behavior is that "Dependent signals for AbortSignal.any() are marked aborted before abort events fire":
https://github.com/web-platform-tests/wpt/blob/e78446e34a1921371658a5df08c71d83f50a2a2f/dom/abort/resources/abort-signal-any-tests.js#L193

@RedYetiDev RedYetiDev added repro-exists abortcontroller Issues and PRs related to the AbortController API labels Aug 28, 2024
@RedYetiDev
Copy link
Member Author

@shubhamsugara22
Copy link

shubhamsugara22 commented Aug 30, 2024

Dependent abort signal that allow one or more 'AbortSignal' object to depend on each other
the dependent signal will automatically aborted if the parent is aborted . So the links explains the behavior that AbortSignal should behave like that but it fails assertion when signal has already been aborted .
Looks like you can test the case by skipping the signal if aborted or check if the signal is aborted creating an exception handler
this 2 case can be tested to see if the impact and early test if we modification can be made to this
if you have any other way please discuss?
@RedYetiDev

@RedYetiDev
Copy link
Member Author

I'm sorry, I'm not exactly following your comment, could you elaborate?

The issue, to clarify, is that in internal assertion is thrown in a place where a signal should be returned (to be WPT compliant).

@RedYetiDev RedYetiDev added the web-standards Issues and PRs related to Web APIs label Aug 30, 2024
@shubhamsugara22
Copy link

😞 Sorry for that i meant this:
What is understood
A dependent abort signal is designed so that when the parent signal is aborted, the dependent signal should automatically follow suit. The DOM specification outlines this behavior, but it seems that Node.js currently fails this internal assertion when a signal has already been aborted.
Solution we can try ?
One possible way to address this would be to check if the signal has been aborted before attempting to create a dependent signal. If it has, we could skip it or handle it with an exception. This would help ensure compliance with the WPT. We could start by testing these approaches to see if they resolve the issue.

If you have any other suggestions or ideas on how to ensure WPT compliance while avoiding this assertion, I'd love to discuss them!

@RedYetiDev
Copy link
Member Author

RedYetiDev commented Aug 30, 2024

Ahh okay. If you have an idea/improvement, opening a PR is a great way to contribute

@shubhamsugara22
Copy link

yeah , thanks will try to open one for this

@RedYetiDev RedYetiDev added confirmed-bug Issues with confirmed bugs. and removed repro-exists labels Sep 10, 2024
targos pushed a commit that referenced this issue Oct 4, 2024
PR-URL: #54826
Fixes: #54466
Fixes: #54601
Reviewed-By: James M Snell <jasnell@gmail.com>
louwers pushed a commit to louwers/node that referenced this issue Nov 2, 2024
PR-URL: nodejs#54826
Fixes: nodejs#54466
Fixes: nodejs#54601
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abortcontroller Issues and PRs related to the AbortController API confirmed-bug Issues with confirmed bugs. web-standards Issues and PRs related to Web APIs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants