search

cyanfish / grpc-dotnet-namedpipes

Named pipe transport for gRPC in C#/.NET
Apache License 2.0
183 stars 47 forks source link

async/await to handle connection loop #42

Closed ikobylinskyi closed 1 year ago

ikobylinskyi commented 1 year ago

In applications with different default synchronization context like WPF, blocking calls may lead to freezes when server endpoint dispatches work to the main thread.

cyanfish commented 1 year ago

Is there a specific issue that was caused by this? As far as I can tell from the code, this shouldn't be an issue as the blocking call happens on a dedicated thread.

As I recall, the reason I did it this way was that I was concerned about high-qps scenarios - in theory task saturation could lead to scenarios where clients try to connect but there's no server connection open as the task continuation for ConnectionLoop could be queued behind other tasks. A dedicated thread avoids that problem.

ikobylinskyi commented 1 year ago

image

This is the situation I started from. Application becomes frozen sometimes when executing endpoint logic which contains Dispatcher.BeginInvoke call. Issue was found with version 1.4.2 of the Nuget package.

As I understood it from your explanation, it could happen only when all threads from ThreadPool are taken. I agree that my changes in StartListenThread and ConnectionLoop will have this issue, and the original solution does not have it.

HandleConnection method in NamedPipeServer was my other thought (actually this method was in thread 2 stack), But later I figured out that this place was modified already to async/await.

It could be that none of my changes in this PR actually help to resolve my case, but it is hard to say for sure. The issue initially reproduced each second cold start. And there are many factors present like dispatcher priorities and other background processes. And as nothing from my application logic was present in a stack trace I though it could be a good idea to get rid of all blocking code. And it looked like these changes helped a bit, the issue started to reproduce less often (aprox once in 15 cold starts). As it did not vanish completely, it might be related to something else.

I think I will try to investigate my problem again, but with package version 2.0.0.

Thanks for your time and explanation of the case I did not consider in my "fix"!

cyanfish commented 1 year ago

Yeah, I would guess this is fixed in 2.0.0. In 1.4.2 RunHandleConnection didn't await.

cyanfish commented 1 year ago

And specifically from your stack traces, (1) is expected, it's just the server threads idling, while (2) is from the RunHandleConnection wait that should be gone in 2.0.0.

ikobylinskyi commented 1 year ago

Hello, @cyanfish. I updated package to version 2.0.0 and it seems everything is good. I think there is no need to have this PR opened. The problem I encountered is somewhere in my application code.

cyanfish commented 1 year ago

Glad to hear 2.0.0 is working, thanks for the update.