Skip to content

Start connections on a fresh ExecutionContext #29995

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Start connections on a fresh ExecutionContext #29995

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3756e8c
Start connections on a fresh ExecutionContext
davidfowl Feb 8, 2021
d94a603
Update src/SignalR/common/Http.Connections/src/Internal/HttpConnectio…
davidfowl Feb 9, 2021
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
41 changes: 35 additions & 6 deletions src/SignalR/common/Http.Connections/src/Internal/HttpConnectionContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ internal class HttpConnectionContext : ConnectionContext,
IHttpContextFeature,
IHttpTransportFeature,
IConnectionInherentKeepAliveFeature,
IConnectionLifetimeFeature
IConnectionLifetimeFeature,
IThreadPoolWorkItem
{
private static long _tenSeconds = TimeSpan.FromSeconds(10).Ticks;

Expand All @@ -47,6 +48,10 @@ internal class HttpConnectionContext : ConnectionContext,
private IDictionary<object, object> _items;
private CancellationTokenSource _connectionClosedTokenSource;

// No need to RunContinuationsAsynchronously since we're at the tail of a threadpool thread
private readonly TaskCompletionSource _connectionDelegateTcs = new ();
private ConnectionDelegate _connectionDelegate;

private CancellationTokenSource _sendCts;
private bool _activeSend;
private long _startedSendTime;
Expand Down Expand Up @@ -547,12 +552,13 @@ private async Task ExecuteApplication(ConnectionDelegate connectionDelegate)
// Verify some initialization invariants
Debug.Assert(TransportType != HttpTransportType.None, "Transport has not been initialized yet");

// Jump onto the thread pool thread so blocking user code doesn't block the setup of the
// connection and transport
await AwaitableThreadPool.Yield();
_connectionDelegate = connectionDelegate;

// Queue the connection for execution
ThreadPool.UnsafeQueueUserWorkItem(this, preferLocal: false);

// Running this in an async method turns sync exceptions into async ones
await connectionDelegate(this);
// Wait for that task to finish signaling the end of the application execution
await _connectionDelegateTcs.Task;
Comment on lines +555 to +561
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you choose this route rather than:

await AwaitableThreadPool.Yield();

Task t;
using (ExecutionContext.SuppressFlow())
    t = connectionDelegate(this);

await t;

?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generally don't like using suppress flow. I prefer the more explicit call that doesn't capture the ExecutionContext.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the fact that SuppressFlow throws if the flow is already suppressed makes it annoying. You need to always code around the fact that somebody further up the stack might have already suppressed it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to always code around the fact that somebody further up the stack might have already suppressed it.

There's no user code higher up the stack here. You shouldn't need to check or need additional code around it.

I generally don't like using suppress flow

Understood in general. In practice here, though, the code using SuppressFlow would seem to be less convoluted and (I'd need to measure to know for sure) less expensive.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no user code higher up the stack here. You shouldn't need to check or need additional code around it.

There is. That's where the async locals come from and why this change is happening in the first place.

Understood in general. In practice here, though, the code using SuppressFlow would seem to be less convoluted and (I'd need to measure to know for sure) less expensive.

I think the code does look less convoluted but we use this pattern in other places in ASP.NET Core and it's a stylistic thing. It's the same reason I prefer using *Unsafe APIs instead of suppressing the flow in various cases.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you saying:

ExecutionContext.SuppressFlow();

await Task.Yield();

ExecutionContext.SuppressFlow();

Works?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other thing I don't like about that is that it's too subtle vs a call that says "This will not capture the execution context" and the code sucks a little bit because. You know what I really want? Task.UnsafeRun. That would work here as well.

Copy link
Member

@stephentoub stephentoub Feb 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you saying ... works?

Forgetting for the moment the fact that Task.Yield() respects current SynchronizationContext as well (which your AwaitableThreadPool.Yield() does not), yes... suppression itself doesn't flow, and the thread pool ensures a work item can't leak a change to suppression to the next work item.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You know what I really want? Task.UnsafeRun. That would work here as well.

Why do you want to queue to the ThreadPool? I thought this this change was strictly about the ExecutionContext.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this change removed the old await AwaitableThreadPool.Yield(); Never mind me.

}

internal void StartSendCancellation()
Expand Down Expand Up @@ -589,6 +595,29 @@ internal void StopSendCancellation()
}
}

public void Execute()
{
async Task ExecuteCore()
{
try
{
await _connectionDelegate(this);

_connectionDelegateTcs.TrySetResult();
}
catch (OperationCanceledException ex)
{
_connectionDelegateTcs.TrySetCanceled(ex.CancellationToken);
}
catch (Exception ex)
{
_connectionDelegateTcs.TrySetException(ex);
}
}

_ = ExecuteCore();
}

private static class Log
{
private static readonly Action<ILogger, string, Exception> _disposingConnection =
Expand Down