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

Editorial: remove redundant contains check in AbortSignal.any() #1244

Merged
merged 1 commit into from
Jan 16, 2024

Conversation

vinhill
Copy link
Contributor

@vinhill vinhill commented Jan 9, 2024

Step 4.2.2. of create a dependent abort signal prevents duplicated signals in source signals for example in the following snippet

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

so maybe the same should hold for

const signal2 = AbortSignal.any([ AbortSignal.any([controller.signal]), controller.signal ]);
const signal2 = AbortSignal.any([ controller.signal, controller.signal ]);

To my knowledge, duplicated signals in source signals should not have any impact due to step 1 of signal abort ensuring signals are only aborted once. So this change should cause no behavior change.

PR Questions


Preview | Diff

@annevk
Copy link
Member

annevk commented Jan 9, 2024

Isn't this what https://infra.spec.whatwg.org/#sets already takes care of?

@vinhill
Copy link
Contributor Author

vinhill commented Jan 9, 2024

My bad, it does. Would it make sense then to remove step 4.2.2.?

@annevk
Copy link
Member

annevk commented Jan 9, 2024

@shaseley might know. Given that it's followed by two appends it's not immediately clear at least.

@shaseley
Copy link
Contributor

shaseley commented Jan 9, 2024

4.2.2 should be safe to remove since both 4.2.3 and 4.2.4 would be no-ops for a duplicate source. I don't think the difference was intentional (and I remember purposely omitting the check in 4.1 because of set behavior); there might have been a previous version of the any() PR where skipping the steps mattered, but that's not the case now -- so removing to match 4.1 SGTM.

Source signals is a set wherefore the step is redundant.
With this change, step 4.2 matches step 4.1 more closely.
@vinhill
Copy link
Contributor Author

vinhill commented Jan 16, 2024

Is it fine to do the change in this pull request, or should I open a new one? Can I omit implementation bugs given this change should have no impact?

@annevk
Copy link
Member

annevk commented Jan 16, 2024

It's fine here. And yes, no need for bugs on editorial changes.

We'll have to prefix it with "Editorial: ", but I can do that when landing.

@annevk annevk changed the title Prevent AbortSignal.any() from causing duplicates in source signals Editorial: remove redundant contains check in AbortSignal.any() Jan 16, 2024
@annevk annevk merged commit c953c86 into whatwg:main Jan 16, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants