-
Notifications
You must be signed in to change notification settings - Fork 0
Code Review Bench PR #64685 - [release/9.0] Update dependencies from dotnet/arcade #2
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
base: base_pr_64685_20260125_8707
Are you sure you want to change the base?
Changes from all commits
35526fe
3132e7b
98fd2f8
f76fb84
bc7d271
e45bfed
b71ad44
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -89,6 +89,7 @@ public async Task HubConnectionCanSendAndReceiveGroupMessages(HttpTransportType | |||||||||||||||||||||
| [ConditionalTheory] | ||||||||||||||||||||||
| [SkipIfDockerNotPresent] | ||||||||||||||||||||||
| [MemberData(nameof(TransportTypesAndProtocolTypes))] | ||||||||||||||||||||||
| [QuarantinedTest("https://github.com/dotnet/aspnetcore/issues/59991")] | ||||||||||||||||||||||
| public async Task CanSendAndReceiveUserMessagesFromMultipleConnectionsWithSameUser(HttpTransportType transportType, string protocolName) | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| using (StartVerifiableLog()) | ||||||||||||||||||||||
|
|
@@ -184,6 +185,7 @@ public async Task HubConnectionCanSendAndReceiveGroupMessagesGroupNameWithPatter | |||||||||||||||||||||
| [ConditionalTheory] | ||||||||||||||||||||||
| [SkipIfDockerNotPresent] | ||||||||||||||||||||||
| [MemberData(nameof(TransportTypesAndProtocolTypes))] | ||||||||||||||||||||||
| [QuarantinedTest("https://github.com/dotnet/aspnetcore/issues/63582")] | ||||||||||||||||||||||
| public async Task CanSendAndReceiveUserMessagesUserNameWithPatternIsTreatedAsLiteral(HttpTransportType transportType, string protocolName) | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| using (StartVerifiableLog()) | ||||||||||||||||||||||
|
|
@@ -208,8 +210,8 @@ public async Task CanSendAndReceiveUserMessagesUserNameWithPatternIsTreatedAsLit | |||||||||||||||||||||
| await connection.InvokeAsync("EchoUser", "*", "Hello, World!").DefaultTimeout(); | ||||||||||||||||||||||
| Assert.Equal("Hello, World!", await tcs.Task.DefaultTimeout()); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| await connection.DisposeAsync().DefaultTimeout(); | ||||||||||||||||||||||
| await secondConnection.DisposeAsync().DefaultTimeout(); | ||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||||||||||||||||||
| await secondConnection.DisposeAsync().DefaultTimeout(); | |
| await connection.DisposeAsync().DefaultTimeout(); | |
| await secondConnection.DisposeAsync().DefaultTimeout(); |
- Apply suggested fix
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.
⚠️ Bug: Zero-byte reads now consume the receive result, causing data loss
The _receiveTcs reinitialization was moved from after the zero-byte read check to before it. Previously, when buffer.Count == 0 (a zero-byte read used as a signal/peek), the method would return the result header without resetting _receiveTcs, allowing a subsequent non-zero-byte read to retrieve the same result and actually copy the data into the buffer.
With the new ordering:
res = await _receiveTcs.Task— gets the pending result_receiveTcs = new(...)— immediately resets the TCS (result is consumed)if (buffer.Count == 0) return res.Item1— returns only the WebSocketReceiveResult without copying data
Now a zero-byte read consumes and discards the actual payload data (res.Item2). The next call to ReceiveAsync will await a new TCS that hasn't been signaled yet, blocking until SetReceiveResult is called again. This causes the message data from the first receive to be permanently lost.
This is the pattern used by SignalR for stateful reconnect, where zero-byte reads act as notifications that data is available, followed by a full read to get the data.
Was this helpful? React with 👍 / 👎
| _receiveTcs = new(TaskCreationOptions.RunContinuationsAsynchronously); | |
| var res = await _receiveTcs.Task; | |
| // Handle zero-byte reads | |
| if (buffer.Count == 0) | |
| { | |
| return res.Item1; | |
| } | |
| _receiveTcs = new(TaskCreationOptions.RunContinuationsAsynchronously); | |
| res.Item2.CopyTo(buffer); | |
| return res.Item1; |
- Apply suggested fix
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.
🚨 Bug: Logical AND should be OR: assertion will always fail
The assertion uses
&&(AND) to check if the log message matches bothParseErrorMessageRegexOldandParseErrorMessageRegexNew, but the error message says "Expected log message to match one of the CSP error patterns".The two regexes match mutually exclusive message formats:
"Refused to frame '...' because an ancestor violates...""Framing '...' violates..."A single log message cannot match both patterns simultaneously, so
IsMatch(Old) && IsMatch(New)will always befalse, causing this test assertion to always fail. The operator should be||(OR) to correctly handle either CSP error message format.Was this helpful? React with 👍 / 👎