Fix race in LongPolling causing flaky tests #8114
Conversation
BrennanConroy
requested review from
analogrelay,
halter73,
davidfowl and
mikaelm12
| @@ -95,7 +98,7 @@ public HttpConnectionContext(string id, IDuplexPipe transport, IDuplexPipe appli | |||
|
|
|||
| public DateTime LastSeenUtc { get; set; } | |||
|
|
|||
| public HttpConnectionStatus Status { get; set; } = HttpConnectionStatus.Inactive; | |||
There was a problem hiding this comment.
Where else is this setter called? It seems weird to protect state transitions in one place, but not elsewhere.
There was a problem hiding this comment.
No, it's used in HttpConnectionContext.DisposeAsync and it was a public API already, so we can't remove it.
| @@ -331,7 +331,8 @@ private async Task ExecuteAsync(HttpContext context, ConnectionDelegate connecti | |||
| // Mark the connection as inactive | |||
| connection.LastSeenUtc = DateTime.UtcNow; | |||
|
|
|||
| connection.Status = HttpConnectionStatus.Inactive; | |||
| // This is done outside a lock because the next poll might be waiting in the lock already and waiting for currentRequestTcs to complete | |||
| Interlocked.CompareExchange(ref connection._status, (int)HttpConnectionStatus.Inactive, (int)HttpConnectionStatus.Active); | |||
There was a problem hiding this comment.
So this only prevents moving from the Disposed to Inactive state, right? Is this by itself enough to fixe the "InvalidOperationException: Reading is not allowed after reader was completed."? Are we sure there's no other way to transition out of the Disposed state?
I think a comment discussing what state transition we're trying to prevent here would be nice.
src/SignalR/common/Http.Connections/src/Internal/HttpConnectionContext.cs
Outdated
Show resolved
Hide resolved
| @@ -309,6 +310,11 @@ private async Task WaitOnTasks(Task applicationTask, Task transportTask, bool cl | |||
| } | |||
| } | |||
|
|
|||
| public bool ChangeState(HttpConnectionStatus from, HttpConnectionStatus to) | |||
| @@ -371,7 +373,7 @@ private async Task ExecuteAsync(HttpContext context, ConnectionDelegate connecti | |||
| } | |||
|
|
|||
| // Mark the connection as active | |||
| connection.Status = HttpConnectionStatus.Active; | |||
| connection.ChangeState(HttpConnectionStatus.Inactive, HttpConnectionStatus.Active); | |||
There was a problem hiding this comment.
Should we just continue on if the state transition fails? Should we assert the transitions are successful?
There was a problem hiding this comment.
I think it's fine to let it continue
There was a problem hiding this comment.
What if you're in the HttpConnectionStatus.Disposed state?
There was a problem hiding this comment.
It can't be because they both use the same lock and there is a check for Disposed state after acquiring the lock in this case.
|
@BrennanConroy I added a commit to this PR to remove the SemaphoreSlim/Interlocked stuff. Tell me if it looks good to you |
…Slim" This reverts commit 53f2ff2.
|
Your change had a lot of issues including making the bug this PR is fixing happen more frequently. I'm reverting it, and if you'd like to refactor Long Polling you can do so at a different time after this fix goes in. |
|
It's your PR. You can just force push my commit away 😉 |
Fixes #6597