feature: add cancellation token to variety of ...WaitFor... APIs #2597

maw136 · 2023-05-30T19:29:31Z

fixes: microsoft/playwright#24083
adds CancellationToken where feasible and extends Waiter class to provide CancellationToken as a source to cancel wait

campersau · 2023-05-30T21:18:07Z

src/Playwright/Core/Waiter.cs

+ var cancelledTask = Task.FromCanceled(cancellationToken);
+ RejectOn(cancelledTask, () => cancelledTask.Dispose());


This should probably look something like this:

Suggested change

var cancelledTask = Task.FromCanceled(cancellationToken);

RejectOn(cancelledTask, () => cancelledTask.Dispose());

if (!cancellationToken.CanBeCanceled) return;

var cts = new TaskCompletionSource<bool>();

var registration = cancellationToken.Register(state => cts.SetCanceled((CancellationToken)state), cancellationToken);

RejectOn(cts.Task, () => registration.Dispose());

Thank you very much. I was fighting with this part. I found some approaches but nothing worked properly.
I am adding tests to cover cancellation scenarios.

campersau · 2023-05-31T04:06:34Z

src/Playwright/Core/Waiter.cs

+ dispose?.Invoke();
+ _error = ex.ToString();
+ Dispose();
+ return default;


To me it would be strange to just get the default value back instead of a canceled exception.
In .NET it is standard that the user needs to handle canceled exceptions as soon as they are passing cancellation tokens around.

I was looking for an accepted general rule for that. Do you suggest having that specific exception bubble up or converting it (as by default) to PlaywrightException?

From my experience cancellation exceptions escpecially ones triggered by a user provided CancellationToken aren't wrapped / converted in other exceptions. So I would vote for letting the cancellation exception bubble up.

I've refactored into rethrow.

campersau · 2023-05-31T04:07:59Z

src/Playwright/Core/Waiter.cs

@@ -218,6 +230,13 @@ internal async Task<T> WaitForPromiseAsync<T>(Task<T> task, Action dispose = nul
 Dispose();
 throw new TimeoutException(ex.Message + FormatLogRecording(_logs), ex);
 }
+ catch (TaskCanceledException ex)


Catch OperationCanceledException instead or both.
See dotnet/runtime#83406 (comment)

If both exception handlers for TaskCanceledException and OperationCanceledException do exactly the same thing then you just need the OperationCanceledException one because TaskCanceledException inherits from it.

(That is what I meant with instead ;)

Yeah, missed that inheritance hierarchy. Fixed

campersau · 2023-05-31T04:16:36Z

src/Playwright/Core/Waiter.cs

+ }
+
+ var cts = new TaskCompletionSource<bool>();
+ var registration = cancellationToken.Register(cts.SetCanceled, false);


If you let the cancellation throw instead (#2597 (comment)) this should use the Set Canceled(cancellationToken) instead so users can inspect it if the catch it higher up in the stack.

I've used TrySetCanceled as it is available in 3.1 as well as 6.0, otherwise I would have to add compile-time conditional

campersau · 2023-05-31T04:24:08Z

src/Playwright/Core/Waiter.cs

+ {
+ return;
+ }
+


Just an idea: maybe it would be good to call cancellationToken.ThrowIfCancellationRequested() (here or maybe higher up in the call stack) so that if it is already cancelled we don't do anything.

I was thinking more of something like this:

if (!cancellationToken.CanBeCanceled) { return; // don't add a cancellation registration if the cancellationToken can never be cancelled (e.g. CancellationToken.None / default) } cancellationToken.ThrowIfCancellationRequested(); // if the cancellation token is already cancelled then throw immediately

If the cancellation token can't be cancelled then ThrowIfCancellationRequested will always noop.

Yes, that was my understanding of your comment as well, alas I have added the throw in the wrong place, fixed now :)

mxschmitt · 2023-06-27T17:20:26Z

src/Playwright/Core/Waiter.cs

@@ -243,7 +267,11 @@ private static async Task WrapActionAsync(Func<Task> action, CancellationTokenSo
 }
 catch
 {
- cts.Cancel();
+ if (!cts.IsCancellationRequested)


Why this change? What breaks if we always cancel here?

There is no need to cancel 'harder'. I have replicated the solution from the other place in this PR where this check is made, and I remembered that I have added this cancellation here some time ago. This is the correct solution for the overall problem. Any code should be written in the most proper/correct way. Simply working is not ever good enough.

mxschmitt · 2023-06-27T17:20:35Z

src/Playwright/Core/Waiter.cs

+ dispose?.Invoke();
+ _error = ex.ToString();
+ Dispose();
+ throw;


would be nice to throw the same exception but with formatted logs like in line 250. We can potentially also have a single catch for both.

With OperationCanceledException the suggested flow is to throw the original exception to have better understanding of the place of cancelation. https://devblogs.microsoft.com/premier-developer/recommended-patterns-for-cancellationtoken/

mxschmitt · 2023-06-27T17:27:09Z

src/Playwright/API/Generated/Options/BrowserContextRunAndWaitForConsoleMessageOptions.cs

+ /// </para>
+ /// </summary>
+ [JsonPropertyName("cancellationToken")]
+ public CancellationToken CancellationToken { get; set; }


Did you add these manually? All these files in that folder are generated automatically. We should add csharp specific properties upstream like here so that the generator will pick it up.

Yes, manual addition. I will move it to the source for the generator.

See here for some notes on how it works.

microsoft/playwright#23953

maw136 · 2023-06-29T10:39:09Z

Added PR for upstream:
microsoft/playwright#23953

…r review changes

…edException

maw136 · 2023-07-18T18:26:36Z

Used this generator for generated members:
microsoft/playwright#23953

mxschmitt · 2023-07-28T09:35:25Z

Closing as per microsoft/playwright#24083 (comment).

campersau reviewed May 30, 2023

View reviewed changes

campersau reviewed May 31, 2023

View reviewed changes

mxschmitt reviewed Jun 27, 2023

View reviewed changes

maw136 force-pushed the feature/add-cancellation-feature branch from 5f1be00 to 5f06aff Compare June 28, 2023 21:48

maw136 mentioned this pull request Jun 29, 2023

docs(dotnet): add-cancellation-feature for to code generation microsoft/playwright#23953

Closed

maw136 added 20 commits July 18, 2023 19:46

feature: add cancellation token to variety of ...WaitFor... APIs

115ef66

Fix waiting task

915273d

Constant for inifinite wait timout

209025c

Test that covers cancellation of wait for request/response.

06c9d5e

Styling

55a5414

Simplified test

993a60c

Rollback lang version upgrade

1bb58a3

Rethrow OperationCanceledException and TaskCanceledException and othe…

b5bc4b1

…r review changes

Style fix

f05fd4b

Review - TaskCanceledException actually inherits from OperationCancel…

8d17252

…edException

Review - wrong conditional

f0742dc

roll Playwright to 1.36.0

9607da7

changed parsing

9b7f4ca

fixed finally

438da07

refactor

58cc808

refactor, properly pass cancellation token

e7b01c5

fix build

2f5748f

roll back to 1.35.1

c6ec1a6

rollback

e4d1d96

rollback

5772c32

maw136 force-pushed the feature/add-cancellation-feature branch from c57d901 to 5772c32 Compare July 18, 2023 18:00

rollback

5e75bd2

mxschmitt closed this Jul 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: add cancellation token to variety of ...WaitFor... APIs #2597

feature: add cancellation token to variety of ...WaitFor... APIs #2597

maw136 commented May 30, 2023

campersau May 30, 2023

maw136 May 30, 2023

campersau May 31, 2023

maw136 May 31, 2023

campersau May 31, 2023

maw136 May 31, 2023

campersau May 31, 2023

campersau May 31, 2023 •

edited

Loading

maw136 Jun 1, 2023

campersau May 31, 2023

maw136 May 31, 2023 •

edited

Loading

campersau May 31, 2023

maw136 May 31, 2023

campersau May 31, 2023

maw136 Jun 1, 2023

mxschmitt Jun 27, 2023

maw136 Jun 28, 2023

mxschmitt Jun 27, 2023 •

edited

Loading

maw136 Jun 28, 2023

mxschmitt Jun 27, 2023

maw136 Jun 27, 2023

mxschmitt Jun 28, 2023

maw136 Jun 29, 2023

maw136 commented Jun 29, 2023

maw136 commented Jul 18, 2023

mxschmitt commented Jul 28, 2023

		var cancelledTask = Task.FromCanceled(cancellationToken);
		RejectOn(cancelledTask, () => cancelledTask.Dispose());

feature: add cancellation token to variety of ...WaitFor... APIs #2597

feature: add cancellation token to variety of ...WaitFor... APIs #2597

Conversation

maw136 commented May 30, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

campersau May 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maw136 May 31, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxschmitt Jun 27, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maw136 commented Jun 29, 2023

maw136 commented Jul 18, 2023

mxschmitt commented Jul 28, 2023

campersau May 31, 2023 •

edited

Loading

maw136 May 31, 2023 •

edited

Loading

mxschmitt Jun 27, 2023 •

edited

Loading