phxnsharp commented on a change in pull request #2395:
URL: https://github.com/apache/thrift/pull/2395#discussion_r639655591
##########
File path: lib/netstd/Thrift/Server/TThreadPoolAsyncServer.cs
##########
@@ -177,7 +177,7 @@ public override async Task ServeAsync(CancellationToken
cancellationToken)
try
{
TTransport client = await
ServerTransport.AcceptAsync(cancellationToken);
- ThreadPool.QueueUserWorkItem(this.Execute, client);
+ var ignore = Task.Run(() => ExecuteAsync(client));
Review comment:
Note, I had meant to put in a comment about this but see I forgot.
Task.Run() is actually not entirely required here. As ExecuteAsync is async,
but we ignore the return value, it would be almost equivalent to just call it
without the Task.Run(). The reason I put it here is that this code is executed
in the SynchronizationContext of the caller. If that is a restricted UI thread,
that would put unneeded burden on ExecuteAsync. Task.Run frees the execution to
run on any thread pool thread.
An alternate solution would be to put .ConfigureAwait(false) on every async
call per [Microsoft's
recommendation](https://devblogs.microsoft.com/dotnet/configureawait-faq/).
Arguably, looking at it now, I probably should have done that anyhow out of
best practice.
As an aside, I considered putting those into the rest of this class but
because this class has callbacks into the client code (PreServeAsync), that
might introduce subtle bugs in client code as those would now be called on the
Thread Pool. You may want to consider that in the long run, though, as it frees
your code from being tied to an unknown SynchronizationContext.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
