OctopusDeploy / Halibut

| Public | A secure communication stack for .NET using JSON-RPC over SSL.
Other
12 stars 44 forks source link

Making WebSocketStream support async disposal #475

Closed sburmanoctopus closed 1 year ago

sburmanoctopus commented 1 year ago

[sc-57814]

Background

While making Halibut async, we found that a WebSocketStream does not support asynchronous disposal.

Results

Related to OctopusDeploy/Issues#8266

Before

WebSocketStream disposed synchronously, resulting in a call to the asynchronous context.CloseOutputAsync with a blocking .GetAwaiter().GetResult() at the end...

After

WebSocketStream now supports async disposal, and therefore calls context.CloseOutputAsync asynchronously.

As a result, when SecureConnection wraps a WebSocketStream, it can dispose of it asynchronously (which it already did, but would result in sync disposal before).

Using SecureConnection, the SecureWebSocketClient can now dispose of of the connection asynchronously (on async paths).

Also, SecureWebSocketListener can now dispose of the WebSocketStream asynchronously also.

How to review this PR

SecureWebSocketListener doesn't have a "sync vs async" path. So making it dispose WebSocketStream async is a different behaviour to the synchronous version.

I think it will be fine, but thought it worth raising in case we wish to split that out into a separate story.

Quality :heavy_check_mark:

Pre-requisites

shortcut-integration[bot] commented 1 year ago

This pull request has been linked to Shortcut Story #57814: SecureWebSocketListener calls sync dispose on WebSocketStream which also doesn't implement AsyncDipose.