From c618999c94f76af4e665b95393ef8b70e29b982e Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Fri, 8 Mar 2024 11:54:19 -0500 Subject: [PATCH 1/2] Backlog: move to full synchronous behavior since we're on a dedicated thread This went through a few iterations early on, but in the current form the issue is that we can await hop to a thread pool thread and have our wait blocking there - not awesome. We want to avoid that entirely and do a full sync path here. --- src/StackExchange.Redis/PhysicalBridge.cs | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/StackExchange.Redis/PhysicalBridge.cs b/src/StackExchange.Redis/PhysicalBridge.cs index f870cf340..522e2fcc9 100644 --- a/src/StackExchange.Redis/PhysicalBridge.cs +++ b/src/StackExchange.Redis/PhysicalBridge.cs @@ -904,7 +904,7 @@ private void StartBacklogProcessor() // to unblock the thread-pool when there could be sync-over-async callers. Note that in reality, // the initial "enough" of the back-log processor is typically sync, which means that the thread // we start is actually useful, despite thinking "but that will just go async and back to the pool" - var thread = new Thread(s => ((PhysicalBridge)s!).ProcessBacklogAsync().RedisFireAndForget()) + var thread = new Thread(s => ((PhysicalBridge)s!).ProcessBacklog()) { IsBackground = true, // don't keep process alive (also: act like the thread-pool used to) Name = "StackExchange.Redis Backlog", // help anyone looking at thread-dumps @@ -985,7 +985,7 @@ internal enum BacklogStatus : byte /// Process the backlog(s) in play if any. /// This means flushing commands to an available/active connection (if any) or spinning until timeout if not. /// - private async Task ProcessBacklogAsync() + private void ProcessBacklog() { _backlogStatus = BacklogStatus.Starting; try @@ -997,7 +997,7 @@ private async Task ProcessBacklogAsync() // TODO: vNext handoff this backlog to another primary ("can handle everything") connection // and remove any per-server commands. This means we need to track a bit of whether something // was server-endpoint-specific in PrepareToPushMessageToBridge (was the server ref null or not) - await ProcessBridgeBacklogAsync().ForAwait(); + ProcessBridgeBacklog(); } // The cost of starting a new thread is high, and we can bounce in and out of the backlog a lot. @@ -1070,7 +1070,7 @@ private async Task ProcessBacklogAsync() /// private readonly AutoResetEvent _backlogAutoReset = new AutoResetEvent(false); - private async Task ProcessBridgeBacklogAsync() + private void ProcessBridgeBacklog() { // Importantly: don't assume we have a physical connection here // We are very likely to hit a state where it's not re-established or even referenced here @@ -1097,10 +1097,10 @@ private async Task ProcessBridgeBacklogAsync() // try and get the lock; if unsuccessful, retry #if NETCOREAPP - gotLock = await _singleWriterMutex.WaitAsync(TimeoutMilliseconds).ForAwait(); + gotLock = _singleWriterMutex.Wait(TimeoutMilliseconds); if (gotLock) break; // got the lock; now go do something with it #else - token = await _singleWriterMutex.TryWaitAsync().ForAwait(); + token = _singleWriterMutex.TryWait(); if (token.Success) break; // got the lock; now go do something with it #endif } @@ -1132,7 +1132,9 @@ private async Task ProcessBridgeBacklogAsync() if (result == WriteResult.Success) { _backlogStatus = BacklogStatus.Flushing; - result = await physical.FlushAsync(false).ForAwait(); +#pragma warning disable CS0618 // Type or member is obsolete + result = physical.FlushSync(false, TimeoutMilliseconds); +#pragma warning restore CS0618 // Type or member is obsolete } _backlogStatus = BacklogStatus.MarkingInactive; From 9f35a2d2e54ae533c3f3ebc91c03ac2e03874bef Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Fri, 8 Mar 2024 12:05:21 -0500 Subject: [PATCH 2/2] Add release notes --- docs/ReleaseNotes.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/ReleaseNotes.md b/docs/ReleaseNotes.md index fa9a2d8af..11998ea01 100644 --- a/docs/ReleaseNotes.md +++ b/docs/ReleaseNotes.md @@ -9,6 +9,7 @@ Current package versions: ## Unreleased - Add new `LoggingTunnel` API; see https://stackexchange.github.io/StackExchange.Redis/Logging ([#2660 by mgravell](https://github.com/StackExchange/StackExchange.Redis/pull/2660)) +- Fix [#2664](https://github.com/StackExchange/StackExchange.Redis/issues/2664): Move ProcessBacklog to fully sync to prevent thread pool hopping and blocking on awaits ([#2667 by NickCraver](https://github.com/StackExchange/StackExchange.Redis/pull/2667)) ## 2.7.27