Skip to content

Commit e40b218

Browse files
Fix race with CTS disposing (#11757)
1 parent 4481046 commit e40b218

File tree

3 files changed

+61
-23
lines changed

3 files changed

+61
-23
lines changed

src/SignalR/common/Http.Connections/src/Internal/HttpConnectionContext.cs

Lines changed: 40 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -253,7 +253,7 @@ private async Task WaitOnTasks(Task applicationTask, Task transportTask, bool cl
253253

254254
if (TransportType == HttpTransportType.WebSockets)
255255
{
256-
// The websocket transport will close the application output automatically when reading is cancelled
256+
// The websocket transport will close the application output automatically when reading is canceled
257257
Cancellation?.Cancel();
258258
}
259259
else
@@ -443,6 +443,45 @@ private void FailActivationUnsynchronized(HttpContext nonClonedContext, ILogger
443443
}
444444
}
445445

446+
internal async Task<bool> CancelPreviousPoll(HttpContext context)
447+
{
448+
CancellationTokenSource cts;
449+
lock (_stateLock)
450+
{
451+
// Need to sync cts access with DisposeAsync as that will dispose the cts
452+
if (Status == HttpConnectionStatus.Disposed)
453+
{
454+
cts = null;
455+
}
456+
else
457+
{
458+
cts = Cancellation;
459+
Cancellation = null;
460+
}
461+
}
462+
463+
using (cts)
464+
{
465+
// Cancel the previous request
466+
cts?.Cancel();
467+
468+
try
469+
{
470+
// Wait for the previous request to drain
471+
await PreviousPollTask;
472+
}
473+
catch (OperationCanceledException)
474+
{
475+
// Previous poll canceled due to connection closing, close this poll too
476+
context.Response.ContentType = "text/plain";
477+
context.Response.StatusCode = StatusCodes.Status204NoContent;
478+
return false;
479+
}
480+
481+
return true;
482+
}
483+
}
484+
446485
public void MarkInactive()
447486
{
448487
lock (_stateLock)

src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs

Lines changed: 7 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,7 @@ private async Task ExecuteAsync(HttpContext context, ConnectionDelegate connecti
164164

165165
Log.EstablishedConnection(_logger);
166166

167-
// Allow the reads to be cancelled
167+
// Allow the reads to be canceled
168168
connection.Cancellation = new CancellationTokenSource();
169169

170170
var ws = new WebSocketsServerTransport(options.WebSockets, connection.Application, connection, _loggerFactory);
@@ -189,28 +189,15 @@ private async Task ExecuteAsync(HttpContext context, ConnectionDelegate connecti
189189
return;
190190
}
191191

192-
// Create a new Tcs every poll to keep track of the poll finishing, so we can properly wait on previous polls
193-
var currentRequestTcs = new TaskCompletionSource<object>(TaskCreationOptions.RunContinuationsAsynchronously);
194-
195-
using (connection.Cancellation)
192+
if (!await connection.CancelPreviousPoll(context))
196193
{
197-
// Cancel the previous request
198-
connection.Cancellation?.Cancel();
199-
200-
try
201-
{
202-
// Wait for the previous request to drain
203-
await connection.PreviousPollTask;
204-
}
205-
catch (OperationCanceledException)
206-
{
207-
// Previous poll canceled due to connection closing, close this poll too
208-
context.Response.ContentType = "text/plain";
209-
context.Response.StatusCode = StatusCodes.Status204NoContent;
210-
return;
211-
}
194+
// Connection closed. It's already set the response status code.
195+
return;
212196
}
213197

198+
// Create a new Tcs every poll to keep track of the poll finishing, so we can properly wait on previous polls
199+
var currentRequestTcs = new TaskCompletionSource<object>(TaskCreationOptions.RunContinuationsAsynchronously);
200+
214201
if (!connection.TryActivateLongPollingConnection(
215202
connectionDelegate, context, options.LongPolling.PollTimeout,
216203
currentRequestTcs.Task, _loggerFactory, _logger))

src/SignalR/common/Http.Connections/test/HttpConnectionDispatcherTests.cs

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1148,9 +1148,22 @@ public async Task RequestToActiveConnectionIdKillsPreviousConnectionLongPolling(
11481148
Assert.True(request1.IsCompleted);
11491149

11501150
request1 = dispatcher.ExecuteAsync(context1, options, app);
1151+
var count = 0;
1152+
// Wait until the request has started internally
1153+
while (connection.TransportTask.IsCompleted && count < 50)
1154+
{
1155+
count++;
1156+
await Task.Delay(15);
1157+
}
1158+
if (count == 50)
1159+
{
1160+
Assert.True(false, "Poll took too long to start");
1161+
}
1162+
11511163
var request2 = dispatcher.ExecuteAsync(context2, options, app);
11521164

1153-
await request1;
1165+
// Wait for poll to be canceled
1166+
await request1.OrTimeout();
11541167

11551168
Assert.Equal(StatusCodes.Status204NoContent, context1.Response.StatusCode);
11561169
Assert.Equal(HttpConnectionStatus.Active, connection.Status);
@@ -1164,7 +1177,6 @@ public async Task RequestToActiveConnectionIdKillsPreviousConnectionLongPolling(
11641177
}
11651178

11661179
[Fact]
1167-
[Flaky("https://github.com/aspnet/AspNetCore-Internal/issues/2040", "All")]
11681180
public async Task MultipleRequestsToActiveConnectionId409ForLongPolling()
11691181
{
11701182
using (StartVerifiableLog())

0 commit comments

Comments
 (0)