Aborting fetch (and possibly streams), part 2

RESOLVED FIXED in Firefox 57

Status

()

RESOLVED FIXED
2 years ago
8 days ago

People

(Reporter: annevk, Assigned: baku)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla57
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(11 attachments, 7 obsolete attachments)

6.57 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
42.79 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
8.51 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
10.94 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
15.51 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
3.34 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
15.49 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
2.16 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
5.13 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
37.38 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
14.44 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
We have two objects defined by DOM (AbortController and AbortSignal):

  https://github.com/whatwg/dom/pull/437

That DOM PR is considered done. The reason it hasn't landed yet is because the Fetch PR https://github.com/whatwg/fetch/pull/523 is not finalized and the Streams PR https://github.com/whatwg/streams/pull/744 doesn't have enough implementer commitments yet (we don't implement writable streams or piping thus far, but we may as well express support I think since we'll do it eventually).

As far as the Fetch PR goes, the API is decided on and is pretty similar to what we have I think, we just need to figure out how we want to write down the requirements in the standards.
Are there any WPT tests written yet?
Keywords: dev-doc-needed
(Assignee)

Comment 2

2 years ago
Posted patch part 1 - Moving to dom/abort (obsolete) — Splinter Review
AbortController/AbortSignal are not part of fetch. We need to have it in a separate folder.
Attachment #8891473 - Flags: review?(bkelly)
(Assignee)

Comment 3

2 years ago
Posted patch part 2 - renaming (obsolete) — Splinter Review
FetchController -> AbortController
FetchSignal -> AbortSignal
Attachment #8891475 - Flags: review?(bkelly)
(Assignee)

Comment 4

2 years ago
Attachment #8891476 - Flags: review?(bkelly)
(Assignee)

Comment 5

2 years ago
Attachment #8891477 - Flags: review?(bkelly)
(Reporter)

Comment 6

2 years ago
Tests for the DOM part:

  https://github.com/w3c/web-platform-tests/pull/5960

Fetch part:

  https://github.com/w3c/web-platform-tests/pull/6484

(Streams is out-of-scope for this bug at this point.)
I wanted to run these patches against the tests now that they are available.  Unfortunately it seems something is missing because they don't compile:

4:12.17 /srv/mozilla-central/obj-x86_64-pc-linux-gnu-optdebug/dom/bindings/RequestBinding.cpp:4:10: fatal error: 'AbortObserver.h' file not found
 4:12.18 #include "AbortObserver.h"


Forget to hg add a file?

Anyway, dropping flags here until you have a chance to run them against the WPT tests.
Attachment #8891473 - Flags: review?(bkelly)
Attachment #8891475 - Flags: review?(bkelly)
Attachment #8891476 - Flags: review?(bkelly)
Attachment #8891477 - Flags: review?(bkelly)
(Assignee)

Comment 8

2 years ago
Attachment #8891473 - Attachment is obsolete: true
Attachment #8899728 - Flags: review?(bkelly)
(Assignee)

Comment 9

2 years ago
Attachment #8891475 - Attachment is obsolete: true
Attachment #8899729 - Flags: review?(bkelly)
(Assignee)

Comment 10

2 years ago
Attachment #8891476 - Attachment is obsolete: true
Attachment #8899730 - Flags: review?(bkelly)
(Assignee)

Updated

2 years ago
Attachment #8891477 - Attachment is obsolete: true
(Assignee)

Comment 12

2 years ago
Attachment #8899733 - Flags: review?(bkelly)
(Assignee)

Comment 13

2 years ago
Posted patch part 6 - WPTsSplinter Review
Many tests pass. The missing ones are failing because we don't implement request.signal yet and because we don't abort the blob()/arrayBuffer()/... but only the networking part.

I think we can implement these 2 issues as follow up, maybe disabling abort+fetch in nightly, and just keepting AbortController/AbortSignal as separate api enabled. We have 2 prefs for 2 reasons.

annevk, what is blocking the AbortController/Signal part to land in the fetch API spec?
Flags: needinfo?(annevk)
Attachment #8899735 - Flags: review?(bkelly)
(Reporter)

Comment 14

2 years ago
Some additional review from Domenic. It's basically done. You can find the PR here:

  https://github.com/whatwg/fetch/pull/523

And a preview of that PR applied to the specification here:

  https://fetch.spec.whatwg.org/branch-snapshots/cancelation/

Hope that helps.
Flags: needinfo?(annevk)
(Assignee)

Comment 15

2 years ago
Posted patch part 7 - Request.signal (obsolete) — Splinter Review
The cache API request.signal is not correctly implemented yet. I just covered the Fetch API here.
Attachment #8900629 - Flags: review?(bkelly)
(Assignee)

Comment 16

2 years ago
Attachment #8900632 - Flags: review?(bkelly)
(Assignee)

Comment 17

2 years ago
Posted patch part 9 - abort streams (obsolete) — Splinter Review
Attachment #8900669 - Flags: review?(bkelly)
(Assignee)

Comment 18

2 years ago
new Request(foo, { signal }).signal is a new object. request.signal has to follow the original signal.
Attachment #8900729 - Flags: review?(bkelly)
(Assignee)

Comment 20

2 years ago
With this patch we are green on try.
Attachment #8901102 - Flags: review?(bkelly)
Attachment #8899728 - Flags: review?(bkelly) → review+
Comment on attachment 8899729 [details] [diff] [review]
part 2 - renaming

Review of attachment 8899729 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.

::: dom/fetch/Request.h
@@ +11,5 @@
>  #include "nsISupportsImpl.h"
>  #include "nsWrapperCache.h"
>  
>  #include "mozilla/dom/Fetch.h"
> +#include "mozilla/dom/AbortSignal.h"

Why do we need this include here if there are no uses FetchSignal/AbortSignal in this file?

::: dom/tests/mochitest/fetch/file_fetch_observer.html
@@ +53,5 @@
>    });
>  }
>  
>  function testObserveErrored() {
> +  var ac = new FetchController();

This is still FetchController?

@@ +70,5 @@
>    });
>  }
>  
>  function testObserveResponding() {
> +  var ac = new FetchController();

Still FetchController?

Do these tests pass?

::: dom/webidl/AbortController.webidl
@@ +5,5 @@
>   */
>  
>  [Constructor(), Exposed=(Window,Worker),
> + Func="AbortController::IsEnabled"]
> +interface AbortController {

Please include a link to the spec now that its landed:

https://dom.spec.whatwg.org/#abortcontroller

@@ +11,4 @@
>  
>    void abort();
> +  void follow(AbortSignal signal);
> +  void unfollow(AbortSignal signal);

I see these are removed in a later patch.

::: dom/webidl/AbortSignal.webidl
@@ +5,5 @@
>   */
>  
>  [Exposed=(Window,Worker),
> + Func="AbortController::IsEnabled"]
> +interface AbortSignal : EventTarget {

Please link to:

https://dom.spec.whatwg.org/#abortsignal

::: dom/webidl/Request.webidl
@@ +53,3 @@
>  
>    [Func="FetchObserver::IsEnabled"]
>    ObserverCallback observe;

I guess FetchObserver is still a possibility, but not in the spec yet.

::: dom/workers/WorkerPrefs.h
@@ +41,5 @@
>  WORKER_SIMPLE_PREF("dom.requestcontext.enabled", RequestContextEnabled, REQUESTCONTEXT_ENABLED)
>  WORKER_SIMPLE_PREF("gfx.offscreencanvas.enabled", OffscreenCanvasEnabled, OFFSCREENCANVAS_ENABLED)
>  WORKER_SIMPLE_PREF("dom.webkitBlink.dirPicker.enabled", WebkitBlinkDirectoryPickerEnabled, DOM_WEBKITBLINK_DIRPICKER_WEBKITBLINK)
>  WORKER_SIMPLE_PREF("dom.netinfo.enabled", NetworkInformationEnabled, NETWORKINFORMATION_ENABLED)
> +WORKER_SIMPLE_PREF("dom.abortController.enabled", AbortControllerEnabled, FETCHCONTROLLER_ENABLED)

Is there a reason you didn't rename FETCHCONTROLLER_ENABLED?
Attachment #8899729 - Flags: review?(bkelly) → review+
Comment on attachment 8899730 [details] [diff] [review]
part 3 - Removing the following algorithm

Review of attachment 8899730 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/abort/AbortSignal.cpp
@@ +99,5 @@
>    Unfollow();
>  }
>  
>  void
>  AbortSignal::Follower::Follow(AbortSignal* aSignal)

Is this Follower type used internally, but no longer exposed to script?
Attachment #8899730 - Flags: review?(bkelly) → review+
Attachment #8899732 - Flags: review?(bkelly) → review+
Comment on attachment 8899733 [details] [diff] [review]
part 5 - RequestInit.signal must be optional

Review of attachment 8899733 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/webidl/Request.webidl
@@ +48,5 @@
>    RequestRedirect redirect;
>    DOMString integrity;
>  
>    [Func="AbortController::IsEnabledInFetch"]
> +  AbortSignal? signal;

The spec proposal here does not make this change.  Why is it necessary?

https://fetch.spec.whatwg.org/branch-snapshots/cancelation/#requestinit
Attachment #8899733 - Flags: review?(bkelly) → review-
Comment on attachment 8899735 [details] [diff] [review]
part 6 - WPTs

Review of attachment 8899735 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.

::: modules/libpref/init/all.js
@@ +5059,5 @@
>  
> +// Abort API
> +#ifdef NIGHTLY_BUILD
> +pref("dom.abortController.enabled", true);
> +pref("dom.abortController.fetch.enabled", true);

This is inadequate because the tests will begin failing once it merges to beta.

I think you need a __dir__.ini file or two for the WPT tests that enables the prefs.
Attachment #8899735 - Flags: review?(bkelly) → review+
Comment on attachment 8899735 [details] [diff] [review]
part 6 - WPTs

Review of attachment 8899735 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/libpref/init/all.js
@@ +5062,5 @@
> +pref("dom.abortController.enabled", true);
> +pref("dom.abortController.fetch.enabled", true);
> +#else
> +pref("dom.abortController.enabled", false);
> +pref("dom.abortController.fetch.enabled", false);

Also, please file a follow-up meta bug for letting abort ride the trains.
Comment on attachment 8900629 [details] [diff] [review]
part 7 - Request.signal

Review of attachment 8900629 [details] [diff] [review]:
-----------------------------------------------------------------

I think we need to fix the subtle spec issue with override the signal with null here. See this comment:

https://github.com/whatwg/fetch/pull/523/files#r135109736

::: dom/fetch/Fetch.h
@@ +230,5 @@
>      mFetchStreamReader = nullptr;
>    }
>  
> +  virtual AbortSignal*
> +  SignalOrNull() const = 0;

nit: Any reason not to use `GetSignal()` which generally implies nullability?

::: dom/fetch/Request.cpp
@@ +660,5 @@
> +AbortSignal*
> +Request::Signal()
> +{
> +  if (!mSignal) {
> +    mSignal = new AbortSignal(false);

I don't think this is right.  Step 31 here suggests the signal can be null:

https://fetch.spec.whatwg.org/branch-snapshots/cancelation/#dom-request

It seems this can happen because of something like this:

  const r = new Request(otherRequest, { signal });

We should probably init AbortSignal() early and then let the RequestInit override it with null if it wants.

::: dom/fetch/Request.h
@@ +156,5 @@
>      return mRequest->GetPrincipalInfo();
>    }
>  
> +  AbortSignal*
> +  Signal();

nit: Maybe rename this to GetOrCreateSignal() to indicate that it can allocate the signal.

@@ +161,5 @@
> +
> +  AbortSignal*
> +  SignalOrNull() const override
> +  {
> +    return mSignal;

nit: Please keep method bodies in the cpp file.

::: dom/webidl/Request.webidl
@@ +27,5 @@
>    readonly attribute RequestRedirect redirect;
>    readonly attribute DOMString integrity;
>  
> +  [Func="AbortController::IsEnabledInFetch"]
> +  readonly attribute AbortSignal signal;

I thought this was exposed in a previous patch in this bug?

::: dom/workers/ServiceWorkerPrivate.cpp
@@ +1601,5 @@
>      nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(globalObj.GetAsSupports());
>      if (NS_WARN_IF(!global)) {
>        return false;
>      }
> +    RefPtr<Request> request = new Request(global, internalReq, nullptr);

This is wrong.  We need to propagate the outer channel being cancelled through the `Request.signal`.  This can be a follow-up, but we definitely need a comment here and a bug filed blocking riding the trains.
Attachment #8900629 - Flags: review?(bkelly) → review-
Attachment #8900632 - Flags: review?(bkelly) → review+
Comment on attachment 8900669 [details] [diff] [review]
part 9 - abort streams

Review of attachment 8900669 [details] [diff] [review]:
-----------------------------------------------------------------

r- mainly because I don't understand why we only start following and abort the readable stream in the body getter.  It seems the spec wants us to follow the signal much earlier.  This would be observable to js readable stream.  It should be errored even if the Response.body getter is never accessed.

::: dom/abort/AbortSignal.h
@@ +35,5 @@
>  
> +    bool
> +    IsFollowing() const
> +    {
> +      return !!mFollowingSignal;

nit: Please keep method bodies in the cpp file.

::: dom/fetch/Fetch.cpp
@@ +1160,4 @@
>    if (mReadableStreamBody) {
> +    if (aborted) {
> +      JS::Rooted<JSObject*> body(aCx, mReadableStreamBody);
> +      AbortStream(aCx, body);

I don't see the spec proposal doing this in the body getter.   Why are we doing this here?

@@ +1314,5 @@
> +{
> +  MOZ_ASSERT(mReadableStreamBody);
> +
> +  AutoJSAPI jsapi;
> +  if (!jsapi.Init(mOwner)) {

I still feel this should be global that owns the mReadableStreamBody, not the Response/Request itself.  Maybe this is addressed by bug 1385890.
Attachment #8900669 - Flags: review?(bkelly) → review-
Comment on attachment 8900729 [details] [diff] [review]
part 10 - signals of signals

Review of attachment 8900729 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/abort/AbortSignal.h
@@ +15,5 @@
>  class AbortController;
>  class AbortSignal;
>  
> +// This class must be implemented by objects who want to follow a AbortSignal.
> +class AbortFollower

This rename could have been in a separate patch.  But I think its ok.
Attachment #8900729 - Flags: review?(bkelly) → review+
Attachment #8900764 - Flags: review?(bkelly) → review+
Attachment #8901102 - Flags: review?(bkelly) → review+
In addition to the comments here I think the follow-up bug to let this ride the train should block on passing all WPT possible.

Also, I think there should also be a WPT test that verifies an outer fetch() abort is proagated to an intercepted FetchEvent.request.signal properly.
(Assignee)

Updated

2 years ago
Blocks: 1394085
(Assignee)

Comment 30

2 years ago
> I think we need to fix the subtle spec issue with override the signal with
> null here. See this comment:
> 
> https://github.com/whatwg/fetch/pull/523/files#r135109736

This is patch 10.

> > +    mSignal = new AbortSignal(false);
> 
> I don't think this is right.  Step 31 here suggests the signal can be null:

Yes, this is patch 10.

> ::: dom/workers/ServiceWorkerPrivate.cpp
> @@ +1601,5 @@
> >      nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(globalObj.GetAsSupports());
> >      if (NS_WARN_IF(!global)) {
> >        return false;
> >      }
> > +    RefPtr<Request> request = new Request(global, internalReq, nullptr);
> 
> This is wrong.  We need to propagate the outer channel being cancelled
> through the `Request.signal`.  This can be a follow-up, but we definitely
> need a comment here and a bug filed blocking riding the trains.

How can be this Request.signal aborted if AbortController is not exposed?

All the other comments are applied.
Flags: needinfo?(bkelly)
(In reply to Andrea Marchesini [:baku] from comment #30)
> > ::: dom/workers/ServiceWorkerPrivate.cpp
> > @@ +1601,5 @@
> > >      nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(globalObj.GetAsSupports());
> > >      if (NS_WARN_IF(!global)) {
> > >        return false;
> > >      }
> > > +    RefPtr<Request> request = new Request(global, internalReq, nullptr);
> > 
> > This is wrong.  We need to propagate the outer channel being cancelled
> > through the `Request.signal`.  This can be a follow-up, but we definitely
> > need a comment here and a bug filed blocking riding the trains.
> 
> How can be this Request.signal aborted if AbortController is not exposed?

It can be aborted in a couple ways:

1. If a navigation is intercepted then FetchEvent.signal should reflect if that original nsIChannel is cancelled.  This can happen if the LoadGroup is canceled due to the window be closed, etc.  This should propagate through the FetchEvent.request.signal.

2. A fwindow can provide a controller which it can use to abort a fetch() call it makes.  If this fetch() is intercepted by a SW then that abort should propagate to the corresponding FetchEvent.request.signal.  The spec does this by saying FetchEvent.request is the same as the window's Request.  Our implementation goes from Request->nsIChannel->Request so its easy for us to lose data elements like this.  We need to make sure they propagate correctly.

Does that make sense?  Thanks.
Flags: needinfo?(bkelly)
(Assignee)

Updated

2 years ago
Blocks: 1394102
(Assignee)

Updated

2 years ago
Attachment #8899733 - Attachment is obsolete: true
(Assignee)

Comment 32

2 years ago
If you prefer I can merge part 10 and this one, but probably we can keep them separate.
Attachment #8900629 - Attachment is obsolete: true
Attachment #8901446 - Flags: review?(bkelly)
(Assignee)

Comment 33

2 years ago
Attachment #8900669 - Attachment is obsolete: true
Attachment #8901447 - Flags: review?(bkelly)
Comment on attachment 8901446 [details] [diff] [review]
part 7 - Request.signal

Review of attachment 8901446 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/ServiceWorkerPrivate.cpp
@@ +1601,5 @@
>      nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(globalObj.GetAsSupports());
>      if (NS_WARN_IF(!global)) {
>        return false;
>      }
> +    RefPtr<Request> request = new Request(global, internalReq, nullptr);

Please add a comment pointing to bug 1394102.
Attachment #8901446 - Flags: review?(bkelly) → review+
Attachment #8901447 - Flags: review?(bkelly) → review+

Comment 35

2 years ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/81c100a42bb6
AbortSignal/AbortController - part 1 - Moving FetchController/FetchSignal into dom/abort, r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5c6f95530f2
AbortSignal/AbortController - part 2 - Renaming FetchController/FetchSignal, r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/203ad2fab9aa
AbortSignal/AbortController - part 3 - Removing the following algorithm, r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/bded779447b2
AbortSignal/AbortController - part 4 - Separate pref for AbortSignal/AbortController, r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/6cbeeec89846
AbortSignal/AbortController - part 5 - Some WPTs pass, r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c6f49d73935
AbortSignal/AbortController - part 6 - Implement Request.signal, r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f2733a59971
AbortSignal/AbortController - part 7 - Fix a WPT related to AbortSignal and Fetch, r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/64d0dbf9706b
AbortSignal/AbortController - part 8 - Aborting ReadableStream when AbortSignal is aborted, r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c41e61d6763
AbortSignal/AbortController - part 9 - Request.signal should not be a reference of RequestInit.signal, r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/68192e795b74
AbortSignal/AbortController - part 10 - Reject the fetch() promise if AbortSignal is already aborted, r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f02d9b8e2ec
AbortSignal/AbortController - part 11 - More WPTs pass, r=bkelly
Backed out for linting failures, e.g. in file_abort_controller.html:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d133ea537108e8b7f9c23d951cfd288c964cf6c6
https://hg.mozilla.org/integration/mozilla-inbound/rev/591f78a8d99d548de1a96fad78c4022454cce4a6
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c5a1bc02ad95ab418269054aae119cfa77fa7e5
https://hg.mozilla.org/integration/mozilla-inbound/rev/101500f732af4bd440c3da45ef4a23c9694d3a16
https://hg.mozilla.org/integration/mozilla-inbound/rev/550f8e4e38de40a663fc4025055250cfb473a9fc
https://hg.mozilla.org/integration/mozilla-inbound/rev/cdc1e65e113ff7dcca703567f2a1fb92b86e3021
https://hg.mozilla.org/integration/mozilla-inbound/rev/a47c8ac7bc41f4b744f4f9917a03817ebc7df1ee
https://hg.mozilla.org/integration/mozilla-inbound/rev/22b6d264d711ed5060160966a277c835686ace80
https://hg.mozilla.org/integration/mozilla-inbound/rev/cccc590388390ccd8f3ebd9c6a53480c51efb6fb
https://hg.mozilla.org/integration/mozilla-inbound/rev/5dbdbf25a9f35b0ec1bbf180f933a7ae0757ef18
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad9b9c71e04bef1db50a913b154730cd16905edb

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=4f02d9b8e2ecd4f1f70fd7e9c31575c8ac16e2b5&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=126647786&repo=mozilla-inbound

[task 2017-08-29T05:46:32.056885Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/dom/abort/tests/file_abort_controller.html:14:16 | 'AbortController' is not defined. (no-undef)
and more
Flags: needinfo?(amarchesini)
I just saw the linting failure mentioned on IRC - since dom/tests isn't covered for lint currently, and you're just moving the tests out, feel free to add a line to the .eslintignore file within the other dom/ lines to ignore the new directory.

I don't think it would be too hard to fix the linting if you want to do that, but if this bug needs to get landed I'm happy for it to be ignored (and we'll get a bug on it later to add linting).

If you want to enable linting, I think it'd be something like:

- add a .eslintrc file to the dom/abort/tests directory which points to "mozilla/mochitest-test" as an extends (there's plenty of other examples of this in test directories)
- Add 'AbortController' and 'AbortSignal' as globals in tools/lint/eslint/eslint-plugin-mozilla/libs/configs/recommended.js
- Fix the remaining issues, I think using `./mach lint -l eslint --fix` will fix the rest of them.

Comment 38

2 years ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec2a82e0ceeb
AbortSignal/AbortController - part 1 - Moving FetchController/FetchSignal into dom/abort, r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/08be9ade792d
AbortSignal/AbortController - part 2 - Renaming FetchController/FetchSignal, r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/17e504194d2f
AbortSignal/AbortController - part 3 - Removing the following algorithm, r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/a76e55d78a27
AbortSignal/AbortController - part 4 - Separate pref for AbortSignal/AbortController, r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe32dbffb2c8
AbortSignal/AbortController - part 5 - Some WPTs pass, r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/938b1b7d7829
AbortSignal/AbortController - part 6 - Implement Request.signal, r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/d07e9dd05709
AbortSignal/AbortController - part 7 - Fix a WPT related to AbortSignal and Fetch, r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ccd7926f66b
AbortSignal/AbortController - part 8 - Aborting ReadableStream when AbortSignal is aborted, r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b10392f26bf
AbortSignal/AbortController - part 9 - Request.signal should not be a reference of RequestInit.signal, r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/52519801d0bc
AbortSignal/AbortController - part 10 - Reject the fetch() promise if AbortSignal is already aborted, r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/44205774c871
AbortSignal/AbortController - part 11 - More WPTs pass, r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/92692c9a4f1e
AbortSignal/AbortController - part 12 - eslint for dom/abort tests, r=me
(Assignee)

Updated

2 years ago
Flags: needinfo?(amarchesini)
Depends on: 1394622
Depends on: 1394676
Depends on: 1395005
Depends on: 1395006
(Assignee)

Updated

2 years ago
Blocks: 1395141
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1349950
Hi Chris,

Is there any chance we could bump documenting the changes here up in priority?  Jake is getting ready to publish a blog post about abortable fetch and it would be nice if MDN docs reflected the current API.

The big changes here are:

  FetchSignal changed to AbortSignal
  FetchController changed to AbortController

Also, we are leaving the FetchObserver stuff turned off for now.  The spec work on that has stalled and its unclear if/when it will happen.

Thanks!
Flags: needinfo?(cmills)
(In reply to Ben Kelly [:bkelly] from comment #41)
> Hi Chris,
> 
> Is there any chance we could bump documenting the changes here up in
> priority?  Jake is getting ready to publish a blog post about abortable
> fetch and it would be nice if MDN docs reflected the current API.
> 
> The big changes here are:
> 
>   FetchSignal changed to AbortSignal
>   FetchController changed to AbortController
> 
> Also, we are leaving the FetchObserver stuff turned off for now.  The spec
> work on that has stalled and its unclear if/when it will happen.
> 
> Thanks!

Hi Ben,

Sorry for the delay on getting this done — I have got an update plan written out, but things keep coming up. This week is a bit more freed up however; I'll get this done in the next couple of days.
Flags: needinfo?(cmills)
Thanks!  FWIW, here is the article Jake wrote for the google site:

https://developers.google.com/web/updates/2017/09/abortable-fetch
I have updated the documentation for this; I'd love a quick tech review if you have the time - let me know if there's anything I've missed or misunderstood.

I have updated all the API ref pages to their new names, URLs, etc.:

https://developer.mozilla.org/en-US/docs/Web/API/AbortController (and child pages)
https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal (and child pages)
https://developer.mozilla.org/en-US/docs/Web/Events/abort_(dom_abort_api)

I have deleted the observer stuff for now; if the spec work does start to move again, we can always reinstate the pages.

I have updated the simple demo I wrote, and it seems to work fine:

* https://github.com/mdn/dom-examples/tree/master/abort-api
* https://mdn.github.io/dom-examples/abort-api/

I have updated the relevant fetch pages:

* https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API (to remove mention of the abort interfaces, as they are not part of this API)
* https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/fetch (to update the signal option description)

I have removed it from the experimental features page as it is now on by default, and added a note to the Fx57 rel notes:

* https://developer.mozilla.org/en-US/Firefox/Releases/57#New_APIs
* https://developer.mozilla.org/en-US/Firefox/Experimental_features

Note that the fetch and dom reference page sidebars will currently be wrong, but I've submitted a PR to fix those:

* https://github.com/mdn/kumascript/pull/338
Keywords: dev-doc-needed → dev-doc-complete
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #44)
> I have updated all the API ref pages to their new names, URLs, etc.:
> 
> https://developer.mozilla.org/en-US/docs/Web/API/AbortController (and child
> pages)
> https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal (and child
> pages)

Not sure how we handle this kind of thing, but it seems its implemented/fixed in a pre-release version of edge.  Not sure if its worth mentioning that in the versions support table.

Also, it might be good to make the examples show that the fetch() rejects with AbortError.

> https://developer.mozilla.org/en-US/docs/Web/Events/abort_(dom_abort_api)

This link seems broken?  What were you trying to link here?

> I have updated the relevant fetch pages:
> 
> * https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API (to remove
> mention of the abort interfaces, as they are not part of this API)

I think it would be good to at least link to the Abort API here.  Being able to stop a fetch() is an important operation many people have been looking for.  Its also something that XHR supports.  I think they will expect it to be mentioned here.

Thanks!
(In reply to Ben Kelly [:bkelly] from comment #45)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #44)
> > I have updated all the API ref pages to their new names, URLs, etc.:
> > 
> > https://developer.mozilla.org/en-US/docs/Web/API/AbortController (and child
> > pages)
> > https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal (and child
> > pages)
> 
> Not sure how we handle this kind of thing, but it seems its
> implemented/fixed in a pre-release version of edge.  Not sure if its worth
> mentioning that in the versions support table.

I couldn't find much information on this, except for a bug report saying that they are working on it. I think I'll leave this for the Edge folk to add, unless I can find more info about it.

> 
> Also, it might be good to make the examples show that the fetch() rejects
> with AbortError.

I've added a note to the pages to make this clear.

> 
> > https://developer.mozilla.org/en-US/docs/Web/Events/abort_(dom_abort_api)
> 
> This link seems broken?  What were you trying to link here?

Ah dammnit, bugzilla hasn't included the closing paren as part of the link, which means that it has broken. This is a link to a reference page for the actual abort event. The bit in parens is for disambiguation, as there are other types on abort event on the web platform.

> 
> > I have updated the relevant fetch pages:
> > 
> > * https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API (to remove
> > mention of the abort interfaces, as they are not part of this API)
> 
> I think it would be good to at least link to the Abort API here.  Being able
> to stop a fetch() is an important operation many people have been looking
> for.  Its also something that XHR supports.  I think they will expect it to
> be mentioned here.

Cool, good idea. I've added this:

https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API#Aborting_a_fetch

I will have to be mindful to add similar links to documentation for other abortable operations, once available.

Is XHR already abortable (in Fx at least?) The XHR constructor doesn't take any parameters, so I was wondering how you attach the signal to the object in that case.

Updated

a year ago
Depends on: 1440941
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.