Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature] External cancellation of ...WaitFor... methods where feasible #24083

Closed
maw136 opened this issue May 30, 2023 · 8 comments
Closed

[Feature] External cancellation of ...WaitFor... methods where feasible #24083

maw136 opened this issue May 30, 2023 · 8 comments

Comments

@maw136
Copy link

maw136 commented May 30, 2023

Add external cancellation of ...WaitFor... methods where feasible.

Reasoning:
Wait for style methods have a timeout, but there are circumstances where external events could be the cause to cancel specific waits.
The current solution doesn't provide any cancellation except default/custom timouts.

@mxschmitt
Copy link
Member

Thanks for the feature request, unfortunately this is not compatible with the way Playwright, the driver and calls to the browser are working. If you sent once something to the browser, it can't be canceled. Since we don't plan on implementing this, I will close it for now.

@maw136
Copy link
Author

maw136 commented May 31, 2023

Could you take another look at this?
The set of APIs that 'WaitFor' events are dotnet-side.
My PR implements the bread and butter of this specific kind of scenario.
The exact use case is like waiting for multiple different Request/Responses and cancelling some waits under some specific circumstances.
The cancellation is not at the level at which the driver calls the browser, the cancellation is at the listening for specific kind of events.
In the current state of Playwright there is no way to stop listening to these events. It is just an early exit.

@mxschmitt
Copy link
Member

Yup sorry for closing this before, re-opening by that.

@pavelfeldman
Copy link
Member

Could you provide a complete example where you benefit from the cancellation token? I'd like to understand the use case better.

@pavelfeldman pavelfeldman transferred this issue from microsoft/playwright-dotnet Jul 6, 2023
@mxschmitt
Copy link
Member

We discussed this feature in our team and came to the conclusion that this change might confuse users. While event handlers, aka. WaitFor* methods can be cancelled, the code you have inside, can not. Often these are clicks, evaluates, etc. To keep a clear boundary about this, we decided to not go forward with this API proposal.

I apologise for being slow on this PRs/issues, since some of our coworkers were out of office. Thank you!

@mxschmitt mxschmitt closed this as not planned Won't fix, can't repro, duplicate, stale Jul 18, 2023
@maw136
Copy link
Author

maw136 commented Jul 18, 2023

But it's all about cancelling the wait/listen for event, not the action. There is no way for early cancel of any WaitFor... waiter

@mxschmitt
Copy link
Member

The code inside the callback still continues to run and does not follow .NET's rule of periodically checking if the cancellation was requested. This is a fundamental feature which Playwright does not offer and yield in unexpected behaviour.

We recommend Task.WhenAny() instead.

@maw136
Copy link
Author

maw136 commented Jul 20, 2023

So more spaghetti code instead. :(
In all meaningful scenarios the 'callback' as you called it is an action that triggers the request/response dialog with backend.
A cancellation would not cause any unexpected behaviour/side effects, as the action have already run to completion either way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants