-
Notifications
You must be signed in to change notification settings - Fork 448
Making HttpConnection restartable (C#) #1147
Conversation
|
||
private int _nextId = 0; | ||
private volatile bool _startCalled; | ||
|
||
public Task Closed { get; } | ||
public event Action<Exception> Closed; |
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.
Shouldn't this be a wrapper around _connection.Closed
?
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.
No. This way the only event that is hooked to HttpConnection is the one from hubConnection and the user subscribes to the object they are using.
{ | ||
return Task.FromException( | ||
new InvalidOperationException("Cannot start a connection that is not in the Initial state.")); | ||
new InvalidOperationException("Cannot start a connection that is not in the Disconnected state.")); |
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.
nameof(ConnectionState.Disconnected)
!
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.
Fixed. Initially it was not possible because state was tracked as int not an enum.
_logger.StoppingClient(_connectionId); | ||
|
||
if (Interlocked.Exchange(ref _connectionState, ConnectionState.Disconnected) == ConnectionState.Initial) | ||
lock(_stateChangeLock) |
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.
space
private class ConnectionState | ||
private ConnectionState ChangeState(ConnectionState from, ConnectionState to) | ||
{ | ||
lock(_stateChangeLock) |
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.
space
Task DisposeAsync(); | ||
|
||
IDisposable OnReceived(Func<byte[], object, Task> callback, object state); | ||
|
||
Task Closed { get; } | ||
event Action<Exception> Closed; |
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.
Used to be Func<Exception, Task>
, any reason we don't go back to that?
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.
I am not sure we want it (and what we had was not implemented correctly). I think we need to try which pattern works better for restarting the connection from this event.
var startTask = connection.StartAsync(); | ||
await allowStopTcs.Task; | ||
|
||
await Task.WhenAll(startTask.OrTimeout(), connection.StopAsync().OrTimeout()); |
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.
Just OrTimeout
the WhenAll
call
} | ||
|
||
[Fact] | ||
public async Task CanStartConnectionStoppedWithError() |
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.
CanStartConnectionAfterStoppedWithError
?
await connection.SendAsync(new byte[] { 0x42 }).OrTimeout(); | ||
} | ||
catch { } | ||
await closeTcs.Task.OrTimeout().OrTimeout(); |
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.
Dupe OrTimeout
[Fact] | ||
public async Task CanStopStoppedConnection() | ||
{ | ||
var mockHttpHandler = new Mock<HttpMessageHandler>(); |
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.
unused
@@ -134,16 +138,16 @@ private byte[] FormatMessageToArray(byte[] message) | |||
} | |||
} | |||
} | |||
_closeTcs.TrySetResult(null); | |||
Closed(null); |
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.
Closed?.Invoke(...)
018d79f
to
ecab6f6
Compare
@@ -364,6 +365,8 @@ private void Shutdown(Exception ex = null) | |||
} | |||
_pendingCalls.Clear(); | |||
} | |||
|
|||
Closed?.Invoke(ex); |
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.
try catch, don't trust user code
if (t.IsCanceled) | ||
{ | ||
_closedTcs.TrySetCanceled(); | ||
Closed?.Invoke(t.Exception.InnerException); |
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.
Can't trust user code, should wrap this
337785a
to
ce39b47
Compare
This needs a bunch of rebasing and adjusting after merging #1165 . I'll do that and ping this when I'm done. |
Ok, it should be 🆙 to 📅 now. I had to refactor some things to handle the new AbortAsync. Also, some tests seem to have been broken by the fact that Dispose waits on Start to finish, but I fixed that. Finally, there was a test for Received not firing after Dispose is called which isn't really possible since the state changes that trigger that all occur after the channel is marked completed anyway. The test was broken so I just removed it (because it is no longer relevant). |
@@ -355,46 +592,63 @@ public void CannotStartConnectionWithInvalidTransportType(TransportType requeste | |||
[Fact] | |||
public async Task EventQueueTimeout() |
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.
Turns out this test wasn't actually working and it was hiding an issue in the EventQueue drain logic :)
} | ||
finally | ||
{ | ||
await connection.DisposeAsync(); | ||
} | ||
} | ||
|
||
[Fact] | ||
public async Task ReceivedCallbackNotRaisedAfterConnectionIsDisposed() |
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.
This is the test that I mentioned in other comments. It is not really relevant any more as the channel is closed before the state changes to make this event no-op occur.
Should be green after this next build. ping @BrennanConroy @mikaelm12 This one is unfortunately a bit of a tricky one, and I'm thinking I might do some test refactoring after this PR to make them clearer as they're a bit messy now (due to how tests tend to grow organically) |
~~(a.k.a. Opening Pandora box)~~
e9bc9eb
to
d51f576
Compare
@@ -306,7 +320,11 @@ private static Uri CreateConnectUrl(Uri url, NegotiationResponse negotiationResp | |||
await _transport.StartAsync(connectUrl, applicationSide, GetTransferMode(), _connectionId, this); | |||
|
|||
// actual transfer mode can differ from the one that was requested so set it on the feature | |||
Debug.Assert(_transport.Mode.HasValue, "transfer mode not set after transport started"); | |||
if(!_transport.Mode.HasValue) |
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.
space
// first AbortAsync call is still set at the time CloseAsync gets to calling the Closed event. However, this can't happen because the | ||
// StartAsync method can't be called until the connection state is changed to Disconnected, which happens AFTER the close code | ||
// captures and resets _abortException. | ||
public async Task AbortAsync(Exception ex) => await StopAsyncCore(ex ?? throw new ArgumentNullException(nameof(ex))).ForceAsync(); |
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.
super nit: public API we should name the argument exception
@@ -0,0 +1,272 @@ | |||
using System; |
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.
CAPYRIT
await connection.StartAsync().OrTimeout(); | ||
|
||
// Stop normally | ||
var stopTask = connection.StopAsync(); |
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.
OrTimeout
try | ||
{ | ||
await connection.StartAsync(); | ||
return connection; | ||
} |
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.
Don't you need to return here? Otherwise this method will never return... or am I missing something?
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.
The return value was changed from Task<HubConnection>
to Task
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.
Yes indeed, but that still means you need a return statement right? Just without the connection...
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.
Correct, unless you're using async await
https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/await
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.
As far as I understand it, the await keyword will just wait for the execution of StartAsync() to finish before continuing with the method. It will not return from the method. As this is inside a while(true) loop, I think it will never return.
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.
What @fretje means is that this is an infinite loop:
while (true)
{
try
{
await connection.StartAsync();
}
catch (Exception)
{
}
}
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.
@acjh Thanks! Yes you guys are right, this is broken. Would either of you like to make a PR to fix it?
(a.k.a. Opening Pandora box)