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

"If signal is not null, then add the following a..." #668

Closed
ricea opened this issue Feb 7, 2018 · 7 comments

Comments

@ricea
Copy link
Collaborator

commented Feb 7, 2018

https://fetch.spec.whatwg.org/commit-snapshots/36ef3c8aef34ff77ebf713b6498d008fe853034f/#request-class

If signal is not null, then add the following abort steps to signal:​

Signal abort on r’s signal.

I may be reading this wrong, but it appears to me that this implies that r's signal will not have the aborted flag set even if |signal| does.

This doesn't appear to the correct behaviour. For example, test "Signal on request object" https://github.com/w3c/web-platform-tests/blob/0d311189506d0268f62198fde499a7a32fc58c87/fetch/api/abort/general.any.js#L71 expects that a fetch will be aborted if created from a Request that was passed an already-aborted signal.

I think it should be

  1. If signal is not null, then
    1. If signal’s aborted flag is set, set r's signal's aborted flag.
    2. Otherwise, add the following abort steps to signal:
      1. Signal abort on r’s signal.
@domenic

This comment has been minimized.

Copy link
Member

commented Feb 7, 2018

@jakearchibald, can you help with this while @annevk is on vacation?

@domenic

This comment has been minimized.

Copy link
Member

commented Feb 7, 2018

Note that a similar issue seems present in clone(). Maybe we would want to add a primitive in DOM for one signal to "follow" another.

@ricea

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 8, 2018

Note that a similar issue seems present in clone(). Maybe we would want to add a primitive in DOM for one signal to "follow" another.

As a datapoint, when implementing this for Chrome I originally included an internal "CreateChainedSignal" method to perform the operation. However, I removed it again, partly because the algorithm in clone() is different, and partly because the signal created by the Request constructor is not always chained off another signal.

@tyoshino

This comment has been minimized.

Copy link
Member

commented Feb 8, 2018

The change proposed by ricea@ looks good. Just a subtle point. Maybe the step (i) is only modifying the aborted flag since we know that it's a just-made AbortSignal. Just as a principle, it's nice in terms of future-proofness to depend on a limited set of public interfaces, i.e. Signal abort. I kinda feel that it makes sense to have some helper to do chaining safely as ricea@ mentioned, in the spec.

@jakearchibald

This comment has been minimized.

Copy link
Collaborator

commented Feb 8, 2018

Thanks for flagging this, will create a PR tomorrow

@jakearchibald jakearchibald self-assigned this Feb 8, 2018

@ricea

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 8, 2018

Just as a principle, it's nice in terms of future-proofness to depend on a limited set of public interfaces, i.e. Signal abort.

This is actually what I did for Chrome's implementation. No point in poking a hole through the AbortSignal abstraction just to avoid a check that "abort algorithms" is empty.

jakearchibald added a commit that referenced this issue Feb 9, 2018
@jakearchibald

This comment has been minimized.

Copy link
Collaborator

commented Feb 9, 2018

PRs:

  • whatwg/dom#569 - Adding the concept of one signal following another.
  • #670 - "following" in fetch.
domenic added a commit that referenced this issue Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.