[core-lro] Set isCancelled when operation status is cancelled#21893
[core-lro] Set isCancelled when operation status is cancelled#21893deyaaeldeen merged 12 commits intoAzure:mainfrom
Conversation
ghost
added
the
|
API change check for API changes are not detected in this pull request for |
|
API change check for API changes are not detected in this pull request for |
| * so throwing an error in this case could be prevent customer from getting | ||
| * access to those partial results. | ||
| */ | ||
| if (["PUT", "DELETE", "PATCH"].includes(lro.requestMethod) && status === "canceled") { |
There was a problem hiding this comment.
If we don't throw even in PUT, DELETE and PATCH. Would users get stuck in a weird state or just wouldn't have interesting information in the request?
If they don't end up in a bad place, would it make sense to not throw and have everything fall into the next check?
There was a problem hiding this comment.
The pollUntilDone() method returns a result so if no error was thrown, it will return an object with a status field set to canceled as the result and it most likely will not match the expected type of the result object.
There was a problem hiding this comment.
I reverted the behavioral change for now and kept the original behavior of throwing in all cancellation scenarios. I will explore not throwing for POST requests in another PR.
|
API change check API changes are not detected in this pull request. |
witemple-msft
left a comment
witemple-msft
left a comment
There was a problem hiding this comment.
These changes make sense to me, and thanks for adding the tests to confirm.
I only have one comment about the version policy.
4 participants
The
canceloperation on the poller can be implemented dynamically by passing a function to thecanceloption in thelroEngine's options bag. However, @witemple-msft pointed out that the poller is being marked as cancelled prematurely before even calling the user-providedcancelcallback.I remedied this situation by:
not setsetisCanceledin the poller's state at all and leave that to the library author to do inside their callbacksisCanceledafter the cancellation callback fully resolves.isCanceledif the operation status returned from the service is set tocanceled.The change #1 is needed because polling stops if
poller.cancelOperation()fully resolves so there is no chance for the change #2 to kick in and setisCanceledaccordingly.The change #2 is needed because cancellation could originate from places other than the poller at hand.
One important behavioral change in this PR to call out is that an operation status set tocanceledcaused the poller to throw an error saying the operation has failed. With this PR, canceling a POST LRO request will no longer throw an error by default. I believe this behavioral change is necessary because some POST LROs support access to partial results even if the operation was canceled (Text Analytics for example). However, library authors can throw errors manually in their callbacks as needed.Please note that this PR is scoped to fixing the issue of prematurely setting
isCanceled. I am happy to look at any feature requests or other issues in other github issues/pull requests.