collect errors more reliably from websocket test client

[graingert]

…iddleware' children

Co-authored-by: Thomas Grainger [email protected]
Co-authored-by: Nikita Gashkov [email protected]

Summary

Checklist

• I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
• I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
• I've updated the documentation accordingly.

[graingert]

@Kludex I think this fixed it

[Kludex]

What was the issue?

[graingert]

The exception bubbling out of the _run was not being collected properly, and I think the queue EOF/shutdown thing also

[graingert]

I'd recommend reviewing this in side by side view: https://github.com/encode/starlette/pull/2814/files?diff=split

[Kludex]

Should we use the if sys.version_info here?

[graingert]

I think it's better to stick to the EOF approach until someone puts up a Queue.shutdown backport, or we can use a MemoryObjectStream with portal

[Kludex]

Is there an analogous to EOF from the standard library on 3.13?

[graingert]

It raises an exception

[Kludex]

As long as this passes the tests, I'm fine with whatever refactor you think is the best here.

[Kludex]

Why is the cancelled exception here? Is this because this except was removed?

The addition of that except was on purpose, if I recall correctly.

[graingert]

this is instead of websocket.should_close.is_set() as it no longer exists - so we need to find out that the async function ran and was cancelled

[graingert]

did you mean to ask about this one: https://github.com/encode/starlette/pull/2814/files#diff-aca25e5f16c4fd49338ccdf3631f72455309335fee1e27f3d3b6016fa94ecedfL145 ?

this is because it's no longer possible to get a intentional cancelled_exc here because it will be caught by the CancelScope
if you do get an cancelled_exc here it's because someone incorrectly issued a native asyncio cancel or manually raised a trio cancel, both of which should be propagated out of the websocket_connect cmgr

[Kludex]

Got it.

So the point is: since the WebSocket doesn't call send it can't receive a disconnect exception, and since it doesn't call receive, it doesn't receive a websocket.disconnected message. Then, the TestClient here will propagate the cancellation exception here, but the user shouldn't worry about it because the TestClient will catch it anyway.

Right?

[graingert]

yeah, we use a CancelScope passed out with task_status.started(cs), this is then used to cancel the task on completion and collect a result. The CancelScope catches the cancellation that it causes to raise in the coro, unless there's another reason for cancellation which is then propagated, this is a fatal case and so the user should be notified

[Kludex]

So if there's a cancelled exception that is not from the TestClient, the user will see it, right?

I'm good with this.

[graingert]

Yeah the only way is if the user was calling .cancel() directly or manually raising constructing a trio cancel (bypassing the private constructor protections)

[Kludex]

Why it should be impossible?

The except BaseException as exc below doesn't have a pragma: no cover, so I assume it's being hit?

[graingert]

this is impossible because the exit stack will raise the exception out of fut.result() and so the queue won't be consumed.

This is only possible to be hit if ws.receive() is interrupted (eg with a KI) while waiting for an exception or message to be placed on the queue.

I'm currently sketching out another slight refactor that uses MemoryObjectStreams here instead that should clean this up a bit