fix(connection): reject pending callbacks on transport death - #3170
fix(connection): reject pending callbacks on transport death#3170Simon Knott (Skn0tt) wants to merge 1 commit into
Conversation
Sorry, something went wrong.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3351313b-bb35-475c-ae04-f79e1d2e4086
| # relies on it to attribute rejections to the right test. | ||
| if browser: | ||
| connection._loop.call_soon(browser._on_close) | ||
| connection.cleanup(reason) |
There was a problem hiding this comment.
this seems like it'd reintroduce the problem solved by #3135
Sorry, something went wrong.
There was a problem hiding this comment.
Hmm, the test added is still green though. Can you explain how?
Sorry, something went wrong.
There was a problem hiding this comment.
that test stays green because page._on_close() synchronously rejects its local Waiter before browser._on_close is queued
this change queues browser._on_close before connection.cleanup() rejects its callback, so disconnected can run first
a pending page.evaluate("new Promise(() => {})") would expose the ordering regression
Sorry, something went wrong.
| }, | ||
| return_when=asyncio.FIRST_COMPLETED, | ||
| ) | ||
| result = await callback.future |
There was a problem hiding this comment.
this seems like it'd reintroduce the problem solved by #3144
Sorry, something went wrong.
There was a problem hiding this comment.
Can you elaborate on how? Task cancellation still triggers __abort__, which ends the request on the server side and eventually makes this callback reject.
Sorry, something went wrong.
There was a problem hiding this comment.
the distinction is that __abort__ is still sent, but the caller no longer waits for it to complete
since callback.future is directly awaited, canceling the task also cancels that future
_abort() then sends __abort__, awaits the already canceled future, catches CancelledError immediately, and returns
the later server response is discarded by the callback.future.cancelled() branch in dispatch(), rather than making the callback reject
the existing test stays green because it checks the eventual effect of sending __abort__, not whether cancellation waits for the protocol response
Sorry, something went wrong.
|
Gave it a shot, but it looks like this change isn't worth the hassle just for being more in-line with JS! Let's stick with what we have. |
Sorry, something went wrong.
Every send used to race the protocol callback against transport.on_error_future, because a dead pipe only completed that future and never tore down in-flight calls.
This matches the Node client more closely: Connection.run() calls cleanup() when the transport ends, which rejects every pending callback. Sends just await their own future.
For remote connect, browser._on_close is still call_soon'd before cleanup(), so waiters still see reject-before-disconnect ordering.