Improve SignalR idle timeout example #32027
Conversation
|
|
||
| ```csharp | ||
| using Microsoft.AspNetCore.Components.Server.Circuits; | ||
| using Microsoft.Extensions.Options; | ||
| using Timer = System.Timers.Timer; |
There was a problem hiding this comment.
Is using System.Timers; insufficient? Or does that create ambiguity in the code somewhere?
There was a problem hiding this comment.
I took that from Dan's code. It looks like going with using System.Timers.Timer; is ambiguous with System.Threading.Timer. I suppose that's why he did it this way.
Here's Dan's code btw ---
There was a problem hiding this comment.
In that case, it's fine how it is!
| { | ||
| currentCircuit = circuit; | ||
|
|
||
| return base.OnCircuitOpenedAsync(circuit, cancellationToken); |
There was a problem hiding this comment.
It's not necessary to call the base method here - it doesn't have a meaningful implementation and just returns Task.CompletedTask.
There was a problem hiding this comment.
If I don't return, I get an error that the OnCircuitOpenedAsync isn't returning a value. How is it set up without returning the base method?
There was a problem hiding this comment.
Make it just return a completed Task? ... rubber 🦆 ... I think I'm answering my own question! 😄
public override Task OnCircuitOpenedAsync(Circuit circuit,
CancellationToken cancellationToken)
{
currentCircuit = circuit;
return Task.CompletedTask;
}There was a problem hiding this comment.
Yep, returning Task.CompletedTask is the right move!
Fixes #32021
Thanks @gerektoolhy! 🚀 ... This will log the ID and fix a few other NITs.
@MackinnonBuck ... The updates seem reasonable 🤔, but I'll wait for you to look because I don't think you saw the original content when it went in. This will give you a chance to look over the entire section in case there are additional remarks to make (e.g., gotchas 😈).
Internal previews