From 7fd42c4e946338f5725ef57f6c2eccf0ed9a078c Mon Sep 17 00:00:00 2001 From: Brennan Conroy Date: Thu, 10 Oct 2019 04:41:00 +0000 Subject: [PATCH 1/4] Merged PR 3115: Wait to Complete Pipe Fixed the `PipeWriterStream` to properly detect a canceled write and throw an `OperationCanceledException` in those cases. And making sure `Complete` is called in a safe manner by ensuring it is in a lock so writes can't be in-progress. --- eng/PatchConfig.props | 6 ++ .../ts/FunctionalTests/selenium/run-tests.ts | 23 ++++++- .../src/Internal/HttpConnectionContext.cs | 28 ++++++++- .../src/Internal/HttpConnectionDispatcher.cs | 15 +++-- .../src/Internal/TaskExtensions.cs | 27 ++++++++ src/SignalR/common/Shared/PipeWriterStream.cs | 12 +++- .../server/Core/src/HubConnectionContext.cs | 61 +++++++++++++++++++ .../SignalR/test/HubConnectionHandlerTests.cs | 4 +- 8 files changed, 164 insertions(+), 12 deletions(-) create mode 100644 src/SignalR/common/Http.Connections/src/Internal/TaskExtensions.cs diff --git a/eng/PatchConfig.props b/eng/PatchConfig.props index 3ba7babc57..83f2b24497 100644 --- a/eng/PatchConfig.props +++ b/eng/PatchConfig.props @@ -44,4 +44,10 @@ Later on, this will be checked using this condition: Microsoft.AspNetCore.CookiePolicy; + + + Microsoft.AspNetCore.Http.Connections; + Microsoft.AspNetCore.SignalR.Core; + + diff --git a/src/SignalR/clients/ts/FunctionalTests/selenium/run-tests.ts b/src/SignalR/clients/ts/FunctionalTests/selenium/run-tests.ts index c74603b12c..6f548f1534 100644 --- a/src/SignalR/clients/ts/FunctionalTests/selenium/run-tests.ts +++ b/src/SignalR/clients/ts/FunctionalTests/selenium/run-tests.ts @@ -1,7 +1,8 @@ import { ChildProcess, spawn } from "child_process"; -import * as fs from "fs"; +import * as _fs from "fs"; import { EOL } from "os"; import * as path from "path"; +import { promisify } from "util"; import { PassThrough, Readable } from "stream"; import { run } from "../../webdriver-tap-runner/lib"; @@ -9,6 +10,16 @@ import { run } from "../../webdriver-tap-runner/lib"; import * as _debug from "debug"; const debug = _debug("signalr-functional-tests:run"); +const ARTIFACTS_DIR = path.resolve(__dirname, "..", "..", "..", "..", "artifacts"); +const LOGS_DIR = path.resolve(ARTIFACTS_DIR, "logs"); + +// Promisify things from fs we want to use. +const fs = { + createWriteStream: _fs.createWriteStream, + exists: promisify(_fs.exists), + mkdir: promisify(_fs.mkdir), +}; + process.on("unhandledRejection", (reason) => { console.error(`Unhandled promise rejection: ${reason}`); process.exit(1); @@ -102,6 +113,13 @@ if (chromePath) { try { const serverPath = path.resolve(__dirname, "..", "bin", configuration, "netcoreapp2.1", "FunctionalTests.dll"); + if (!await fs.exists(ARTIFACTS_DIR)) { + await fs.mkdir(ARTIFACTS_DIR); + } + if (!await fs.exists(LOGS_DIR)) { + await fs.mkdir(LOGS_DIR); + } + debug(`Launching Functional Test Server: ${serverPath}`); const dotnet = spawn("dotnet", [serverPath], { env: { @@ -117,6 +135,9 @@ if (chromePath) { } } + const logStream = fs.createWriteStream(path.resolve(LOGS_DIR, "ts.functionaltests.dotnet.log")); + dotnet.stdout.pipe(logStream); + process.on("SIGINT", cleanup); process.on("exit", cleanup); diff --git a/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionContext.cs b/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionContext.cs index 045e821ee1..425c15d76e 100644 --- a/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionContext.cs +++ b/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionContext.cs @@ -274,13 +274,35 @@ namespace Microsoft.AspNetCore.Http.Connections.Internal // Cancel any pending flushes from back pressure Application?.Output.CancelPendingFlush(); - // Shutdown both sides and wait for nothing + // Normally it isn't safe to try and acquire this lock because the Send can hold onto it for a long time if there is backpressure + // It is safe to wait for this lock now because the Send will be in one of 4 states + // 1. In the middle of a write which is in the middle of being canceled by the CancelPendingFlush above, when it throws + // an OperationCanceledException it will complete the PipeWriter which will make any other Send waiting on the lock + // throw an InvalidOperationException if they call Write + // 2. About to write and see that there is a pending cancel from the CancelPendingFlush, go to 1 to see what happens + // 3. Enters the Send and sees the Dispose state from DisposeAndRemoveAsync and releases the lock + // 4. No Send in progress + await WriteLock.WaitAsync(); + try + { + // Complete the applications read loop + Application?.Output.Complete(transportTask.Exception?.InnerException); + } + finally + { + WriteLock.Release(); + } + + Log.WaitingForTransportAndApplication(_logger, TransportType); + + // Wait for application so we can complete the writer safely + await applicationTask.NoThrow(); + + // Shutdown application side now that it's finished Transport?.Output.Complete(applicationTask.Exception?.InnerException); - Application?.Output.Complete(transportTask.Exception?.InnerException); try { - Log.WaitingForTransportAndApplication(_logger, TransportType); // A poorly written application *could* in theory get stuck forever and it'll show up as a memory leak await Task.WhenAll(applicationTask, transportTask); } diff --git a/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs b/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs index 50910bccfe..2d8b5c24ad 100644 --- a/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs +++ b/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs @@ -511,6 +511,14 @@ namespace Microsoft.AspNetCore.Http.Connections.Internal context.Response.StatusCode = StatusCodes.Status404NotFound; context.Response.ContentType = "text/plain"; + + // There are no writes anymore (since this is the write "loop") + // So it is safe to complete the writer + // We complete the writer here because we already have the WriteLock acquired + // and it's unsafe to complete outside of the lock + // Other code isn't guaranteed to be able to acquire the lock before another write + // even if CancelPendingFlush is called, and the other write could hang if there is backpressure + connection.Application.Output.Complete(); return; } @@ -549,11 +557,8 @@ namespace Microsoft.AspNetCore.Http.Connections.Internal Log.TerminatingConection(_logger); - // Complete the receiving end of the pipe - connection.Application.Output.Complete(); - - // Dispose the connection gracefully, but don't wait for it. We assign it here so we can wait in tests - connection.DisposeAndRemoveTask = _manager.DisposeAndRemoveAsync(connection, closeGracefully: true); + // Dispose the connection, but don't wait for it. We assign it here so we can wait in tests + connection.DisposeAndRemoveTask = _manager.DisposeAndRemoveAsync(connection, closeGracefully: false); context.Response.StatusCode = StatusCodes.Status202Accepted; context.Response.ContentType = "text/plain"; diff --git a/src/SignalR/common/Http.Connections/src/Internal/TaskExtensions.cs b/src/SignalR/common/Http.Connections/src/Internal/TaskExtensions.cs new file mode 100644 index 0000000000..a901379b75 --- /dev/null +++ b/src/SignalR/common/Http.Connections/src/Internal/TaskExtensions.cs @@ -0,0 +1,27 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System.Runtime.CompilerServices; + +namespace System.Threading.Tasks +{ + internal static class TaskExtensions + { + public static async Task NoThrow(this Task task) + { + await new NoThrowAwaiter(task); + } + } + + internal readonly struct NoThrowAwaiter : ICriticalNotifyCompletion + { + private readonly Task _task; + public NoThrowAwaiter(Task task) { _task = task; } + public NoThrowAwaiter GetAwaiter() => this; + public bool IsCompleted => _task.IsCompleted; + // Observe exception + public void GetResult() { _ = _task.Exception; } + public void OnCompleted(Action continuation) => _task.GetAwaiter().OnCompleted(continuation); + public void UnsafeOnCompleted(Action continuation) => OnCompleted(continuation); + } +} diff --git a/src/SignalR/common/Shared/PipeWriterStream.cs b/src/SignalR/common/Shared/PipeWriterStream.cs index eb5b6d5add..245731bfd9 100644 --- a/src/SignalR/common/Shared/PipeWriterStream.cs +++ b/src/SignalR/common/Shared/PipeWriterStream.cs @@ -1,4 +1,4 @@ -// Copyright (c) .NET Foundation. All rights reserved. +// Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; @@ -76,7 +76,15 @@ namespace System.IO.Pipelines _length += source.Length; var task = _pipeWriter.WriteAsync(source); - if (!task.IsCompletedSuccessfully) + if (task.IsCompletedSuccessfully) + { + // Cancellation can be triggered by PipeWriter.CancelPendingFlush + if (task.Result.IsCanceled) + { + throw new OperationCanceledException(); + } + } + else if (!task.IsCompletedSuccessfully) { return WriteSlowAsync(task); } diff --git a/src/SignalR/server/Core/src/HubConnectionContext.cs b/src/SignalR/server/Core/src/HubConnectionContext.cs index 5a1049e780..085249438a 100644 --- a/src/SignalR/server/Core/src/HubConnectionContext.cs +++ b/src/SignalR/server/Core/src/HubConnectionContext.cs @@ -33,6 +33,7 @@ namespace Microsoft.AspNetCore.SignalR private long _lastSendTimestamp = Stopwatch.GetTimestamp(); private ReadOnlyMemory _cachedPingMessage; + private volatile bool _connectionAborted; /// /// Initializes a new instance of the class. @@ -99,6 +100,12 @@ namespace Microsoft.AspNetCore.SignalR return new ValueTask(WriteSlowAsync(message)); } + if (_connectionAborted) + { + _writeLock.Release(); + return default; + } + // This method should never throw synchronously var task = WriteCore(message); @@ -129,6 +136,12 @@ namespace Microsoft.AspNetCore.SignalR return new ValueTask(WriteSlowAsync(message)); } + if (_connectionAborted) + { + _writeLock.Release(); + return default; + } + // This method should never throw synchronously var task = WriteCore(message); @@ -158,6 +171,8 @@ namespace Microsoft.AspNetCore.SignalR { Log.FailedWritingMessage(_logger, ex); + Abort(); + return new ValueTask(new FlushResult(isCanceled: false, isCompleted: true)); } } @@ -175,6 +190,8 @@ namespace Microsoft.AspNetCore.SignalR { Log.FailedWritingMessage(_logger, ex); + Abort(); + return new ValueTask(new FlushResult(isCanceled: false, isCompleted: true)); } } @@ -188,6 +205,8 @@ namespace Microsoft.AspNetCore.SignalR catch (Exception ex) { Log.FailedWritingMessage(_logger, ex); + + Abort(); } finally { @@ -201,6 +220,11 @@ namespace Microsoft.AspNetCore.SignalR await _writeLock.WaitAsync(); try { + if (_connectionAborted) + { + return; + } + // Failed to get the lock immediately when entering WriteAsync so await until it is available await WriteCore(message); @@ -208,6 +232,8 @@ namespace Microsoft.AspNetCore.SignalR catch (Exception ex) { Log.FailedWritingMessage(_logger, ex); + + Abort(); } finally { @@ -219,6 +245,11 @@ namespace Microsoft.AspNetCore.SignalR { try { + if (_connectionAborted) + { + return; + } + // Failed to get the lock immediately when entering WriteAsync so await until it is available await _writeLock.WaitAsync(); @@ -227,6 +258,8 @@ namespace Microsoft.AspNetCore.SignalR catch (Exception ex) { Log.FailedWritingMessage(_logger, ex); + + Abort(); } finally { @@ -250,6 +283,11 @@ namespace Microsoft.AspNetCore.SignalR { try { + if (_connectionAborted) + { + return; + } + await _connectionContext.Transport.Output.WriteAsync(_cachedPingMessage); Log.SentPing(_logger); @@ -257,6 +295,8 @@ namespace Microsoft.AspNetCore.SignalR catch (Exception ex) { Log.FailedWritingMessage(_logger, ex); + + Abort(); } finally { @@ -293,6 +333,12 @@ namespace Microsoft.AspNetCore.SignalR /// public virtual void Abort() { + _connectionAborted = true; + + // Cancel any current writes or writes that are about to happen and have already gone past the _connectionAborted bool + // We have to do this outside of the lock otherwise it could hang if the write is observing backpressure + _connectionContext.Transport.Output.CancelPendingFlush(); + // If we already triggered the token then noop, this isn't thread safe but it's good enough // to avoid spawning a new task in the most common cases if (_connectionAbortedTokenSource.IsCancellationRequested) @@ -423,9 +469,24 @@ namespace Microsoft.AspNetCore.SignalR internal Task AbortAsync() { Abort(); + + // Acquire lock to make sure all writes are completed + if (!_writeLock.Wait(0)) + { + return AbortAsyncSlow(); + } + + _writeLock.Release(); return _abortCompletedTcs.Task; } + private async Task AbortAsyncSlow() + { + await _writeLock.WaitAsync(); + _writeLock.Release(); + await _abortCompletedTcs.Task; + } + private void KeepAliveTick() { var timestamp = Stopwatch.GetTimestamp(); diff --git a/src/SignalR/server/SignalR/test/HubConnectionHandlerTests.cs b/src/SignalR/server/SignalR/test/HubConnectionHandlerTests.cs index 06fa3841eb..c310087095 100644 --- a/src/SignalR/server/SignalR/test/HubConnectionHandlerTests.cs +++ b/src/SignalR/server/SignalR/test/HubConnectionHandlerTests.cs @@ -79,9 +79,11 @@ namespace Microsoft.AspNetCore.SignalR.Tests { var connectionHandlerTask = await client.ConnectAsync(connectionHandler); - await client.InvokeAsync(nameof(AbortHub.Kill)); + await client.SendInvocationAsync(nameof(AbortHub.Kill)); await connectionHandlerTask.OrTimeout(); + + Assert.Null(client.TryRead()); } } From 57e68b069e4fe83720565717375a1e1a3bfeb74b Mon Sep 17 00:00:00 2001 From: Brennan Conroy Date: Fri, 11 Oct 2019 01:46:11 +0000 Subject: [PATCH 2/4] Merged PR 3557: Revert "Wait to Complete Pipe" --- eng/PatchConfig.props | 6 -- .../ts/FunctionalTests/selenium/run-tests.ts | 23 +------ .../src/Internal/HttpConnectionContext.cs | 28 +-------- .../src/Internal/HttpConnectionDispatcher.cs | 15 ++--- .../src/Internal/TaskExtensions.cs | 27 -------- src/SignalR/common/Shared/PipeWriterStream.cs | 12 +--- .../server/Core/src/HubConnectionContext.cs | 61 ------------------- .../SignalR/test/HubConnectionHandlerTests.cs | 4 +- 8 files changed, 12 insertions(+), 164 deletions(-) delete mode 100644 src/SignalR/common/Http.Connections/src/Internal/TaskExtensions.cs diff --git a/eng/PatchConfig.props b/eng/PatchConfig.props index 83f2b24497..3ba7babc57 100644 --- a/eng/PatchConfig.props +++ b/eng/PatchConfig.props @@ -44,10 +44,4 @@ Later on, this will be checked using this condition: Microsoft.AspNetCore.CookiePolicy; - - - Microsoft.AspNetCore.Http.Connections; - Microsoft.AspNetCore.SignalR.Core; - - diff --git a/src/SignalR/clients/ts/FunctionalTests/selenium/run-tests.ts b/src/SignalR/clients/ts/FunctionalTests/selenium/run-tests.ts index 6f548f1534..c74603b12c 100644 --- a/src/SignalR/clients/ts/FunctionalTests/selenium/run-tests.ts +++ b/src/SignalR/clients/ts/FunctionalTests/selenium/run-tests.ts @@ -1,8 +1,7 @@ import { ChildProcess, spawn } from "child_process"; -import * as _fs from "fs"; +import * as fs from "fs"; import { EOL } from "os"; import * as path from "path"; -import { promisify } from "util"; import { PassThrough, Readable } from "stream"; import { run } from "../../webdriver-tap-runner/lib"; @@ -10,16 +9,6 @@ import { run } from "../../webdriver-tap-runner/lib"; import * as _debug from "debug"; const debug = _debug("signalr-functional-tests:run"); -const ARTIFACTS_DIR = path.resolve(__dirname, "..", "..", "..", "..", "artifacts"); -const LOGS_DIR = path.resolve(ARTIFACTS_DIR, "logs"); - -// Promisify things from fs we want to use. -const fs = { - createWriteStream: _fs.createWriteStream, - exists: promisify(_fs.exists), - mkdir: promisify(_fs.mkdir), -}; - process.on("unhandledRejection", (reason) => { console.error(`Unhandled promise rejection: ${reason}`); process.exit(1); @@ -113,13 +102,6 @@ if (chromePath) { try { const serverPath = path.resolve(__dirname, "..", "bin", configuration, "netcoreapp2.1", "FunctionalTests.dll"); - if (!await fs.exists(ARTIFACTS_DIR)) { - await fs.mkdir(ARTIFACTS_DIR); - } - if (!await fs.exists(LOGS_DIR)) { - await fs.mkdir(LOGS_DIR); - } - debug(`Launching Functional Test Server: ${serverPath}`); const dotnet = spawn("dotnet", [serverPath], { env: { @@ -135,9 +117,6 @@ if (chromePath) { } } - const logStream = fs.createWriteStream(path.resolve(LOGS_DIR, "ts.functionaltests.dotnet.log")); - dotnet.stdout.pipe(logStream); - process.on("SIGINT", cleanup); process.on("exit", cleanup); diff --git a/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionContext.cs b/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionContext.cs index 425c15d76e..045e821ee1 100644 --- a/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionContext.cs +++ b/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionContext.cs @@ -274,35 +274,13 @@ namespace Microsoft.AspNetCore.Http.Connections.Internal // Cancel any pending flushes from back pressure Application?.Output.CancelPendingFlush(); - // Normally it isn't safe to try and acquire this lock because the Send can hold onto it for a long time if there is backpressure - // It is safe to wait for this lock now because the Send will be in one of 4 states - // 1. In the middle of a write which is in the middle of being canceled by the CancelPendingFlush above, when it throws - // an OperationCanceledException it will complete the PipeWriter which will make any other Send waiting on the lock - // throw an InvalidOperationException if they call Write - // 2. About to write and see that there is a pending cancel from the CancelPendingFlush, go to 1 to see what happens - // 3. Enters the Send and sees the Dispose state from DisposeAndRemoveAsync and releases the lock - // 4. No Send in progress - await WriteLock.WaitAsync(); - try - { - // Complete the applications read loop - Application?.Output.Complete(transportTask.Exception?.InnerException); - } - finally - { - WriteLock.Release(); - } - - Log.WaitingForTransportAndApplication(_logger, TransportType); - - // Wait for application so we can complete the writer safely - await applicationTask.NoThrow(); - - // Shutdown application side now that it's finished + // Shutdown both sides and wait for nothing Transport?.Output.Complete(applicationTask.Exception?.InnerException); + Application?.Output.Complete(transportTask.Exception?.InnerException); try { + Log.WaitingForTransportAndApplication(_logger, TransportType); // A poorly written application *could* in theory get stuck forever and it'll show up as a memory leak await Task.WhenAll(applicationTask, transportTask); } diff --git a/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs b/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs index 2d8b5c24ad..50910bccfe 100644 --- a/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs +++ b/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs @@ -511,14 +511,6 @@ namespace Microsoft.AspNetCore.Http.Connections.Internal context.Response.StatusCode = StatusCodes.Status404NotFound; context.Response.ContentType = "text/plain"; - - // There are no writes anymore (since this is the write "loop") - // So it is safe to complete the writer - // We complete the writer here because we already have the WriteLock acquired - // and it's unsafe to complete outside of the lock - // Other code isn't guaranteed to be able to acquire the lock before another write - // even if CancelPendingFlush is called, and the other write could hang if there is backpressure - connection.Application.Output.Complete(); return; } @@ -557,8 +549,11 @@ namespace Microsoft.AspNetCore.Http.Connections.Internal Log.TerminatingConection(_logger); - // Dispose the connection, but don't wait for it. We assign it here so we can wait in tests - connection.DisposeAndRemoveTask = _manager.DisposeAndRemoveAsync(connection, closeGracefully: false); + // Complete the receiving end of the pipe + connection.Application.Output.Complete(); + + // Dispose the connection gracefully, but don't wait for it. We assign it here so we can wait in tests + connection.DisposeAndRemoveTask = _manager.DisposeAndRemoveAsync(connection, closeGracefully: true); context.Response.StatusCode = StatusCodes.Status202Accepted; context.Response.ContentType = "text/plain"; diff --git a/src/SignalR/common/Http.Connections/src/Internal/TaskExtensions.cs b/src/SignalR/common/Http.Connections/src/Internal/TaskExtensions.cs deleted file mode 100644 index a901379b75..0000000000 --- a/src/SignalR/common/Http.Connections/src/Internal/TaskExtensions.cs +++ /dev/null @@ -1,27 +0,0 @@ -// Copyright (c) .NET Foundation. All rights reserved. -// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. - -using System.Runtime.CompilerServices; - -namespace System.Threading.Tasks -{ - internal static class TaskExtensions - { - public static async Task NoThrow(this Task task) - { - await new NoThrowAwaiter(task); - } - } - - internal readonly struct NoThrowAwaiter : ICriticalNotifyCompletion - { - private readonly Task _task; - public NoThrowAwaiter(Task task) { _task = task; } - public NoThrowAwaiter GetAwaiter() => this; - public bool IsCompleted => _task.IsCompleted; - // Observe exception - public void GetResult() { _ = _task.Exception; } - public void OnCompleted(Action continuation) => _task.GetAwaiter().OnCompleted(continuation); - public void UnsafeOnCompleted(Action continuation) => OnCompleted(continuation); - } -} diff --git a/src/SignalR/common/Shared/PipeWriterStream.cs b/src/SignalR/common/Shared/PipeWriterStream.cs index 245731bfd9..eb5b6d5add 100644 --- a/src/SignalR/common/Shared/PipeWriterStream.cs +++ b/src/SignalR/common/Shared/PipeWriterStream.cs @@ -1,4 +1,4 @@ -// Copyright (c) .NET Foundation. All rights reserved. +// Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; @@ -76,15 +76,7 @@ namespace System.IO.Pipelines _length += source.Length; var task = _pipeWriter.WriteAsync(source); - if (task.IsCompletedSuccessfully) - { - // Cancellation can be triggered by PipeWriter.CancelPendingFlush - if (task.Result.IsCanceled) - { - throw new OperationCanceledException(); - } - } - else if (!task.IsCompletedSuccessfully) + if (!task.IsCompletedSuccessfully) { return WriteSlowAsync(task); } diff --git a/src/SignalR/server/Core/src/HubConnectionContext.cs b/src/SignalR/server/Core/src/HubConnectionContext.cs index 085249438a..5a1049e780 100644 --- a/src/SignalR/server/Core/src/HubConnectionContext.cs +++ b/src/SignalR/server/Core/src/HubConnectionContext.cs @@ -33,7 +33,6 @@ namespace Microsoft.AspNetCore.SignalR private long _lastSendTimestamp = Stopwatch.GetTimestamp(); private ReadOnlyMemory _cachedPingMessage; - private volatile bool _connectionAborted; /// /// Initializes a new instance of the class. @@ -100,12 +99,6 @@ namespace Microsoft.AspNetCore.SignalR return new ValueTask(WriteSlowAsync(message)); } - if (_connectionAborted) - { - _writeLock.Release(); - return default; - } - // This method should never throw synchronously var task = WriteCore(message); @@ -136,12 +129,6 @@ namespace Microsoft.AspNetCore.SignalR return new ValueTask(WriteSlowAsync(message)); } - if (_connectionAborted) - { - _writeLock.Release(); - return default; - } - // This method should never throw synchronously var task = WriteCore(message); @@ -171,8 +158,6 @@ namespace Microsoft.AspNetCore.SignalR { Log.FailedWritingMessage(_logger, ex); - Abort(); - return new ValueTask(new FlushResult(isCanceled: false, isCompleted: true)); } } @@ -190,8 +175,6 @@ namespace Microsoft.AspNetCore.SignalR { Log.FailedWritingMessage(_logger, ex); - Abort(); - return new ValueTask(new FlushResult(isCanceled: false, isCompleted: true)); } } @@ -205,8 +188,6 @@ namespace Microsoft.AspNetCore.SignalR catch (Exception ex) { Log.FailedWritingMessage(_logger, ex); - - Abort(); } finally { @@ -220,11 +201,6 @@ namespace Microsoft.AspNetCore.SignalR await _writeLock.WaitAsync(); try { - if (_connectionAborted) - { - return; - } - // Failed to get the lock immediately when entering WriteAsync so await until it is available await WriteCore(message); @@ -232,8 +208,6 @@ namespace Microsoft.AspNetCore.SignalR catch (Exception ex) { Log.FailedWritingMessage(_logger, ex); - - Abort(); } finally { @@ -245,11 +219,6 @@ namespace Microsoft.AspNetCore.SignalR { try { - if (_connectionAborted) - { - return; - } - // Failed to get the lock immediately when entering WriteAsync so await until it is available await _writeLock.WaitAsync(); @@ -258,8 +227,6 @@ namespace Microsoft.AspNetCore.SignalR catch (Exception ex) { Log.FailedWritingMessage(_logger, ex); - - Abort(); } finally { @@ -283,11 +250,6 @@ namespace Microsoft.AspNetCore.SignalR { try { - if (_connectionAborted) - { - return; - } - await _connectionContext.Transport.Output.WriteAsync(_cachedPingMessage); Log.SentPing(_logger); @@ -295,8 +257,6 @@ namespace Microsoft.AspNetCore.SignalR catch (Exception ex) { Log.FailedWritingMessage(_logger, ex); - - Abort(); } finally { @@ -333,12 +293,6 @@ namespace Microsoft.AspNetCore.SignalR /// public virtual void Abort() { - _connectionAborted = true; - - // Cancel any current writes or writes that are about to happen and have already gone past the _connectionAborted bool - // We have to do this outside of the lock otherwise it could hang if the write is observing backpressure - _connectionContext.Transport.Output.CancelPendingFlush(); - // If we already triggered the token then noop, this isn't thread safe but it's good enough // to avoid spawning a new task in the most common cases if (_connectionAbortedTokenSource.IsCancellationRequested) @@ -469,24 +423,9 @@ namespace Microsoft.AspNetCore.SignalR internal Task AbortAsync() { Abort(); - - // Acquire lock to make sure all writes are completed - if (!_writeLock.Wait(0)) - { - return AbortAsyncSlow(); - } - - _writeLock.Release(); return _abortCompletedTcs.Task; } - private async Task AbortAsyncSlow() - { - await _writeLock.WaitAsync(); - _writeLock.Release(); - await _abortCompletedTcs.Task; - } - private void KeepAliveTick() { var timestamp = Stopwatch.GetTimestamp(); diff --git a/src/SignalR/server/SignalR/test/HubConnectionHandlerTests.cs b/src/SignalR/server/SignalR/test/HubConnectionHandlerTests.cs index c310087095..06fa3841eb 100644 --- a/src/SignalR/server/SignalR/test/HubConnectionHandlerTests.cs +++ b/src/SignalR/server/SignalR/test/HubConnectionHandlerTests.cs @@ -79,11 +79,9 @@ namespace Microsoft.AspNetCore.SignalR.Tests { var connectionHandlerTask = await client.ConnectAsync(connectionHandler); - await client.SendInvocationAsync(nameof(AbortHub.Kill)); + await client.InvokeAsync(nameof(AbortHub.Kill)); await connectionHandlerTask.OrTimeout(); - - Assert.Null(client.TryRead()); } } From 147f950686f9f3201be8a2913e811fb4d8d338cf Mon Sep 17 00:00:00 2001 From: John Luo Date: Wed, 20 Nov 2019 17:10:59 -0800 Subject: [PATCH 3/4] Update branding to 2.1.15 (#17273) --- build/submodules.props | 2 +- eng/Baseline.Designer.props | 6 +++--- eng/Baseline.xml | 6 +++--- eng/PatchConfig.props | 4 ++++ .../ArchiveBaseline.2.1.14.txt | 1 + .../Archive.CiServer.Patch/ArchiveBaseline.2.1.14.txt | 1 + version.props | 2 +- 7 files changed, 14 insertions(+), 8 deletions(-) create mode 100644 src/PackageArchive/Archive.CiServer.Patch.Compat/ArchiveBaseline.2.1.14.txt create mode 100644 src/PackageArchive/Archive.CiServer.Patch/ArchiveBaseline.2.1.14.txt diff --git a/build/submodules.props b/build/submodules.props index a088430493..4370c412c7 100644 --- a/build/submodules.props +++ b/build/submodules.props @@ -37,6 +37,6 @@ - + diff --git a/eng/Baseline.Designer.props b/eng/Baseline.Designer.props index c82f441f14..29e2efcf98 100644 --- a/eng/Baseline.Designer.props +++ b/eng/Baseline.Designer.props @@ -2,7 +2,7 @@ $(MSBuildAllProjects);$(MSBuildThisFileFullPath) - 2.1.13 + 2.1.14 @@ -235,7 +235,7 @@ - 2.1.2 + 2.1.14 @@ -1219,7 +1219,7 @@ - 2.1.1 + 2.1.14 diff --git a/eng/Baseline.xml b/eng/Baseline.xml index 8ab070f7e9..69761f4e19 100644 --- a/eng/Baseline.xml +++ b/eng/Baseline.xml @@ -4,7 +4,7 @@ This file contains a list of all the packages and their versions which were rele build of ASP.NET Core 2.1.x. Update this list when preparing for a new patch. --> - + @@ -30,7 +30,7 @@ build of ASP.NET Core 2.1.x. Update this list when preparing for a new patch. - + @@ -124,7 +124,7 @@ build of ASP.NET Core 2.1.x. Update this list when preparing for a new patch. - + \ No newline at end of file diff --git a/eng/PatchConfig.props b/eng/PatchConfig.props index 3ba7babc57..513d6f73b1 100644 --- a/eng/PatchConfig.props +++ b/eng/PatchConfig.props @@ -44,4 +44,8 @@ Later on, this will be checked using this condition: Microsoft.AspNetCore.CookiePolicy; + + + + diff --git a/src/PackageArchive/Archive.CiServer.Patch.Compat/ArchiveBaseline.2.1.14.txt b/src/PackageArchive/Archive.CiServer.Patch.Compat/ArchiveBaseline.2.1.14.txt new file mode 100644 index 0000000000..8b13789179 --- /dev/null +++ b/src/PackageArchive/Archive.CiServer.Patch.Compat/ArchiveBaseline.2.1.14.txt @@ -0,0 +1 @@ + diff --git a/src/PackageArchive/Archive.CiServer.Patch/ArchiveBaseline.2.1.14.txt b/src/PackageArchive/Archive.CiServer.Patch/ArchiveBaseline.2.1.14.txt new file mode 100644 index 0000000000..8b13789179 --- /dev/null +++ b/src/PackageArchive/Archive.CiServer.Patch/ArchiveBaseline.2.1.14.txt @@ -0,0 +1 @@ + diff --git a/version.props b/version.props index ed48958394..6f78c654d6 100644 --- a/version.props +++ b/version.props @@ -2,7 +2,7 @@ 2 1 - 14 + 15 servicing Servicing t000 From 049cdec742ac8a582d6a5d85ed4ae590b42db681 Mon Sep 17 00:00:00 2001 From: Matt Mitchell Date: Mon, 2 Dec 2019 21:30:51 -0800 Subject: [PATCH 4/4] Unpin package (#17536) --- build/dependencies.props | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/build/dependencies.props b/build/dependencies.props index a9edc6f2b9..405c2e1ec2 100644 --- a/build/dependencies.props +++ b/build/dependencies.props @@ -4,7 +4,7 @@ 2.1.12 2.1.12 - + 4.5.0 @@ -199,7 +199,6 @@ 1.6.0 4.5.2 4.3.0 - 4.5.0 4.5.0 4.5.0 4.5.1