Re: [fetch] Aborting a fetch (#27)

@jan-ivar 

>  Here you're cancelling at the intersection point. Of course that works. The problem is downstream. 

That's not the use case you describe in the comment you linked to though - it fixes that. I think it would be helpful to look at more code - got any motivating examples where cancellation wouldn't work?

There's https://github.com/whatwg/fetch/issues/27#issuecomment-87134228 but that's obviously not a very good example since bluebird semantics would work there. A bigger issue is that it contains incorrect code and has an inherent race condition for caching a flag and not a promise, more than one invocation will call `innocuousActivity` more than once - cancellation wouldn't make sense anyway. A correct way to write it is:

```js
Obj.prototype.doSomethingInnocuous = function() { 
  if(this.p) return this.p; 
  return (this.p = innocuousActivity(t)); // cache the promise, not the value
}
```

Now, let's argue for a minute "but people can still write code that looks like the original example" AND fork one level up and not cancel the actual `fetch`. Something like: 

```js
Obj.prototype.doSomethingInnocuous = function(t) { 
  if (this.alreadyRun) { return Promise.resolve(); } // OK with race condition.
  return fetch(commitUrl, {method: 'POST'})).then(x => this.alreadyRun = true);
}
var commitFetch = obj.doSomethingInnocuous()
  .then(showResult);
cancelButton.onclick = e => commitFetch.abort();
```

That would _still_ work since there is only one subscriber - let's see if we can contrive it a little further:

```js
Obj.prototype.doSomethingInnocuous = function(t) { 
  if (this.alreadyRun) { return Promise.resolve(); } // OK with race condition.
  var wizard = fetch(commitUrl, {method: 'POST'}));
  wizard.then(x => x.alreadyRun = true); // contrive it by forking the chain for side effects
  return wizard.then(x => x); // again, forking here is contrived, but let's do it anyway.
}
var commitFetch = obj.doSomethingInnocuous()
  .then(showResult);
cancelButton.onclick = e => commitFetch.abort();
```

Here we have contrived the use case enough for cancellation to not work. Indeed this can be a problem. The code in doSomethingInnocuous would have to call `abort` on the chain in the part that does strict side effects.

No let's say for a second this more contrived use case is plausible in real code - this is still way way safer than abort semantics - with abort semantics both ends of the fork would get rejected and you have an unhandled error condition you're never dealing with and an error to the console - code dealing with a network error and retrying the fetch for example would run (since `catch` clauses run) and users would have to guard explicitly against that.

On the other hand, a simple warning (like with unhandled promise rejections) can solve this pretty nicely - although in all honesty I've never had issues with cancellation counting semantics. 

---
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/fetch/issues/27#issuecomment-178431783

Received on Tuesday, 2 February 2016 07:55:19 UTC