feat: support AbortSignal on server-side methods#41141
Conversation
Adds a client-only `signal` option that aborts an in-flight server operation via a new fire-and-forget `__cancel__` message, which aborts the call's ProgressController in the dispatcher. Wires it up on Locator.click and Locator.waitFor as a starting point. Fixes: microsoft#40578
Adds the signal option next to every existing timeout option on action, getter and navigation methods, threading it through the client so in-flight calls can be cancelled via the __cancel__ wire message. Refs: microsoft#40578
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Skn0tt
changed the title
|
|
||
| async ariaSnapshot(options: TimeoutOptions & { mode?: 'ai' | 'default', depth?: number, boxes?: boolean, _track?: string } = {}): Promise<string> { | ||
| const result = await this.mainFrame()._channel.ariaSnapshot({ timeout: this._timeoutSettings.timeout(options), track: options._track, mode: options.mode, depth: options.depth, boxes: options.boxes }); | ||
| const result = await this.mainFrame()._channel.ariaSnapshot({ timeout: this._timeoutSettings.timeout(options), track: options._track, mode: options.mode, depth: options.depth, boxes: options.boxes, ...{ signal: options.signal } }); |
There was a problem hiding this comment.
How can we make sure that signal is passed for every method where it is defined? I think we are at least lacking signal option defined in many methods, and types like FetchOptions or ElectronOptions. Perhaps passing it explicitly as a second argument to generated channel methods is better? Or somehow ensuring in TypeScript that we pass non-optional AbortSignal | SomeSpecialSymbol to make sure it is always there.
There was a problem hiding this comment.
The explicit second argument would be neat, but we already have progress?: Progress as the argument and it's hard to change since client/dispatcher share the interface. I made it a non-optional param, all callers that don't pass a signal have to pass undefined.
- inject required `signal` into every generated *Params type so every channel call site must forward it - strip signal from validation/instrumentation in channelOwner and thread it to sendMessageToServer; use signal.throwIfAborted() - omit client-only signal from server-facing channel param types - link setDefaultTimeout from the AbortSignal option docs
…server-side # Conflicts: # packages/playwright-core/src/client/page.ts # utils/generate_channels.js
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
The connect-family channel params are built manually from public option types that do not carry a signal, previously using a cast to read a field that is never present. Replace with an explicit signal: undefined to match the rest of the rollout.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Test results for "MCP"1 failed 7340 passed, 1122 skipped Merge workflow run. |
Test results for "tests 1"39579 passed, 743 skipped Merge workflow run. |
2 participants
Summary
signal?: AbortSignaloption next to every existingtimeoutoption on action, getter and navigation methods.__cancel__wire message that aborts the dispatcher'sProgressController; a pre-aborted signal throwssignal.reasonsynchronously.AbortSignalsupport to client-side Waiter methods #41136 (Waiter-basedwaitFor*methods), which is excluded here as it already has signal support.Not included (follow-ups)
expectassertions (toBeVisible, etc.) — deferred to a separate PR.ElementHandle.inputValue— intentionally left out. UnlikeLocator/Page/Frame.inputValue(which route through theFrame.inputValueprotocol method that carriestimeout), theElementHandle.inputValueprotocol method (packages/protocol/spec/handles.yml) declares no parameters, so thetimeoutoption the public types have long advertised there is silently dropped by the wire validator. Addingsignalwould require a signature change and inherit that broken-timeout behavior. Fixing it is a separate change: addtimeout/signalto theinputValueentry inhandles.yml.Refs #40578