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.

Sign up for GitHub

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

Jump to bottom

Use an AbortController instead of signaling abort on AbortSignal #1279

Merged
merged 2 commits into from
May 31, 2023
Merged

Use an AbortController instead of signaling abort on AbortSignal #1279

merged 2 commits into from
May 31, 2023

Conversation

shaseley
Copy link
Contributor

@shaseley shaseley commented May 24, 2023 •
edited by pr-preview bot
Loading

For whatwg/dom#1194 (and follow-up from #1277).

  • Replace WritableStreamDefaultController's [[signal]] with [[abortController]], using this abort controller's signal where [[signal]] was used. Signal abort on [[abortController]] rather than the [[signal]].
  • Remove the note/warning about specs not signaling abort on the WritableStream's signal, since this won't be allowed once AbortSignal's "signal abort" no longer exported.

Preview | Diff

@shaseley
Use an AbortController instead of signaling abort on AbortSignal
10c960e 
 - Replace WritableStreamDefaultController's [[signal]] with
   [[AbortController]], using the controller's signal where [[signal]]
   was used. Signal abort on [[abortController]] rather than the
   [[signal]].
 - Remove the note about specs not signaling abort on the
   WritableStream's signal, since this won't be possible once
   AbortSignal's "signal abort" no longer exported.
@shaseley
Copy link
Contributor Author

@annevk how does this look?

annevk
annevk reviewed May 25, 2023
View reviewed changes
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you replace the getter with the appropriate reference I think this works. Thanks!

@MattiasBuelens should do the final review though.

index.bs Outdated
@@ -4508,7 +4508,7 @@ closed. It is only used internally, and is never exposed to web developers.
The <dfn id="ws-default-controller-signal" attribute
for="WritableStreamDefaultController">signal</dfn> getter steps are:

1. Return [=this=].[=WritableStreamDefaultController/[[signal]]=].
1. Return [=this=].[=WritableStreamDefaultController/[[abortController]]=].{{AbortController/signal}}.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to return the value of the "internal slot" and not reference the "public" getter.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand Bikeshed's autolink syntax correctly, this should do the trick:

Suggested change
1. Return [=this=].[=WritableStreamDefaultController/[[abortController]]=].{{AbortController/signal}}.
1. Return [=this=].[=WritableStreamDefaultController/[[abortController]]=].[=AbortController/signal=].

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd have to be something like

Suggested change
1. Return [=this=].[=WritableStreamDefaultController/[[abortController]]=].{{AbortController/signal}}.
1. Return [=this=].[=WritableStreamDefaultController/[[abortController]]=]'s [=AbortController/signal=].

as we don't define dot syntax for these.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah okay, makes sense. We'll need to export [=AbortController/signal=] for this to build/link, which I can do along with removing export from "signal abort".

index.bs Outdated Show resolved Hide resolved
MattiasBuelens
MattiasBuelens requested changes May 25, 2023
View reviewed changes
index.bs Outdated Show resolved Hide resolved
index.bs Outdated
@@ -4508,7 +4508,7 @@ closed. It is only used internally, and is never exposed to web developers.
The <dfn id="ws-default-controller-signal" attribute
for="WritableStreamDefaultController">signal</dfn> getter steps are:

1. Return [=this=].[=WritableStreamDefaultController/[[signal]]=].
1. Return [=this=].[=WritableStreamDefaultController/[[abortController]]=].{{AbortController/signal}}.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand Bikeshed's autolink syntax correctly, this should do the trick:

Suggested change
1. Return [=this=].[=WritableStreamDefaultController/[[abortController]]=].{{AbortController/signal}}.
1. Return [=this=].[=WritableStreamDefaultController/[[abortController]]=].[=AbortController/signal=].

shaseley
shaseley commented May 25, 2023
View reviewed changes
Copy link
Contributor Author

@shaseley shaseley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reviewing! I'll put together the dom changes this depends on.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated
@@ -4508,7 +4508,7 @@ closed. It is only used internally, and is never exposed to web developers.
The <dfn id="ws-default-controller-signal" attribute
for="WritableStreamDefaultController">signal</dfn> getter steps are:

1. Return [=this=].[=WritableStreamDefaultController/[[signal]]=].
1. Return [=this=].[=WritableStreamDefaultController/[[abortController]]=].{{AbortController/signal}}.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah okay, makes sense. We'll need to export [=AbortController/signal=] for this to build/link, which I can do along with removing export from "signal abort".

index.bs Outdated Show resolved Hide resolved
ricea
ricea approved these changes May 29, 2023
View reviewed changes
Copy link
Collaborator

@ricea ricea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm once it builds.

@MattiasBuelens
Copy link
Collaborator

Latest build failure looks like a problem with Bikeshed itself. I've opened speced/bikeshed#2561.

@MattiasBuelens
Copy link
Collaborator

The new Bikeshed version fixes the build issue, so I think we're good to go! 🙂

@ricea ricea merged commit eba8c79 into whatwg:main May 31, 2023
aarongable pushed a commit to chromium/chromium that referenced this pull request May 31, 2023
@shaseley
AbortSignal: Don't call SignalAbort directly in streams
6202f75 
Instead:
 - WritableStreamDefaultController has a AbortController
 - WritableStreamDefaultController::signal() returns the controller's
   signal
 - The signal is aborted via AbortController::abort()

Spec PR: whatwg/streams#1279

Bug: 1448352
Change-Id: I2abe12b9570b11de0767888a369316d3622c908d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4564333
Commit-Queue: Scott Haseley <shaseley@chromium.org>
Reviewed-by: Adam Rice <ricea@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1151315}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@annevk annevk annevk left review comments

@MattiasBuelens MattiasBuelens MattiasBuelens approved these changes

@ricea ricea ricea approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@shaseley @MattiasBuelens @annevk @ricea