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

lib: prefer iterable weak set than WeakRefs in AbortSignal.any #54943

Closed
wants to merge 1 commit into from

Conversation

jazelly
Copy link
Member

@jazelly jazelly commented Sep 14, 2024

The dom spec specified the usage of weak sets for dependent and source signals. This came from a discussion in the original dom spec PR, which explicitly stated "weak sets" rather than "a set of weak references" to avoid extra new WeakRef and deref when interacting with the signals. This PR makes that behavior spec compliant.

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Sep 14, 2024
Copy link

codecov bot commented Sep 14, 2024

Codecov Report

Attention: Patch coverage is 97.67442% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.08%. Comparing base (75e4d0d) to head (2a7a07f).
Report is 21 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/abort_controller.js 97.67% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #54943      +/-   ##
==========================================
+ Coverage   88.06%   88.08%   +0.01%     
==========================================
  Files         651      651              
  Lines      183451   183567     +116     
  Branches    35821    35866      +45     
==========================================
+ Hits       161564   161692     +128     
+ Misses      15133    15130       -3     
+ Partials     6754     6745       -9     
Files with missing lines Coverage Δ
lib/internal/abort_controller.js 98.02% <97.67%> (+0.11%) ⬆️

... and 42 files with indirect coverage changes

@benjamingr
Copy link
Member

I don't think this is clearer or aligns any more or less with the spec than the previous implementation to be honest.

If there is a spec compliance issue, please add a WPT with a failing case before (but passing after) this change

@anonrig anonrig added the needs-benchmark-ci PR that need a benchmark CI run. label Sep 14, 2024
@jazelly
Copy link
Member Author

jazelly commented Sep 15, 2024

@benjamingr Hi Benjamin, thanks for the review. I actually thought about that since this doesn't fix any WPT. However, there is one behaviour change and one benefit that I saw initially.

  1. Spec specified this to be an ordered set, but ours were a set, which is unorderd (correct me if I'm wrong), which is why I chose array to support this ordered behaviour.
  2. This will not confuse implementers with other spec, as others distinguish themselve with explicitly stated weak references. For example, when I was doing another PR on abort_controller, some of the steps didn't mention deref at all, but one needs to carefully manage that with our implementation.

This might be trivial, but those are my thoughts : )

@benjamingr
Copy link
Member

Every JavaScript set is an ordered set (on its insertion/iteration order). This (iteration order) is actually why the DOM spec typically uses an ordered set. This is weird but the language works like this - it's also why the WPTs related to order passed.

I think the relevant part is whatwg/dom#1152 (comment) between Domenic/Scott in which case I agree with Domenic it made the spec code cleaner, I'm not sure that translates to our code. Cancellation semantics and weak collections/refs are both notoriously tricky and going through a new collection here doesn't help.

(I appreciate your effort here and engagement, I hope the fact I don't like this change doesn't come off as less appreciating of the work put here - more people to engage around these areas is greatly appreciated so thanks 🙏 )

@jazelly
Copy link
Member Author

jazelly commented Sep 15, 2024

it's also why the WPTs related to order passed

I missed that. That makes sense.

I hope the fact I don't like this change doesn't come off as less appreciating of the work put here

Not at all. I only raised this as to get others' ideas. I leared a lot from the conversation. Thanks!

@jazelly jazelly closed this Sep 15, 2024
@jazelly jazelly deleted the weakset-abort-signal branch September 15, 2024 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-benchmark-ci PR that need a benchmark CI run. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants