Skip to content

chore: do not continue with no_reply messages #1905

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.

Sign up for GitHub

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

Merged
mxschmitt merged 3 commits into from
May 9, 2023
Merged

chore: do not continue with no_reply messages #1905

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9d1fc5a
chore: do not continue with no_reply messages
mxschmitt May 9, 2023
dd7a6f0
review feedback
mxschmitt May 9, 2023
4506332
nit
mxschmitt May 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions playwright/_impl/_connection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,10 @@ async def send_return_as_dict(self, method: str, params: Dict = None) -> Any:
)

def send_no_reply(self, method: str, params: Dict = None) -> None:
# No reply messages are used to e.g. waitForEventInfo(after).
self._connection.wrap_api_call_sync(
lambda: self._connection._send_message_to_server(
self._guid, method, {} if params is None else params
self._guid, method, {} if params is None else params, True
)
)

Expand Down Expand Up @@ -178,6 +179,7 @@ def remove_listener(self, event: str, f: Any) -> None:
class ProtocolCallback:
def __init__(self, loop: asyncio.AbstractEventLoop) -> None:
self.stack_trace: traceback.StackSummary
self.no_reply: bool
self.future = loop.create_future()
# The outer task can get cancelled by the user, this forwards the cancellation to the inner task.
current_task = asyncio.current_task()
Expand Down Expand Up @@ -305,7 +307,7 @@ def set_in_tracing(self, is_tracing: bool) -> None:
self._tracing_count -= 1

def _send_message_to_server(
self, guid: str, method: str, params: Dict
self, guid: str, method: str, params: Dict, no_reply: bool = False
) -> ProtocolCallback:
if self._closed_error_message:
raise Error(self._closed_error_message)
Expand All @@ -317,6 +319,7 @@ def _send_message_to_server(
traceback.StackSummary,
getattr(task, "__pw_stack_trace__", traceback.extract_stack()),
)
callback.no_reply = no_reply
self._callbacks[id] = callback
stack_trace_information = cast(ParsedStackTrace, self._api_zone.get())
frames = stack_trace_information.get("frames", [])
Expand Down Expand Up @@ -357,6 +360,10 @@ def dispatch(self, msg: ParsedMessagePayload) -> None:
callback = self._callbacks.pop(id)
if callback.future.cancelled():
return
# No reply messages are used to e.g. waitForEventInfo(after) which returns exceptions on page close.
# To prevent 'Future exception was never retrieved' we just ignore such messages.
if callback.no_reply:
Copy link
Copy Markdown
Member

@pavelfeldman pavelfeldman May 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was bad about calling set_result on the callback future?

Copy link
Copy Markdown
Contributor Author

@mxschmitt mxschmitt May 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the PR description, that we received an error (waitForEventInfo after), were setting it on the protocol callback but never awaiting it, hence it was showing the warning in the console.

Copy link
Copy Markdown
Member

@pavelfeldman pavelfeldman May 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd do a), but b) also works. Please add a comment about it when you are assining no_reply to True

return
error = msg.get("error")
if error:
parsed_error = parse_error(error["error"]) # type: ignore
Expand Down