Skip to content
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

Synchronize writing to connection and aborting connection #10043

Merged
merged 9 commits into from May 12, 2019

Conversation

Projects
None yet
6 participants
@BrennanConroy
Copy link
Member

commented May 7, 2019

Fixes #6701

Verified perf didn't change.

@davidfowl

This comment has been minimized.

Copy link
Member

commented May 7, 2019

Help me understand this change? The description of the fix is a bit light on explanation.

@BrennanConroy

This comment has been minimized.

Copy link
Member Author

commented May 7, 2019

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.

BrennanConroy added some commits May 6, 2019

@BrennanConroy BrennanConroy force-pushed the brecon/sync branch from 91f36af to fc0b1f9 May 9, 2019

@JamesNK

This comment has been minimized.

Copy link
Member

commented May 10, 2019

Unit test?

BrennanConroy added some commits May 10, 2019

Revert "tunnel vision"
This reverts commit de2735a.
fb
fb
@BrennanConroy

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

Unit test?

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)

This comment has been minimized.

Copy link
@halter73

halter73 May 10, 2019

Member

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.

This comment has been minimized.

Copy link
@davidfowl
fb
@davidfowl

This comment has been minimized.

Copy link
Member

commented May 11, 2019

Do we need to backport this?

@BrennanConroy

This comment has been minimized.

Copy link
Member Author

commented May 11, 2019

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.

@davidfowl

This comment has been minimized.

Copy link
Member

commented May 11, 2019

Looks like a legit test failures:

  • Microsoft.AspNetCore.SignalR.Tests.HubConnectionHandlerTests.ConnectionTimesOutIfInitialPingAndThenNoMessages

  • Microsoft.AspNetCore.SignalR.Tests.HubConnectionHandlerTests.AbortFromHubMethodForcesClientDisconnect

@BrennanConroy

This comment has been minimized.

@aspnet-hello

This comment has been minimized.

Copy link

commented May 12, 2019

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)
aspnet/AspNetCore-Internal#2511

@BrennanConroy BrennanConroy merged commit 208299a into master May 12, 2019

14 of 15 checks passed

AspNetCore-helix-test Build #20190511.15 had test failures
Details
AspNetCore-ci Build #20190511.17 had test failures
Details
AspNetCore-ci (Build: Linux ARM) Build: Linux ARM succeeded
Details
AspNetCore-ci (Build: Linux ARM64) Build: Linux ARM64 succeeded
Details
AspNetCore-ci (Build: Linux Musl x64) Build: Linux Musl x64 succeeded
Details
AspNetCore-ci (Build: Linux x64) Build: Linux x64 succeeded
Details
AspNetCore-ci (Build: Windows ARM) Build: Windows ARM succeeded
Details
AspNetCore-ci (Build: Windows x64/x86) Build: Windows x64/x86 succeeded
Details
AspNetCore-ci (Build: macOS) Build: macOS succeeded
Details
AspNetCore-ci (Code check) Code check succeeded
Details
AspNetCore-ci (Test: Templates - Windows Server 2016 x64) Test: Templates - Windows Server 2016 x64 succeeded
Details
AspNetCore-ci (Test: Ubuntu 16.04 x64) Test: Ubuntu 16.04 x64 succeeded
Details
AspNetCore-ci (Test: Windows Server 2016 x64) Test: Windows Server 2016 x64 succeeded
Details
AspNetCore-ci (Test: macOS 10.13) Test: macOS 10.13 succeeded
Details
license/cla All CLA requirements met.
Details

@BrennanConroy BrennanConroy deleted the brecon/sync branch May 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.