Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
collect errors more reliably from websocket test client #2814
collect errors more reliably from websocket test client #2814
Changes from 13 commits
5442570
3978a17
8eec435
44c1bc2
c210c27
3fde98b
4d2ddbc
57701e7
3dc727d
afc65e7
6e60cda
3c8071f
2ef7a53
509b9c0
c013d14
9b016c5
82dad82
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an analogous to EOF from the standard library on 3.13?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It raises an exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why it should be impossible?
The
except BaseException as exc
below doesn't have apragma: no cover
, so I assume it's being hit?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use the
if sys.version_info
here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 callreceive
, it doesn't receive awebsocket.disconnected
message. Then, theTestClient
here will propagate the cancellation exception here, but the user shouldn't worry about it because theTestClient
will catch it anyway.Right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if there's a cancelled exception that is not from the TestClient, the user will see it, right?
I'm good with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the only way is if the user was calling .cancel() directly or manually raising constructing a trio cancel (bypassing the private constructor protections)