-
Notifications
You must be signed in to change notification settings - Fork 10.3k
[SignalR] Seamless Reconnect #48338
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SignalR] Seamless Reconnect #48338
Changes from all commits
40a2313
a20570a
f69e607
73fe26e
6371689
e57793c
0fb6bda
2bde741
638ecc0
510eef1
3eee427
ba54c5d
32ba07c
6c7de6c
512aee6
83647d9
e398721
ad31426
39cdb7e
ef40a04
3f83ef1
3da2c76
680bb23
209885f
610b8c8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System; | ||
using System.Collections.Generic; | ||
using System.Linq; | ||
using System.Text; | ||
using System.Threading.Tasks; | ||
|
||
namespace Microsoft.AspNetCore.Connections.Abstractions; | ||
|
||
/// <summary> | ||
/// | ||
/// </summary> | ||
public interface IReconnectFeature | ||
{ | ||
/// <summary> | ||
/// | ||
/// </summary> | ||
public Action NotifyOnReconnect { get; set; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there any useful context that would be meaningful on a per-invoke basis? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just double-checking: should this be an event rather than a property? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't like how there's a race where you don't know if you're writing to the original or new connection since NotifyOnReconnect fires at some arbitrary point after ConnectionContext.Transport has been swapped to the new connection. The write my throw sometimes, but it also might not. With the way we use it, that's okay because we can ignore anything prior to the sequence message on the reading side, but it feels like an unnecessary weakness in the design of the feature. Can we move the reconnect logic to be more above the transport layer? I'm thinking a new ConnectionContext with a new connection ID. This would still require a new feature to correlate the new connection with the old connection ID based on the token on the server, and we'd still want to make it seamlessly use the old connection ID from the perspective code using Hub APIs. But since there's no seamless deduping at the transport layer, I think it's best to avoid any magical potentially leaky abstractions there. I know this would be a major redesign, so I'm okay with shipping this as a WebSocket transport feature at first. But if we can relayer this, we could probably avoid a bunch of transport-specific logic.
In SignalR and most of the rest of ASP.NET Core we prefer plain old Funcs and Actions over events. |
||
|
||
// TODO | ||
// void DisableReconnect(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couldn't see one linked - has this gone though API review?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also: intellisense