Expose ConnectionId on .NET Client #8668
Conversation
|
Looks like I need to do the |
src/SignalR/clients/csharp/Client/test/FunctionalTests/HubConnectionTests.cs
Outdated
Show resolved
Hide resolved
| @@ -405,6 +420,8 @@ private async Task StopAsyncCore(bool disposing) | |||
| (_serviceProvider as IDisposable)?.Dispose(); | |||
| _disposed = true; | |||
| } | |||
|
|
|||
| _connectionId = null; | |||
There was a problem hiding this comment.
Not sure if we care about this, lets see what other people think
There was a problem hiding this comment.
I like nulling it out once the connection is stopped.
There was a problem hiding this comment.
Stopped. The ConnectionId isn't that useful after you've disconnected, and it can take on multiple values prior to being disposed.
| @@ -116,6 +117,14 @@ public partial class HubConnection | |||
| /// </summary> | |||
| public TimeSpan HandshakeTimeout { get; set; } = DefaultHandshakeTimeout; | |||
|
|
|||
| /// <summary> | |||
| /// Get the connections current connection Id. | |||
There was a problem hiding this comment.
Gets the connection's current Id. reads better IMO
There was a problem hiding this comment.
Will ConnectionId be null in some cases or has that changed?
A null connection ID for an active connection will confuse some people. It should be mentioned in the comments.
There was a problem hiding this comment.
I agree we should document when this can be null. Is this only when the client is disconnected? Or is it also possible to establish a connection without negotiating using HubConnection?
There was a problem hiding this comment.
I believe a HubConnection with the transport explicitly specified as WebSockets will skip negotiation and never have the connection ID sent to the client.
There was a problem hiding this comment.
Yeah, when negotiation is skipped the client connection Id will be null as well as when the client is disconnected. I can file a docs issue so that we follow up once we get this change in across the clients.
There was a problem hiding this comment.
Whether or not we decide this is worth mentioning in a doc article, I think we should also mention this in the doc comments. Something similar to what you have for the Java client should be fine. E.g.
/// Gets the connections current Id. This value will be cleared when the connection is stopped and will have a new value every time the connection is (re)established.
/// This value will be null if the WebSockets transport is explicitly specified because the client skips negotiation in that case.
There was a problem hiding this comment.
I like that wording. We should also note that this will always be null when the SkipNegotiation flag is set to true.
There was a problem hiding this comment.
Good coverage w.r.t. docs. thanks!
| @@ -116,6 +117,13 @@ public partial class HubConnection | |||
| /// </summary> | |||
| public TimeSpan HandshakeTimeout { get; set; } = DefaultHandshakeTimeout; | |||
|
|
|||
| /// <summary> | |||
| /// Gets the connection's current Id. This value will be cleared when the connection is stopped and will have a new value every time the connection is (re)established. | |||
| /// This value will be null if the negotiation step is skipped via HttpConnectionOptions or if the WebSockets transport is explicitly specified because the | |||
There was a problem hiding this comment.
This is incorrect. Negotiate is only skipped when Websockets AND SkipNegotiate are set.
There was a problem hiding this comment.
I think SkipNegotiate is only possible when Websockets is set. Could simplify the comment to just meantion it.
Exposing the ConnectionId on the .NET Client.
Part of: #8681