phxnsharp commented on a change in pull request #2395:
URL: https://github.com/apache/thrift/pull/2395#discussion_r639625433
##########
File path: lib/netstd/Thrift/Server/TThreadPoolAsyncServer.cs
##########
@@ -218,11 +218,11 @@ public override async Task ServeAsync(CancellationToken
cancellationToken)
/// threadContext will be a TTransport instance
/// </summary>
/// <param name="threadContext"></param>
- private void Execute(object threadContext)
+ private async void ExecuteAsync(TTransport client)
{
Review comment:
We use it only in specific circumstances like this one where it matches
with the reality of what you are creating. In this case we are effectively
starting a new "thread" (though it isn't an actual thick thread) and nothing
will be listening to the return value. Having the signature makes it obvious to
everyone working on the code that nothing should be returned and no exceptions
should be let to exit the function.
However, there is also value in writing it the way you have in your version.
It lets the caller decide if they want to know when the task/thread is done and
could be helpful in unit testing. I fully support the change as this code
should match your style.
--
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]
