dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.46k stars 10.03k forks source link

[SignalR] Client keepalive check is disabled when I debug my hub server #41673

Open vicancy opened 2 years ago

vicancy commented 2 years ago

Is there an existing issue for this?

Describe the bug

Client ping timeout check is disabled when I debug my hub server

Expected Behavior

I should be able to see OnDisconnectedAsync triggered when client does not send out ping messages

Steps To Reproduce

  1. Create a simple Hub server
  2. Shorten ClientTimeoutInterval to 5 seconds: services.AddSignalR(o=>o.ClientTimeoutInterval=TimeSpan.FromSeconds(5));
  3. Override OnDisconnectedAsync in the hub and insert a breakpoint there
  4. F5 to start debugging my hub
  5. OnDisconnectedAsync is never triggered because KeepAlive check is disabled when Debugger is attached: https://github.com/dotnet/aspnetcore/blob/88ee825bca9978df7f1d8c8c09b71b6dffeeb53b/src/SignalR/server/Core/src/HubConnectionContext.cs#L656

Exceptions (if any)

No response

.NET Version

6.0.203

Anything else?

No response

davidfowl commented 2 years ago

We should probably put all of these checks under a #if DEBUG check

halter73 commented 2 years ago

We should probably put all of these checks under a #if DEBUG check

We do this to make it easy for developers using SignalR don't time out their connection. Trying to test timeouts is a niche use case, and you can generally do testing for that specifically, when not under a debugger.

davidfowl commented 2 years ago

I'm not sure we've proven that 😄

vicancy commented 2 years ago

This sounds inconsistent experience to me though. It is a hidden trick that I don't know until looking into the code. Maybe some explicit config to disable the timeout could be better?

BrennanConroy commented 2 years ago

The scenario where you test OnDisconnectedAsync when the client fails to send a keep alive is very specific IMO, if the client actually disconnects (via StopAsync, LongPoll takes 90 seconds, etc) then OnDisconnectedAsync will be called while debugging. I'm not sure how useful an option for this would be since it's still something people will need to find and set and it's a very specific scenario that can generally be done while not under a debugger. My 2 cents.

davidfowl commented 2 years ago

Feels bad that there's no workaround for this. Even if it's obscure.

halter73 commented 2 years ago

To me it feels a lot like how you can't test what happens when task become unrooted under a debugger making it impossible for library authors like us to write tests about how we treat this condition (which is pretty important because it's one of the few ways to test code that enters a try/finally but never exits) that runs under a debugger.

https://github.com/dotnet/aspnetcore/blob/9a81d79124c46fa883132b48715ab3a08b7425a5/src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpConnectionManagerTests.cs#L22-L29

I don't think we need to add a knob for a niche thing like this. To many settings can be overwhelming. But maybe we can make our if (Debugger.IsAttached) check read a static field that's easily modifiable by a debugger along the lines of what Task does.

https://github.com/dotnet/runtime/blob/327e266efd36608331acdc620e846a889dc6aebc/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs#L180-L182

In our case, the debugger wouldn't be automatically setting the field, so the check would have to be something like if (Debugger.IsAttached && !_ignoreDebugger).

davidfowl commented 2 years ago

What about an appcontext switch or env variable?

halter73 commented 2 years ago

You have a debugger. A static field is manually and programmatically configurable with a debugger. Reflection is an option too. Who's setting appcontext switch or setting an environment variable when they attach a debugger?

davidfowl commented 2 years ago

The setting isn't about the debugger. The setting is the value of the timeout when the debugger is attached.

BrennanConroy commented 2 years ago

I don't think a configurable timeout is a good idea, we already let the user change keep alive interval and server timeout etc. this wouldn't actually do what users would want.

We could have an appcontext switch, but I think it's super low priority.

ghost commented 2 years ago

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

davidfowl commented 2 years ago

This isn't about a configurable timeout, it's a way to turn off the default behavior where debugger attached is assumed to mean infinite.

AbakumovAlexandr commented 1 year ago

Are there any updates on this?

This is really a nonsensical and obscure 'feature'. I was under an impression that disconnecting doesn't work in SignalR at all until doing a long Google search.

We do this to make it easy for developers using SignalR don't time out their connection.

If I do want to not time out connections while debugging - I simply would increase those config timeout options in the Debug mode and it would make the prefect sense for just about everyone using SignalR.

Why on earth there should be undocumented\hidden behavior discrepancies between how the Framework code works in Debug vs Release mode? And moreover, why there's no any config option to disable this lovely 'feature'?

charlesdwmorrison commented 2 months ago

Just want to mention that a Bing search brought me to this issue as well. I'm on .Net8 and the 8.08 versions of the SignalR NuGet packages. And was using SignalR in an NUnit test method. But I think there is not really an issue. So I a posting my solution for anyone else who lands here.

So that was the solution for me: change my "Setup()" method to aysnc and then do awaits like this:

await _hubConnection.StartAsync(); await _hubConnection.InvokeAsync("ExecuteMyMethodOnTheHub");

Note that the hub initialization code does NOT require awaits . . . . so it's easy to miss this.

Why this worked in Run mode, but not in debug mode, I don't understand. But that consistently happened for me. Was driving me crazy.

It seems like maybe the .Net framework should trigger a red-squiggly if you fail to put await in front of _hubConnection.StartAsync();.

SignalR is a great technology, Microsoft did a great job with it and I hope to see it continued to be pushed forward.