Add HubConnection.State #2204
Add HubConnection.State #2204
Conversation
| // TODO: Documentation | ||
| public enum HubConnectionStatus | ||
| { | ||
| Started, |
There was a problem hiding this comment.
Hmm, would connected make more sense here?
There was a problem hiding this comment.
Could do! I went with Started and Stopped because of StartAsync and StopAsync.
🚲 🏠 @davidfowl @anurse
There was a problem hiding this comment.
I like Started and Stopped as well, given the method names.
There was a problem hiding this comment.
Hmmm, the methods are Started and Stopped but they're on a class with connection in the name.
I think I prefer Disconnected and Connected.
| get | ||
| { | ||
| // Local copy for thread-safety | ||
| var connectionState = _connectionState; |
There was a problem hiding this comment.
Since ConnectionState is a class, wouldn't this still reference the original object?
There was a problem hiding this comment.
Yes but the field can be set to null by another thread in-between checking it is not null and getting IsStopped.
Copy is the wrong word. A better comment would be "Local reference for thread-safety".
There was a problem hiding this comment.
?. will do this for you (source)
It's mostly just a clever trick though, so I don't really mind either way. Was mostly just curious if if would do the Right Thing.
There was a problem hiding this comment.
I like taking a local variable for thread-safety issues because it gives a good opportunity to comment and make it obvious that it is a thing future readers of the code need to think about.
|
🆙 📅 |
|
@dotnet-bot test Windows Release x64 Build please |
|
Hey @davidfowl @anurse, is there any value in having a Connecting and Disconnecting state? |
|
@JamesNK Yeah, I guess that makes sense. Automatic Reconnect is also going to to throw a complexity in there (do we need a |
|
@davidfowl said two is fine. It's an enum so no reason more can't be added as needed. |
| @@ -215,6 +224,58 @@ Task<ConnectionContext> ConnectionFactory(TransferFormat format) | |||
| }); | |||
| } | |||
|
|
|||
| [Fact] | |||
| public async Task StatusIsNotStartedUntilStartAsyncIsFinished() | |||
There was a problem hiding this comment.
The title should be "StatusIsNotConnected..."
| } | ||
|
|
||
| [Fact] | ||
| public async Task StatusIsStoppedInCloseEvent() |
There was a problem hiding this comment.
And here replace Stopped with Disconnected
Fixes #2127