Issue with canceling XHR request using promises - fixed #12525 #12657

netman92 · 2015-08-23T11:08:38Z

@jtheoof no conflicts, is it ok now?

jtheoof · 2015-08-23T14:31:30Z

👍 Although the commit message does not seem to follow the angular commit message conventions. I would preprend the commit the message with: fix(ngResource): ...

netman92 · 2015-08-23T14:38:21Z

@jtheoof fixed

caitp · 2015-08-23T17:16:44Z

src/ngResource/resource.js

@@ -568,8 +568,10 @@ angular.module('ngResource', ['ng']).
              undefined;

            forEach(action, function(value, key) {
-              if (key != 'params' && key != 'isArray' && key != 'interceptor') {
+              if (key != 'params' && key != 'isArray' && key != 'interceptor' && key != 'timeout') {


What if we just made this a switch statement for readability?

@caitp Done

jtheoof · 2015-08-24T13:57:25Z

👍

caitp · 2015-08-24T16:19:39Z

test/ngResource/resourceSpec.js

@@ -1356,5 +1357,36 @@ describe('resource', function() {
      );
  });

+  it('If timeout promise is resolved, cancel the request', function() {


nit: can you change the test description to match the grammar of other ones in the file?

@caitp done

caitp · 2015-08-24T16:20:13Z

Other than the nit it looks great to me. Would be good if @gkalpak or @Narretz would take a look as well before merging

gkalpak · 2015-08-24T17:38:26Z

test/ngResource/resourceSpec.js

+    CreditCard.query();
+
+    canceler.resolve();
+    expect(function() { $httpBackend.flush();}).toThrow(new Error("No pending request to flush !"));


Nit: expect($httpBackend.flush).toThrow(...); is more concise and readable (here and below).
(Else you need a space after flush();.)

If it (flush) uses this at all, it's not an option anyways

@gkalpak fixed

gkalpak · 2015-08-24T17:44:31Z

test/ngResource/resourceSpec.js

+    });
+
+    CreditCard.query();
+    expect(function() { $httpBackend.flush();}).not.toThrow(new Error("No pending request to flush !"));


I would leave it at .not.toThrow() (to account for the unlikely situation where the error message changes and the behaviour breaks (simultaneously)).
It's also more concise and readable.

gkalpak · 2015-08-24T17:49:25Z

LGTM 👍
_{TBH, I don't find it very useful, since having to recreate the resource class everytime a timeout promise is resolved doesn't seem very practical. But if people are using it, it's fine by me 😃}

netman92 · 2015-08-25T10:19:05Z

@caitp Can you merge it?

Narretz · 2015-08-25T11:14:18Z

What @gkalpak said seems like the behavior is not well defined. Do we need to update the docs? Also, the commits must be squashed and the commit message doesn't follow our guidelines.

caitp · 2015-08-25T11:19:22Z

the behaviour seems pretty well defined to me @Narretz. It would be cool if you could override it on a per-request basis, but hey.

Some time ago, I had proposed that we allow letting you specify a factory function to produce the timeout promise, I can't remember if that's been done yet or not. That would solve the usability concerns.

@netman92 can you update the docs to indicate that the timeout parameter can be specified during initial resource class construction? Then I'll squash and merge

netman92 · 2015-08-25T11:26:19Z

@caitp https://docs.angularjs.org/api/ngResource/service/$resource
and param timeout

timeout – {number|Promise} – timeout in milliseconds, or promise that should abort the request when resolved.

I think, it is already in docs.

caitp · 2015-08-25T13:19:19Z

Not really, it's not an accurate depiction of what happens

netman92 · 2015-08-31T09:17:25Z

@caitp Sorry, but I do not know, what to add to docs.

jtheoof · 2015-08-31T14:36:56Z

I agree with @netman92. The documentation seems correct, the behaviour was incorrect in a sense that the Promise was copied, therefore not cancellable. I think it's good to merge.

jvorcak · 2015-09-10T08:37:21Z

The documentation seems correct to me as well. Do you think it will be merged eventually @caitp? I'd really need this for one of my apps.

Closes #12657 Closes #12675 Closes #10890 Closes #9332

gkalpak · 2015-10-29T08:15:12Z

You know what they say: It's never too late, right ?)
Well, I just realized there is an issue with this change:

If the user fails to re-instantiate the resource after having cancelled a request (i.e. after having resolved the timeout promise), all subsequent requests will fail (and this behaviour isn't even documented).
You can see it in action here (with the current master).

IMO, we should come up with a better approach to request cancelling for $resource (let's discuss in #13058).
At the very least, a warning should be added in the docs and a BC notice in the changelog (but I would prefer fixing the behaviour instead).

petebacondarwin · 2015-10-29T08:56:21Z

I agree - let's expedite #13058

gkalpak · 2015-11-18T08:32:34Z

#13058 has been superseeded by #13210.

…ning Promises never worked correctly as values for `timeout` in `$resource`, because the same value has to be re-used for multiple requests (and it is not possible to `angular.copy()` a promise). Now (in addition to ignoring a non-numeric `timeout`), a warning is logged to the console using `$log.debug()`. Partly fixes #13393. BREAKING CHANGE: Possible breaking change for users who updated their code to provide a `timeout` promise for a `$resource` request in version 1.4.8. Up to v1.4.7 (included), using a promise as a timeout in `$resource`, would silently fail (i.e. have no effect). In v1.4.8, using a promise as timeout would have the (buggy) behaviour described in #12657 (comment) (i.e. it will work as expected for the first time you resolve the promise and will cancel all subsequent requests after that - one has to re-create the resource class. This is feature was not documented.) With this change, using a promise as timeout in 1.4.9 onwsards is not allowed. It will log a warning and ignore the timeout value. If you need support for cancellable `$resource` actions, you should upgrade to version 1.5 or higher.

<a name="1.4.9"></a> # 1.4.9 implicit-superannuation (2016-01-21) ## Bug Fixes - **Animation** - ensure that animate promises resolve when the document is hidden ([9a60408c](angular/angular.js@9a60408)) - do not trigger animations if the document is hidden ([09f6061a](angular/angular.js@09f6061), [elastic#12842](angular/angular.js#12842), [elastic#13776](angular/angular.js#13776)) - only copy over the animation options once ([2fc954d3](angular/angular.js@2fc954d), [elastic#13722](angular/angular.js#13722), [elastic#13578](angular/angular.js#13578)) - allow event listeners on document in IE ([5ba4419e](angular/angular.js@5ba4419), [elastic#13548](angular/angular.js#13548), [elastic#13696](angular/angular.js#13696)) - allow removing classes that are added by a running animation ([6c4581fc](angular/angular.js@6c4581f), [elastic#13339](angular/angular.js#13339), [elastic#13380](angular/angular.js#13380), [elastic#13414](angular/angular.js#13414), [elastic#13472](angular/angular.js#13472), [elastic#13678](angular/angular.js#13678)) - do not use `event.timeStamp` anymore for time tracking ([620a20d1](angular/angular.js@620a20d), [elastic#13494](angular/angular.js#13494), [elastic#13495](angular/angular.js#13495)) - ignore children without animation data when closing them ([be01cebf](angular/angular.js@be01ceb), [elastic#11992](angular/angular.js#11992), [elastic#13424](angular/angular.js#13424)) - do not alter the provided options data ([7a81e6fe](angular/angular.js@7a81e6f), [elastic#13040](angular/angular.js#13040), [elastic#13175](angular/angular.js#13175)) - correctly handle `$animate.pin()` host elements ([a985adfd](angular/angular.js@a985adf), [elastic#13783](angular/angular.js#13783)) - allow animations when pinned element is parent element ([4cb8ac61](angular/angular.js@4cb8ac6), [elastic#13466](angular/angular.js#13466)) - allow enabled children to animate on disabled parents ([6d85f24e](angular/angular.js@6d85f24), [elastic#13179](angular/angular.js#13179), [elastic#13695](angular/angular.js#13695)) - correctly access `minErr` ([0c1b54f0](angular/angular.js@0c1b54f)) - ensure animate runner is the same with and without animations ([937942f5](angular/angular.js@937942f), [elastic#13205](angular/angular.js#13205), [elastic#13347](angular/angular.js#13347)) - remove animation end event listeners on close ([d9157849](angular/angular.js@d915784), [elastic#13672](angular/angular.js#13672)) - consider options.delay value for closing timeout ([592bf516](angular/angular.js@592bf51), [elastic#13355](angular/angular.js#13355), [elastic#13363](angular/angular.js#13363)) - **$controller:** allow identifiers containing `$` ([2563ff7b](angular/angular.js@2563ff7), [elastic#13736](angular/angular.js#13736)) - **$http:** throw if url passed is not a string ([c5bf9dae](angular/angular.js@c5bf9da), [elastic#12925](angular/angular.js#12925), [elastic#13444](angular/angular.js#13444)) - **$parse:** handle interceptors with `undefined` expressions ([7bb2414b](angular/angular.js@7bb2414)) - **$resource:** don't allow using promises as `timeout` and log a warning ([47486524](angular/angular.js@4748652)) - **formatNumber:** cope with large and small number corner cases ([9c49eb13](angular/angular.js@9c49eb1), [elastic#13394](angular/angular.js#13394), [elastic#8674](angular/angular.js#8674), [elastic#12709](angular/angular.js#12709), [elastic#8705](angular/angular.js#8705), [elastic#12707](angular/angular.js#12707), [elastic#10246](angular/angular.js#10246), [elastic#10252](angular/angular.js#10252)) - **input:** - fix URL validation being too strict ([6610ae81](angular/angular.js@6610ae8), [elastic#13528](angular/angular.js#13528), [elastic#13544](angular/angular.js#13544)) - add missing chars to URL validation regex ([2995b54a](angular/angular.js@2995b54), [elastic#13379](angular/angular.js#13379), [elastic#13460](angular/angular.js#13460)) - **isArrayLike:** recognize empty instances of an Array subclass ([323f9ab7](angular/angular.js@323f9ab), [elastic#13560](angular/angular.js#13560), [elastic#13708](angular/angular.js#13708)) - **ngInclude:** do not compile template if original scope is destroyed ([9590bcf0](angular/angular.js@9590bcf)) - **ngOptions:** - don't skip `optgroup` elements with `value === ''` ([85e392f3](angular/angular.js@85e392f), [elastic#13487](angular/angular.js#13487), [elastic#13489](angular/angular.js#13489)) - don't `$dirty` multiple select after compilation ([f163c905](angular/angular.js@f163c90), [elastic#13211](angular/angular.js#13211), [elastic#13326](angular/angular.js#13326)) - **select:** re-define `ngModelCtrl.$render` in the `select` directive's postLink function ([529b2507](angular/angular.js@529b250), [elastic#13583](angular/angular.js#13583), [elastic#13583](angular/angular.js#13583), [elastic#13663](angular/angular.js#13663)) ## Minor Features - **ngLocale:** add support for standalone months ([54c4041e](angular/angular.js@54c4041), [elastic#3744](angular/angular.js#3744), [elastic#10247](angular/angular.js#10247), [elastic#12642](angular/angular.js#12642), [elastic#12844](angular/angular.js#12844)) - **ngMock:** add support for `$animate.closeAndFlush()` ([512c0811](angular/angular.js@512c081)) ## Performance Improvements - **ngAnimate:** speed up `areAnimationsAllowed` check ([2d3303dd](angular/angular.js@2d3303d)) ## Breaking Changes While we do not deem the following to be a real breaking change we are highlighting it here in the changelog to ensure that it does not surprise anyone. - **$resource:** due to [47486524](angular/angular.js@4748652), **Possible breaking change** for users who updated their code to provide a `timeout` promise for a `$resource` request in version v1.4.8. Up to v1.4.7 (included), using a promise as a timeout in `$resource`, would silently fail (i.e. have no effect). In v1.4.8, using a promise as timeout would have the (buggy) behaviour described in angular/angular.js#12657 (comment). (I.e. it will work as expected for the first time you resolve the promise and will cancel all subsequent requests after that - one has to re-create the resource class. This was not documented.) With this change, using a promise as timeout in v1.4.9 onwards is not allowed. It will log a warning and ignore the timeout value. If you need support for cancellable `$resource` actions, you should upgrade to version 1.5 or higher. <a name="1.4.8"></a> # 1.4.8 ice-manipulation (2015-11-19) ## Bug Fixes - **$animate:** ensure leave animation calls `close` callback ([6bd6dbff](angular/angular.js@6bd6dbf), [elastic#12278](angular/angular.js#12278), [elastic#12096](angular/angular.js#12096), [elastic#13054](angular/angular.js#13054)) - **$cacheFactory:** check key exists before decreasing cache size count ([2a5a52a7](angular/angular.js@2a5a52a), [elastic#12321](angular/angular.js#12321), [elastic#12329](angular/angular.js#12329)) - **$compile:** - bind all directive controllers correctly when using `bindToController` ([5d8861fb](angular/angular.js@5d8861f), [elastic#11343](angular/angular.js#11343), [elastic#11345](angular/angular.js#11345)) - evaluate against the correct scope with bindToController on new scope ([b9f7c453](angular/angular.js@b9f7c45), [elastic#13021](angular/angular.js#13021), [elastic#13025](angular/angular.js#13025)) - fix scoping of transclusion directives inside replace directive ([74da0340](angular/angular.js@74da034), [elastic#12975](angular/angular.js#12975), [elastic#12936](angular/angular.js#12936), [elastic#13244](angular/angular.js#13244)) - **$http:** apply `transformResponse` even when `data` is empty ([c6909464](angular/angular.js@c690946), [elastic#12976](angular/angular.js#12976), [elastic#12979](angular/angular.js#12979)) - **$location:** ensure `$locationChangeSuccess` fires even if URL ends with `#` ([6f8ddb6d](angular/angular.js@6f8ddb6), [elastic#12175](angular/angular.js#12175), [elastic#13251](angular/angular.js#13251)) - **$parse:** evaluate once simple expressions only once ([e4036824](angular/angular.js@e403682), [elastic#12983](angular/angular.js#12983), [elastic#13002](angular/angular.js#13002)) - **$resource:** allow XHR request to be cancelled via a timeout promise ([7170f9d9](angular/angular.js@7170f9d), [elastic#12657](angular/angular.js#12657), [elastic#12675](angular/angular.js#12675), [elastic#10890](angular/angular.js#10890), [elastic#9332](angular/angular.js#9332)) - **$rootScope:** prevent IE9 memory leak when destroying scopes ([87b0055c](angular/angular.js@87b0055), [elastic#10706](angular/angular.js#10706), [elastic#11786](angular/angular.js#11786)) - **Angular.js:** fix `isArrayLike` for unusual cases ([70edec94](angular/angular.js@70edec9), [elastic#10186](angular/angular.js#10186), [elastic#8000](angular/angular.js#8000), [elastic#4855](angular/angular.js#4855), [elastic#4751](angular/angular.js#4751), [elastic#10272](angular/angular.js#10272)) - **isArrayLike:** handle jQuery objects of length 0 ([d3da55c4](angular/angular.js@d3da55c)) - **jqLite:** - deregister special `mouseenter` / `mouseleave` events correctly ([22f66025](angular/angular.js@22f6602), [elastic#12795](angular/angular.js#12795), [elastic#12799](angular/angular.js#12799)) - ensure mouseenter works with svg elements on IE ([c1f34e8e](angular/angular.js@c1f34e8), [elastic#10259](angular/angular.js#10259), [elastic#10276](angular/angular.js#10276)) - **limitTo:** start at 0 if `begin` is negative and exceeds input length ([4fc40bc9](angular/angular.js@4fc40bc), [elastic#12775](angular/angular.js#12775), [elastic#12781](angular/angular.js#12781)) - **merge:** - ensure that jqlite->jqlite and DOM->DOM ([2f8db1bf](angular/angular.js@2f8db1b)) - clone elements instead of treating them like simple objects ([838cf4be](angular/angular.js@838cf4b), [elastic#12286](angular/angular.js#12286)) - **ngAria:** don't add tabindex to radio and checkbox inputs ([59f1f4e1](angular/angular.js@59f1f4e), [elastic#12492](angular/angular.js#12492), [elastic#13095](angular/angular.js#13095)) - **ngInput:** change URL_REGEXP to better match RFC3987 ([cb51116d](angular/angular.js@cb51116), [elastic#11341](angular/angular.js#11341), [elastic#11381](angular/angular.js#11381)) - **ngMock:** reset cache before every test ([91b7cd9b](angular/angular.js@91b7cd9), [elastic#13013](angular/angular.js#13013)) - **ngOptions:** - skip comments and empty options when looking for options ([0f58334b](angular/angular.js@0f58334), [elastic#12190](angular/angular.js#12190), [elastic#13029](angular/angular.js#13029), [elastic#13033](angular/angular.js#13033)) - override select option registration to allow compilation of empty option ([7b2ecf42](angular/angular.js@7b2ecf4), [elastic#11685](angular/angular.js#11685), [elastic#12972](angular/angular.js#12972), [elastic#12968](angular/angular.js#12968), [elastic#13012](angular/angular.js#13012)) ## Performance Improvements - **$compile:** use static jquery data method to avoid creating new instances ([55ad192e](angular/angular.js@55ad192)) - **copy:** - avoid regex in `isTypedArray` ([19fab4a1](angular/angular.js@19fab4a)) - only validate/clear if the user specifies a destination ([d1293540](angular/angular.js@d129354), [elastic#12068](angular/angular.js#12068)) - **merge:** remove unnecessary wrapping of jqLite element ([ce6a96b0](angular/angular.js@ce6a96b), [elastic#13236](angular/angular.js#13236)) ## Breaking Changes Signed-off-by: Kevin Kirsche <[email protected]>

googlebot added the cla: yes label Aug 23, 2015

jtheoof mentioned this pull request Aug 23, 2015

Issue with canceling XHR request using promises - fixed #11035

Closed

netman92 force-pushed the allow-promise-cancelling branch from b1a5eba to 2955bbf Compare August 23, 2015 14:36

fix(ngResource): canceling XHR request using promise

2955bbf

caitp reviewed Aug 23, 2015
View reviewed changes

netman92 force-pushed the allow-promise-cancelling branch from 1c232a1 to 16f3f9d Compare August 23, 2015 18:22

caitp reviewed Aug 24, 2015
View reviewed changes

netman92 force-pushed the allow-promise-cancelling branch from 16f3f9d to 2ac00f5 Compare August 24, 2015 16:38

gkalpak reviewed Aug 24, 2015
View reviewed changes

netman92 force-pushed the allow-promise-cancelling branch from 2ac00f5 to e20af70 Compare August 24, 2015 17:43

gkalpak reviewed Aug 24, 2015
View reviewed changes

netman92 force-pushed the allow-promise-cancelling branch 2 times, most recently from e8437a2 to 3da5b53 Compare August 24, 2015 17:48

fix(ngResource): canceling XHR request using promise - refactor

3da5b53

Narretz added type: bug component: ngResource labels Sep 24, 2015

Narretz added this to the 1.4.7 milestone Sep 24, 2015

Narretz self-assigned this Sep 24, 2015

Narretz modified the milestones: 1.4.7, 1.4.8 Oct 1, 2015

petebacondarwin mentioned this pull request Oct 28, 2015

WIP - feat($resource): add support for cancelling requests using promises #13058

Closed

petebacondarwin pushed a commit that referenced this pull request Oct 28, 2015

fix($resource): allow XHR request to be cancelled via timeout promise

7170f9d

Closes #12657 Closes #12675 Closes #10890 Closes #9332

petebacondarwin closed this in 4fc7346 Oct 28, 2015

gkalpak mentioned this pull request Nov 27, 2015

fix($resource): remove (broken) support for _promise as timeout_ #13393

Closed

3 tasks

gkalpak mentioned this pull request Dec 8, 2015

fix($resource): remove (broken) support for promises as timeout #13462

Closed

kkirsche mentioned this pull request Feb 21, 2016

Update AngularJS to 1.4.9 elastic/kibana#6293

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue with canceling XHR request using promises - fixed #12525 #12657

Issue with canceling XHR request using promises - fixed #12525 #12657

netman92 commented Aug 23, 2015

jtheoof commented Aug 23, 2015

netman92 commented Aug 23, 2015

caitp Aug 23, 2015

netman92 Aug 23, 2015

jtheoof commented Aug 24, 2015

caitp Aug 24, 2015

netman92 Aug 24, 2015

caitp commented Aug 24, 2015

gkalpak Aug 24, 2015

caitp Aug 24, 2015

netman92 Aug 24, 2015

gkalpak Aug 24, 2015

gkalpak commented Aug 24, 2015

netman92 commented Aug 25, 2015

Narretz commented Aug 25, 2015

caitp commented Aug 25, 2015

netman92 commented Aug 25, 2015

caitp commented Aug 25, 2015

netman92 commented Aug 31, 2015

jtheoof commented Aug 31, 2015

jvorcak commented Sep 10, 2015

gkalpak commented Oct 29, 2015

petebacondarwin commented Oct 29, 2015

gkalpak commented Nov 18, 2015

Issue with canceling XHR request using promises - fixed #12525 #12657

Issue with canceling XHR request using promises - fixed #12525 #12657

Conversation

netman92 commented Aug 23, 2015

jtheoof commented Aug 23, 2015

netman92 commented Aug 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtheoof commented Aug 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caitp commented Aug 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak commented Aug 24, 2015

netman92 commented Aug 25, 2015

Narretz commented Aug 25, 2015

caitp commented Aug 25, 2015

netman92 commented Aug 25, 2015

caitp commented Aug 25, 2015

netman92 commented Aug 31, 2015

jtheoof commented Aug 31, 2015

jvorcak commented Sep 10, 2015

gkalpak commented Oct 29, 2015

petebacondarwin commented Oct 29, 2015

gkalpak commented Nov 18, 2015