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

Fix #2664 Backlog: move to full synchronous behavior since we're on a dedicated thread #2667

Merged
merged 3 commits into from
Mar 11, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/ReleaseNotes.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
16 changes: 9 additions & 7 deletions src/StackExchange.Redis/PhysicalBridge.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
/// </summary>
private async Task ProcessBacklogAsync()
private void ProcessBacklog()
{
_backlogStatus = BacklogStatus.Starting;
try
Expand All @@ -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.
Expand Down Expand Up @@ -1070,7 +1070,7 @@ private async Task ProcessBacklogAsync()
/// </summary>
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
Expand All @@ -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
}
Expand Down Expand Up @@ -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;
Expand Down
Loading