From db6d9124153dd33d189cc6c6b1aa578d6adc1210 Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Fri, 16 Oct 2026 09:54:05 -0700 Subject: [PATCH] chore: fix the review findings on the typed channels - Channel allows a handler to be registered more than once; each off call removes one registration. - Move the Channel base class to _channel.py so that _connection.py imports the generated channel classes at module level. - Route.fulfill raises on a body that is neither str nor bytes. - convert_input_files reads the file mtimes inside the internal createTempFiles call, as upstream does. - CDPSession "event" payload has no None placeholder for params. --- playwright/_impl/_cdp_session.py | 5 +- playwright/_impl/_channel.py | 129 +++++++++++++++++++ playwright/_impl/_connection.py | 106 +-------------- playwright/_impl/_js_handle.py | 3 +- playwright/_impl/_network.py | 2 + playwright/_impl/_protocol_channels.py | 2 +- playwright/_impl/_protocol_types.py | 2 +- playwright/_impl/_set_input_files_helpers.py | 37 +++--- scripts/codegen/generate_protocol.py | 4 +- 9 files changed, 164 insertions(+), 126 deletions(-) create mode 100644 playwright/_impl/_channel.py diff --git a/playwright/_impl/_cdp_session.py b/playwright/_impl/_cdp_session.py index 1fdf77d9d..89a4dff21 100644 --- a/playwright/_impl/_cdp_session.py +++ b/playwright/_impl/_cdp_session.py @@ -39,7 +39,10 @@ def __init__( def _on_event(self, event: "channels.CDPSessionEventEvent") -> None: self.emit(event["method"], event["params"]) - self.emit(CDPSession.Events.Event, event) + self.emit( + CDPSession.Events.Event, + {key: value for key, value in event.items() if value is not None}, + ) async def send(self, method: str, params: Optional[Dict]) -> Dict: return (await self._channel.send(method=method, params=params))["result"] diff --git a/playwright/_impl/_channel.py b/playwright/_impl/_channel.py new file mode 100644 index 000000000..51c7ec879 --- /dev/null +++ b/playwright/_impl/_channel.py @@ -0,0 +1,129 @@ +# Copyright (c) Microsoft Corporation. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Port of the channel proxy in packages/playwright-core/src/client/channelOwner.ts.""" + +import asyncio +from typing import TYPE_CHECKING, Any, Callable, Dict, List, Mapping, Optional, Tuple + +from pyee.asyncio import AsyncIOEventEmitter + +if TYPE_CHECKING: # pragma: no cover + from playwright._impl._connection import ChannelOwner, Connection + + +class Channel: + """The base of the generated channels in _protocol_channels.py.""" + + def __init__(self, connection: "Connection", object: "ChannelOwner") -> None: + self._connection = connection + self._guid = object._guid + self._object = object + self._emitter = AsyncIOEventEmitter() + self._emitter.on( + "error", lambda exc: self._connection._on_event_listener_error(exc) + ) + # (event, handler) -> the wrappers registered for it, in order. + self._event_listeners: Dict[Tuple[str, Any], List[Callable[[Any], Any]]] = {} + + async def _call( + self, method: str, params: Mapping[str, Any], timeout: float + ) -> Dict[str, Any]: + """Sends a command and returns its result, {} when it has none.""" + result = await self._connection.wrap_api_call( + lambda: self._inner_send(method, _filter_none(params), timeout) + ) + return result or {} + + def _add_event_listener( + self, event: str, handler: Any, listener: Callable[[Any], Any] + ) -> None: + """Registers listener, the wrapper that calls handler with the typed payload. + + A handler can be registered more than once, like on an EventEmitter; + each off call removes one registration. + """ + self._event_listeners.setdefault((event, handler), []).append(listener) + self._emitter.on(event, listener) + + def _remove_event_listener(self, event: str, handler: Any) -> None: + listeners = self._event_listeners.get((event, handler)) + if not listeners: + return + listener = listeners.pop() + if not listeners: + del self._event_listeners[(event, handler)] + self._emitter.remove_listener(event, listener) + + def send_no_reply( + self, + method: str, + params: Mapping[str, Any] = None, + is_internal: bool = False, + title: str = None, + ) -> None: + """Sends a message without waiting for the reply. + + For messages that must go out synchronously, such as __waitInfo__ and + updateSubscription. + """ + self._connection.wrap_api_call_sync( + lambda: self._connection._send_message_to_server( + self._object, method, _filter_none(params or {}), 0, True + ), + is_internal, + title, + ) + + async def _inner_send( + self, method: str, params: Dict[str, Any], timeout: float + ) -> Optional[Dict[str, Any]]: + if self._connection._error: + error = self._connection._error + self._connection._error = None + raise error + callback = self._connection._send_message_to_server( + self._object, method, params, timeout + ) + try: + done, _ = await asyncio.wait( + { + self._connection._transport.on_error_future, + callback.future, + }, + return_when=asyncio.FIRST_COMPLETED, + ) + except asyncio.CancelledError as exc: + await self._connection._abort( + self._object, + callback, + str(exc) or "Task was cancelled", + ) + raise + if not callback.future.done(): + callback.future.cancel() + result = next(iter(done)).result() + if not result: + return None + assert isinstance(result, dict) + return result + + +def _filter_none(d: Mapping) -> Dict: + result = {} + for k, v in d.items(): + if v is None: + continue + result[k] = _filter_none(v) if isinstance(v, dict) else v + return result diff --git a/playwright/_impl/_connection.py b/playwright/_impl/_connection.py index 325276959..3c2c7f773 100644 --- a/playwright/_impl/_connection.py +++ b/playwright/_impl/_connection.py @@ -27,9 +27,7 @@ Callable, Dict, List, - Mapping, Optional, - Tuple, TypedDict, Union, cast, @@ -40,6 +38,7 @@ import playwright import playwright._impl._impl_to_api_mapping +from playwright._impl._channel import Channel from playwright._impl._errors import ( Error, TargetClosedError, @@ -50,6 +49,7 @@ from playwright._impl._helper import ( ParsedMessagePayload, ) +from playwright._impl._protocol_channels import CHANNEL_CLASSES from playwright._impl._protocol_check import check_message, is_enabled from playwright._impl._transport import Transport @@ -61,96 +61,6 @@ _PLAYWRIGHT_MODULE_PATH = str(Path(playwright.__file__).parents[0]) -class Channel: - """The base of the generated channels in _protocol_channels.py. - - Port of the channel proxy in packages/playwright-core/src/client/channelOwner.ts. - """ - - def __init__(self, connection: "Connection", object: "ChannelOwner") -> None: - self._connection = connection - self._guid = object._guid - self._object = object - self._emitter = AsyncIOEventEmitter() - self._emitter.on( - "error", lambda exc: self._connection._on_event_listener_error(exc) - ) - self._event_listeners: Dict[Tuple[str, Any], Callable[[Any], Any]] = {} - - async def _call( - self, method: str, params: Mapping[str, Any], timeout: float - ) -> Dict[str, Any]: - """Sends a command and returns its result, {} when it has none.""" - result = await self._connection.wrap_api_call( - lambda: self._inner_send(method, _filter_none(params), timeout) - ) - return result or {} - - def _add_event_listener( - self, event: str, handler: Any, listener: Callable[[Any], Any] - ) -> None: - self._event_listeners[(event, handler)] = listener - self._emitter.on(event, listener) - - def _remove_event_listener(self, event: str, handler: Any) -> None: - listener = self._event_listeners.pop((event, handler), None) - if listener: - self._emitter.remove_listener(event, listener) - - def send_no_reply( - self, - method: str, - params: Mapping[str, Any] = None, - is_internal: bool = False, - title: str = None, - ) -> None: - """Sends a message without waiting for the reply. - - For messages that must go out synchronously, such as __waitInfo__ and - updateSubscription. - """ - self._connection.wrap_api_call_sync( - lambda: self._connection._send_message_to_server( - self._object, method, _filter_none(params or {}), 0, True - ), - is_internal, - title, - ) - - async def _inner_send( - self, method: str, params: Dict[str, Any], timeout: float - ) -> Optional[Dict[str, Any]]: - if self._connection._error: - error = self._connection._error - self._connection._error = None - raise error - callback = self._connection._send_message_to_server( - self._object, method, params, timeout - ) - try: - done, _ = await asyncio.wait( - { - self._connection._transport.on_error_future, - callback.future, - }, - return_when=asyncio.FIRST_COMPLETED, - ) - except asyncio.CancelledError as exc: - await self._connection._abort( - self._object, - callback, - str(exc) or "Task was cancelled", - ) - raise - if not callback.future.done(): - callback.future.cancel() - result = next(iter(done)).result() - if not result: - return None - assert isinstance(result, dict) - return result - - class ChannelOwner(AsyncIOEventEmitter): def __init__( self, @@ -171,9 +81,6 @@ def __init__( parent if isinstance(parent, ChannelOwner) else None ) self._objects: Dict[str, "ChannelOwner"] = {} - # Imported here: the generated channels import this module. - from playwright._impl._protocol_channels import CHANNEL_CLASSES - self._channel: Channel = CHANNEL_CLASSES.get(type, Channel)( self._connection, self ) @@ -722,15 +629,6 @@ def _attach_api_call_information( return stack_trace_information -def _filter_none(d: Mapping) -> Dict: - result = {} - for k, v in d.items(): - if v is None: - continue - result[k] = _filter_none(v) if isinstance(v, dict) else v - return result - - def format_call_log(log: Optional[List[str]]) -> str: if not log: return "" diff --git a/playwright/_impl/_js_handle.py b/playwright/_impl/_js_handle.py index b629fbebe..740d18e84 100644 --- a/playwright/_impl/_js_handle.py +++ b/playwright/_impl/_js_handle.py @@ -17,7 +17,8 @@ from pathlib import Path from typing import TYPE_CHECKING, Any, Dict, List, Optional, Union -from playwright._impl._connection import Channel, ChannelOwner, from_channel +from playwright._impl._channel import Channel +from playwright._impl._connection import ChannelOwner, from_channel from playwright._impl._errors import is_target_closed_error from playwright._impl._serializers import parse_value, serialize_value diff --git a/playwright/_impl/_network.py b/playwright/_impl/_network.py index 61bb3d444..2526c2e4e 100644 --- a/playwright/_impl/_network.py +++ b/playwright/_impl/_network.py @@ -445,6 +445,8 @@ async def _inner_fulfill( encoded_body = base64.b64encode(body).decode() is_base64 = True length = len(body) + elif body is not None: + raise Error(f"body must be a str or bytes, got {type(body).__name__}") elif path: file_content = Path(path).read_bytes() encoded_body = base64.b64encode(file_content).decode() diff --git a/playwright/_impl/_protocol_channels.py b/playwright/_impl/_protocol_channels.py index 4c459dd8e..0d0a8ec6a 100644 --- a/playwright/_impl/_protocol_channels.py +++ b/playwright/_impl/_protocol_channels.py @@ -28,7 +28,7 @@ from typing import TYPE_CHECKING, Any, Callable, Dict, Literal, Optional, Sequence, Type -from playwright._impl._connection import Channel +from playwright._impl._channel import Channel if TYPE_CHECKING: from playwright._impl import _protocol_types as channels diff --git a/playwright/_impl/_protocol_types.py b/playwright/_impl/_protocol_types.py index 16fcc36d3..cf40f2d2f 100644 --- a/playwright/_impl/_protocol_types.py +++ b/playwright/_impl/_protocol_types.py @@ -45,7 +45,7 @@ if TYPE_CHECKING: from typing_extensions import NotRequired - from playwright._impl._connection import Channel + from playwright._impl._channel import Channel from playwright._impl._protocol_channels import ( AndroidChannel, AndroidDeviceChannel, diff --git a/playwright/_impl/_set_input_files_helpers.py b/playwright/_impl/_set_input_files_helpers.py index 00a583930..9633ad43f 100644 --- a/playwright/_impl/_set_input_files_helpers.py +++ b/playwright/_impl/_set_input_files_helpers.py @@ -83,25 +83,30 @@ async def convert_input_files( (_list_files(local_directory) if local_directory else local_paths), ) streams = [] - root_dir_name = ( - os.path.basename(local_directory) if local_directory else None - ) - temp_files: List[channels.BrowserContextCreateTempFilesParamsItems] = [ - { - "name": ( - os.path.relpath(file, local_directory) - if local_directory - else os.path.basename(file) + + async def create_temp_files() -> ( + "channels.BrowserContextCreateTempFilesResult" + ): + return await context._channel.createTempFiles( + rootDirName=( + os.path.basename(local_directory) if local_directory else None ), - "lastModifiedMs": int(os.path.getmtime(file) * 1000), - } - for file in files_to_stream - ] + items=[ + { + "name": ( + os.path.relpath(file, local_directory) + if local_directory + else os.path.basename(file) + ), + "lastModifiedMs": int(os.path.getmtime(file) * 1000), + } + for file in files_to_stream + ], + ) + result: channels.BrowserContextCreateTempFilesResult = ( await context._connection.wrap_api_call( - lambda: context._channel.createTempFiles( - rootDirName=root_dir_name, items=temp_files - ) + create_temp_files, is_internal=True ) ) for i, file in enumerate(result["writableStreams"]): diff --git a/scripts/codegen/generate_protocol.py b/scripts/codegen/generate_protocol.py index 35c8eb41f..bf60f5d66 100644 --- a/scripts/codegen/generate_protocol.py +++ b/scripts/codegen/generate_protocol.py @@ -358,7 +358,7 @@ def generate_types(protocol: Dict[str, Any]) -> Tuple[str, List[Interface], Set[ "if TYPE_CHECKING:", " from typing_extensions import NotRequired", "", - " from playwright._impl._connection import Channel", + " from playwright._impl._channel import Channel", " from playwright._impl._protocol_channels import (", *[f" {channel}," for channel in used_channels], " )", @@ -461,7 +461,7 @@ def generate_channels(interfaces: List[Interface], type_names: Set[str]) -> str: "", f"from typing import {', '.join(sorted(typing_names))}", "", - "from playwright._impl._connection import Channel", + "from playwright._impl._channel import Channel", "", "if TYPE_CHECKING:", " from playwright._impl import _protocol_types as channels",