Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

feat($resource): add support for cancelling requests #13210

Closed

Conversation

gkalpak
Copy link
Member

@gkalpak gkalpak commented Oct 30, 2015

Introduced changes:

  • Deprecate passing a promise as timeout (for $resource actions).
    It never worked correctly anyway.
    Now a warning is logged (using $log.debug()) and the property is
    removed.
  • Add support for a boolean cancellable property to actions'
    configuration, the $resource classes options of the
    $resourceProvider's defaults.
    If true, the $cancelRequest method (added to all returned values for
    non-instance calls) will abort the request (if it's not already
    completed or aborted).
    If there is timeout specified on the action's configuration, the value
    of cancellable is ignored.

Example usage:

var Post = $resource('/posts/:id', {id: '@id'}, {
  get: {
    method: 'GET',
    cancellable: true
  }
});

var currentPost = Post.get({id: 1});
...
// A moment later the user selects another post, so
// we don't need the previous request any more
currentPost.$cancelRequest();
currentPost = Post.get({id: 2});
...

Fixes #9332
Closes #13050
Closes #13058

@gkalpak
Copy link
Member Author

gkalpak commented Oct 30, 2015

This is an alternative approach to #13058 as discussed in #13058 (comment).
Still WIP - looking for feedback 😃

Pending:

  • Add docs.
  • Decide on/Improve names/warning message text etc.
  • Decide if we need a BC notice ? (Probably...)
  • Decide if we want something more "aggressive" whe using a promise as timeout.
    (E.g. $log.warn/error, throw error, etc.) (Probably not...)
  • Decide if we want to support cancellation on instance calls as well (which usually are not idempotent) ? (Probably not...)
    If yes, how ?
    Implementation detail: The instance may still have a (noop'd out) $cancelRequest() method from when it was created. Could that trick the user into believing they can cancel the request ? Can we do anything about it ?
  • Decide if we want to restrict this to some types of requests only (e.g. GET).
    It might be a good idea to only support cancellation on idempotent requests, but then again you don't really know what requests are idempotent and what aren't.
  • Last but not least: Stop overthinking it and just get this merged 😛

/cc @petebacondarwin

@petebacondarwin
Copy link
Contributor

The API looks good to me. I haven't looked at the implementation but I think it is worth pursuing this line.

@iyeldinov
Copy link

Good job! +1

@gkalpak gkalpak modified the milestones: 1.5.0-beta.3, 1.5.x - migration-facilitation Nov 18, 2015
@petebacondarwin
Copy link
Contributor

  • Add docs. Yes
  • Decide on/Improve names/warning message text etc. Names look good to me
  • Decide if we need a BC notice ? (Probably...) Yes
  • Decide if we want something more "aggressive" whe using a promise as timeout. (E.g. $log.warn/error, throw error, etc.) (Probably not...) No
  • Decide if we want to support cancellation on instance calls as well (which usually are not idempotent) ? (Probably not...) If yes, how ? Implementation detail: The instance may still have a (noop'd out) $cancelRequest() method from when it was created. Could that trick the user into believing they can cancel the request ? Can we do anything about it ? For simplicity we should start only with making static call requests cancellable. Can we delete the $cancelRequest method from the instance once the request completes (or is cancelled)?
  • Decide if we want to restrict this to some types of requests only (e.g. GET). It might be a good idea to only support cancellation on idempotent requests, but then again you don't really know what requests are idempotent and what aren't. No

@petebacondarwin
Copy link
Contributor

@gkalpak if you can get this up and running before next release it can go in 1.5.0 otherwise let's punt it to 1.6

@gkalpak
Copy link
Member Author

gkalpak commented Nov 20, 2015

Can we delete the $cancelRequest method from the instance once the request completes (or is cancelled)?

@petebacondarwin , the problem is that the user should then always check if the method is available before calling it (and that makes the API weird).

Consider the typical usecase, where the app loads some data based on a user's choice. As soon as the user changes their choice, the old request is irrelevant and a new one should be made. At this point, it is a good practice to cancel the previous request, but we don't really know if that request has already completed or is still pending.

@petebacondarwin
Copy link
Contributor

OK, an alternate API is that you pass the object back to a cancel request on the original resource object...

@gkalpak
Copy link
Member Author

gkalpak commented Nov 21, 2015

Would be possible, but requires more "housekeeping", right ?
Could go either way, but slightly prefer this PR's way.

What are the benefits of calling a static method (maybe I missed something) ?

@petebacondarwin
Copy link
Contributor

The benefit is that you don't need to modify the properties on the instance object.
Something like...

var postsResource = $resource(...);
var posts = postsResource.query(...);
postsResource.cancelRequest(posts);

@gkalpak
Copy link
Member Author

gkalpak commented Nov 22, 2015

Yes, but you need to keep an association between requested resources/instances and promises internally, so I feel there is more housekeeping to be done. It would be also more easy to introduce memory leaks (if at all avoidable without WeakMaps).

So, off the top of my head, implementation-wise, modifying the instance properties should be preferrable/simpler. It is also more consistent imo with $promise and $resolved, which are already attached to the instance (or array of instances).

But from a user's perspective, would there be any benefit ?

@petebacondarwin
Copy link
Contributor

Do the $promise and $resolved properties work for instance requests?

@petebacondarwin
Copy link
Contributor

If we provide a new promise in the $promise property for every request then how about adding a cancel method to that object?

@gkalpak
Copy link
Member Author

gkalpak commented Nov 22, 2015

That would work as well, but:

  1. It would make that promise "special" (i.e. not a regular promise).
  2. What is the benefit (from a user's perspective or implementation-wise) ?

@petebacondarwin
Copy link
Contributor

The $resource service makes me sad. OK, you have convinced me. My ideas do not improve the user experience.

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@gkalpak gkalpak force-pushed the feat-$resource-abortable-requests branch from 7967347 to 392a261 Compare November 23, 2015 11:38
@googlebot
Copy link

CLAs look good, thanks!

@gkalpak
Copy link
Member Author

gkalpak commented Nov 23, 2015

I added the docs (and the BC notice can be added upon squashing).
There is an issue with the @requires $log in the docs. It doesn't seem to be able to find $log as an available service, so it doesn't probably link to it.

Will try to get a look at it today.

@gkalpak
Copy link
Member Author

gkalpak commented Nov 23, 2015

So, it seems this somehow related to how the dgeni-packages/links#getDocFromAlias service works
Namely, when there are several docs definitions, but none matches the originatingDocs module, then an emty list is returned. IMO, it would make more sense to look for a "doc definition" under the ng module as a fallback.

In any case, the issue in this PR can be solved by changing @requires $log to @requires ng.$log.
/cc @petebacondarwin to decide if this needs to be fixed in dgeni-packages or works as expected.

@gkalpak gkalpak force-pushed the feat-$resource-abortable-requests branch from 392a261 to ea65cfb Compare November 23, 2015 13:20
Introduced changes:

- Deprecate passing a promise as `timeout` (for `$resource` actions).
  It never worked correctly anyway.
  Now a warning is logged (using `$log.debug()`) and the property is
  removed.
- Add support for a boolean `cancellable` property in actions'
  configuration, the `$resource` factory's `options` parameter and the
  `$resourceProvider`'s `defaults` property.
  If true, the `$cancelRequest` method (added to all returned values for
  non-instance calls) will abort the request (if it's not already
  completed or aborted).
  If there is a numeric `timeout` specified on the action's configuration,
  the value of `cancellable` will be ignored.

Example usage:

```js
var Post = $resource('/posts/:id', {id: '@id'}, {
  get: {
    method: 'GET',
    cancellable: true
  }
});

var currentPost = Post.get({id: 1});
...
// A moment later the user selects another post, so
// we don't need the previous request any more
currentPost.$cancelRequest();
currentPost = Post.get({id: 2});
...
```

BREAKING CHANGE:

Using a promise as `timeout` is no longer supported and will log a
warning. It never worked the way it was supposed to anyway.

Before:

```js
var deferred = $q.defer();
var User = $resource('/api/user/:id', {id: '@id'}, {
  get: {method: 'GET', timeout: deferred.promise}
});

var user = User.get({id: 1});   // sends a request
deferred.resolve();             // aborts the request

// Now, we need to re-define `User` passing a new promise as `timeout`
// or else all subsequent requests from `someAction` will be aborted
User = $resource(...);
user = User.get({id: 2});
```

After:

```js
var User = $resource('/api/user/:id', {id: '@id'}, {
  get: {method: 'GET', cancellable: true}
});

var user = User.get({id: 1});   // sends a request
instance.$cancelRequest();      // aborts the request

user = User.get({id: 2});
```

Fixes angular#9332
Closes angular#13050
Closes angular#13058
@gkalpak gkalpak force-pushed the feat-$resource-abortable-requests branch from ea65cfb to e15961e Compare November 23, 2015 14:34
@petebacondarwin
Copy link
Contributor

@gkalpak thanks for the investigation but that is indeed how dgeni-packages is supposed to work :-)

@gkalpak
Copy link
Member Author

gkalpak commented Nov 23, 2015

I fixed the @requires $log issue and added BC notice in the first commit's message.
This is ready for final review afaict :)

@petebacondarwin, isn't it kind of unexpected that if there is only 1 "doc definition" then it is returned (even if doesn't match the originatingDoc's module), but if there are >1 "doc definitions" (none of which matches originatingDoc's module) then no doc is returned. I would expect at least one to be returned (e.g. the one on the ng module if available or all of them).

* `transformResponse` to an empty array: `transformResponse: []`
* By default, transformResponse will contain one function that checks if the response looks
* like a JSON string and deserializes it using `angular.fromJson`. To prevent this behavior,
* set `transformResponse` to an empty array: `transformResponse: []`
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and above, I just truncated the lines to 100 chars max.
Sorry, I couldn't help myself 😃

@petebacondarwin
Copy link
Contributor

The way it works is that we match the text you provide against all the aliases of the docs in the system.

  • If exactly one matches then we are happy and we use that one.
  • If none match then we have an invalid link.
  • If more than one match then we try (as a one off) looking in the current doc's module, since it makes sense that you might be referring to the service in the current module.
    • If there is exactly one match in the current module then we are happy
  • Otherwise we have an ambiguous link that needs to be fixed by providing more context (e.g. the module).

@gkalpak
Copy link
Member Author

gkalpak commented Nov 23, 2015

I see. This makes sense as well, I guess. Tweaking the "smartness" vs predictability ratio is a matter of preferrence after all 😃
Thx for clarifying !

@petebacondarwin
Copy link
Contributor

I guess we could also have a heuristic to favour ng if there are multiple but where does it end?

@petebacondarwin petebacondarwin modified the milestones: 1.6.x, 1.5.0-beta.3 Nov 23, 2015
@gkalpak gkalpak changed the title WIP - feat($resource): add support for cancelling requests feat($resource): add support for cancelling requests Nov 23, 2015
@gkalpak
Copy link
Member Author

gkalpak commented Nov 23, 2015

It doesn't ! Now that I now the reasoning behind it, it makes sense, so it's fine by me :)

* will be cancelled (if not already completed) by calling `$cancelRequest()` on the call's
* return value. Calling `$cancelRequest()` for a non-cancellable or an already
* completed/cancelled request will have no effect.<br />
* **Note:** If a timeout is specified in millisecondes, `cancellable` is ignored.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is that so?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By design 😄

Basically, both need to pass a value as timeout to $http's config (either the number of milliseconds or a promise).
You can't use both a numeric timeout and a promise, so I chose for timeout to take precedence.
(Note that calcellable can be configured as a global default or as per resource default (using the options argument), while timeout can't.)

if (!isInstanceCall && action.cancellable) {
var deferred = $q.defer();
httpConfig.timeout = deferred.promise;
value.$cancelRequest = deferred.resolve;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to worry about memory leaks if this promise gets attached to the http request?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and the returned value?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't given it too much thought tbh, but here is what I think:

  • httpConfig and value do not reference each other, so the question is whether each one is disposed of properly on its own.
  • httpConfig should get gc'ed (but maybe it doesn't because it is referenced inside the promise callback - I am not sure). If it doesn't, we should fix it. In any case, this is not introduced by or in any way related to changes in this PR.
  • value.$cancelRequest does live on, so we need to de-reference deferred as soon as that's possible, in order to enable it to be gc'ed. That is what's done in the finally block.

I might have missed something though, so take the above with a grain of salt.

@gkalpak
Copy link
Member Author

gkalpak commented Nov 24, 2015

I added a commit to replace calls to .hasOwnProperty() on action and options to hasOwnProperty.call() (as discussed off-GitHub).
I wonder: if we are trying to support custom objects (for action and options), would it make sense to omit the hasOwnProperty check altogether (e.g. use action.cancellable or options.cancellable etc) ?

@gkalpak
Copy link
Member Author

gkalpak commented Nov 24, 2015

Note to self: This needs to be backported to v1.4.x, to solve the issue introduced with 7170f9d.

@petebacondarwin
Copy link
Contributor

Hmm - I feel uncomfortable about backporting to 1.4.8. I think it might be safer to just revert 7170f9d instead?

@gkalpak
Copy link
Member Author

gkalpak commented Nov 24, 2015

Works for me as well.

@gkalpak
Copy link
Member Author

gkalpak commented Nov 24, 2015

And maybe also log a warning on v1.4.x that promises are not supported as timeout in $resource.

@gkalpak gkalpak closed this in 98528be Nov 24, 2015
@gkalpak
Copy link
Member Author

gkalpak commented Nov 25, 2015

@petebacondarwin, thx for merging !

So, what about v.1.4.x ?

  • Should we just revert 7170f9d ?
  • Update the docs to explain that promise as timeout is not supported ?
  • Log a warning when someone does ?

@gkalpak gkalpak deleted the feat-$resource-abortable-requests branch November 26, 2015 08:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants