[GitHub] [thrift] phxnsharp commented on a change in pull request #2395: THRIFT-5419: Update TThreadPoolAsyncServer.Execute to be async/await

[GitBox <gi...@apache.org>]

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:
users@infra.apache.org