- From: Benjamin Gruenbaum <notifications@github.com>
- Date: Mon, 01 Feb 2016 23:54:46 -0800
- To: whatwg/fetch <fetch@noreply.github.com>
- Message-ID: <whatwg/fetch/issues/27/178431783@github.com>
@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