Skip to content
This repository was archived by the owner on Dec 18, 2018. It is now read-only.

Give Client a chance to receive Close Frame from Server #730

Merged
merged 6 commits into from
Aug 18, 2017

Conversation

BrennanConroy
Copy link
Member

Issue was HttpConnection.DisposeAsync() was being called and it would complete the Channel then call transport.StopAsync(), the Channel completion would trigger the transport send loop to exit and cancel the _transportCts which would cause _webSocket.ReceiveAsync() to exit so it can't receive the close frame. This would happen in parallel with us calling _webSocket.CloseOutputAsync.

Now we use a different CTS for the webSocket calls so we have a chance to send and receive a close frame before exiting.
#720

@@ -62,7 +62,7 @@ public WebSocketsTransportTests(ServerFixture<Startup> serverFixture, ITestOutpu
await webSocketsTransport.StartAsync(new Uri(_serverFixture.WebSocketsUrl + "/echo"), channelConnection,
TransferMode.Binary, connectionId: string.Empty);
connectionToTransport.Out.TryComplete();
await webSocketsTransport.Running.OrTimeout();
await webSocketsTransport.Running.OrTimeout(TimeSpan.FromSeconds(10));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a fan... can we add an option or something?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you need to extend this timeout?
Also, this test failed on AppVeyor so it does not seem increasing this timeout helped.

Copy link
Member Author

@BrennanConroy BrennanConroy Aug 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The known flaky SSE test failed in AppVeyor. And the timeout is because we wait 5 seconds in WebSockets before canceling the ReceiveAsync

@moozzyk
Copy link
Contributor

moozzyk commented Aug 16, 2017

You can still exit the loop in two places if _transportCts is cancelled and will not wait for the Close frame:

@@ -198,7 +208,7 @@ public WebSocketsTransport(ILoggerFactory loggerFactory)
finally
{
_logger.SendStopped(_connectionId);
_transportCts.Cancel();
TriggerCancel();
Copy link
Contributor

@moozzyk moozzyk Aug 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed? if you get here it means that receiveCts has been cancelled or there was an exception. I think we only need _transportCts.Cancel() here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scratch that - I did not expand and thought it was in the Receive loop.

}
}
catch (OperationCanceledException)
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth logging?

@@ -198,7 +208,7 @@ public WebSocketsTransport(ILoggerFactory loggerFactory)
finally
{
_logger.SendStopped(_connectionId);
_transportCts.Cancel();
TriggerCancel();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scratch that - I did not expand and thought it was in the Receive loop.

@moozzyk
Copy link
Contributor

moozzyk commented Aug 18, 2017

🐑🇮🇹

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants