Synchronize writing to connection and aborting connection #10043
Conversation
BrennanConroy
requested review from
analogrelay,
halter73 and
mikaelm12
as code owners
|
Help me understand this change? The description of the fix is a bit light on explanation. |
|
Servers could get error logs saying "Writing is not allowed after writer was completed" if the connection was closed while writing, especially when writing the ping message. This synchronizes the abort with the writing so we wont see error logs when writing would be expected to fail. |
|
Unit test? |
This reverts commit de2735a.
It's a race and there isn't anywhere to inject our own test code to force it. |
| @@ -173,7 +178,7 @@ public virtual ValueTask WriteAsync(HubMessage message, CancellationToken cancel | |||
| /// <returns></returns> | |||
| public virtual ValueTask WriteAsync(SerializedHubMessage message, CancellationToken cancellationToken = default) | |||
| { | |||
| if (_connectedAborted) | |||
| if (_connectionAborted) | |||
There was a problem hiding this comment.
Is there much reason to check _connectionAborted before acquiring the lock when we check immediately after? Presumably, if the connection is aborted, whoever has the lock shouldn't for long. I prefer removing redundant checks if possible.
|
Do we need to backport this? |
Worst case it's an erroneous error log. There is no functional issue here. Based on that I don't think we should, but open to other opinions. |
|
Looks like a legit test failures:
|
|
This comment was made automatically. If there is a problem contact aspnetcore-build@microsoft.com. I've triaged the above build. I've created/commented on the following issue(s) |
Fixes #6701
Verified perf didn't change.