11685: Restore cancellation token behavior #11693
Conversation
| // args array and the `CANCELLATION_TOKEN_KEY` string instead. | ||
| const cancellationToken: CancellationToken | undefined = args.length && CancellationToken.is(args[args.length - 1]) ? args.pop() : undefined; | ||
| if (cancellationToken && cancellationToken.isCancellationRequested) { | ||
| return Promise.reject(this.cancelError()); | ||
| } |
There was a problem hiding this comment.
From the comment above about subbing in a magic string for the cancellation token itself, it looks like we don't pass the state of token - whether it's already been cancelled or not - to the receiving end. If that's the case, then by removing this block, we remove handling the case in which the token has already been cancelled when we get here, since I don't believe that it will fire the onCancellationRequested event again. In that case, should we send the cancellation notice immediately after sending the request itself?
There was a problem hiding this comment.
Good point. Yes, sending the cancellation notice directly after sending the request sounds reasonable to me.
There was a problem hiding this comment.
@colin-grant-work After investigating and testing this it turns out that both main CancellationToken implementations (Theia and vscode-languageserver) already have a special shortcut event in place that will trigger newly registered cancellation listeners on already canceled tokens immediately. So the cancellation notice for early canceled tokens is already passed correctly to the receiving side.
However, just to be on the safe side (and since in theory also custom cancellation token implementations could be used) I still added the discussed behavior. Directly after sending the request we check whether the token is already canceled. If so, send the cancellation notice, otherwise listen on the onCancellationRequested event.
Ensure that cancelling an RPC call does not result in an automatic rejection on the calling side. Instead the token is passed to the receiving side and handed there. This restores the cancellation strategy that was used with the old json-rpc architecture (prior to 1.28) Contributed on behalf of STMicroelectronics. Fixes eclipse-theia#11685
|
@JonasHelming, I have a high level of confidence in this solution, and I believe there's little chance of regression. But I wouldn't insist on its inclusion in the community release if there are objections or concerns. |
|
OK, I talked to Tobi and agree. We do not have a formal process, but @paul-marechal or @tsmaeder would you mind to either give your OK, too or veto? |
|
The solution looks good to me, as for whether to include this in the community release: does the behaviour we fix in this PR cause problems "in the wild" and how bad are those problems? #11685 doesn't really have info about the real world impact. If it has significant impact, let's include it, if it doesn't, it's an automatic "no" from me ;-) |
|
Yes, that is a valid point, @colin-grant-work could you comment on this? I think I remember you mentioned it to be a "corner case"? |
|
@JonasHelming, @tsmaeder, the impact on us was non-trivial - a whole widget ended up hanging in an indeterminate state because we hadn't expected the Promise generated by the backend call to ever get rejected - either it would succeed or be canceled with partial return. But it took a number of steps to get there: we had to know we wanted to time out at some point and issue the cancelation but also expect cancellation to work in a fairly timely manner and without rejection. Those assumptions had been correct before, but I'm not sure how many teams will have gotten themselves in the same situation. |
|
@colin-grant-work : Will you pick up the release on Thursday or shortly after? If so, we would have another 2 weeks before we actually anounce the release in case there are any regressions. |
|
@JonasHelming, since we uplifted to 1.28 and encountered this problem, we've adjusted our code to handle the cancellation rejection on the frontend, so it isn't urgent for us that it make it into the community release, but if there are products waiting to uplift until they can uplift to the community release, then it might still be worth it to get this fix in so that they don't run into the same problem when they do. |
|
Sorry, I rephrase my question: Will you uplift your project to the community release soon? If that is the case, you would most likely identify any potential regressions we find with this fix before we announce the release in two weeks from now. Even if the risk is low, this would be another safety net. |
|
In any case, my vote is a "yes" on this one based on what you described. @paul-marechal ? |
|
@JonasHelming, I think we will start an uplift to 1.30, yes, so we should catch any regressions in practical code. |
|
OK, we will include this. |
Ensure that cancelling an RPC call does not result in an automatic rejection on the calling side. Instead the token is passed to the receiving side and handled there. This restores the cancellation strategy that was used with the old json-rpc architecture (prior to 1.28). Contributed on behalf of STMicroelectronics.
Ensure that cancelling an RPC call does not result in an automatic rejection on the calling side. Instead the token is passed to the receiving side and handled there. This restores the cancellation strategy that was used with the old json-rpc architecture (prior to 1.28). Contributed on behalf of STMicroelectronics.
|
@colin-grant-work given the real-world impact, I'm fine with the inclusion, as well. |
|
This is now part of the release (1.29.1) Thanks @colin-grant-work for bringing this up! |
What it does
Ensure that cancelling an RPC call does not result in an automatic rejection on the calling side. Instead the token is passed to the receiving side and handed there. This restores the cancellation strategy that was used with the old json-rpc architecture (prior to 1.28)
Contributed on behalf of STMicroelectronics.
Fixes #11685
How to test
The simple service that I used for testing is available here:
https://github.com/eclipsesource/theia/tree/cancelation-token-test
Review checklist
Reminder for reviewers