-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Disposing Asynchronously #434
Conversation
# Conflicts: # source/Halibut.Tests/Transport/ConnectionManagerFixture.cs
# Conflicts: # source/Halibut.Tests/Transport/ConnectionPoolFixture.cs # source/Halibut/Transport/ConnectionManager.cs # source/Halibut/Transport/ConnectionPool.cs
# Conflicts: # source/Halibut.Tests/Support/TestConnection.cs # source/Halibut.Tests/Transport/ConnectionPoolFixture.cs # source/Halibut/HalibutRuntime.cs # source/Halibut/Transport/ConnectionManager.cs # source/Halibut/Transport/ConnectionManagerAsync.cs # source/Halibut/Transport/ConnectionPool.cs # source/Halibut/Transport/ConnectionPoolAsync.cs # source/Halibut/Transport/IConnectionManager.cs # source/Halibut/Transport/IConnectionPool.cs # source/Halibut/Transport/SecureClient.cs # source/Halibut/Transport/SecureListeningClient.cs # source/Halibut/Transport/SecureWebSocketClient.cs
This pull request has been linked to Shortcut Story #53211: 🎉 🎉 🎉 Make Halibut Support Async RPC Calls 🎉 🎉 🎉. |
# Conflicts: # source/Halibut/ServiceModel/PendingRequestQueueAsync.cs # source/Halibut/Transport/SecureConnection.cs
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.
Yeah looks good dude, nice one!
public abstract class AsyncDisposableStream : Stream | ||
#if NETFRAMEWORK | ||
, IAsyncDisposable | ||
#endif |
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.
Truly, truly awful 😆
It's good we're keeping this hidden
@@ -99,7 +119,7 @@ public async Task<IConnection> AcquireConnectionAsync(ExchangeProtocolBuilder ex | |||
Tuple<IConnection, Action> CreateNewConnection(ExchangeProtocolBuilder exchangeProtocolBuilder, IConnectionFactory connectionFactory, ServiceEndPoint serviceEndpoint, ILog log, CancellationToken cancellationToken) | |||
{ | |||
var lazyConnection = new Lazy<IConnection>(() => connectionFactory.EstablishNewConnection(exchangeProtocolBuilder, serviceEndpoint, log, cancellationToken)); | |||
var connection = new DisposableNotifierConnection(lazyConnection, OnConnectionDisposed); | |||
var connection = new DisposableNotifierConnection(lazyConnection, OnConnectionDisposed, OnConnectionDisposedAsync); |
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.
Note: This sync implementation is slated for removal for the context of other readers.
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.
LGTM 👍
[sc-53211]
Background
As part of making Halibut async, we need to ensure we dispose of things asynchronously as well.
Results
Related to OctopusDeploy/Issues#8266
Before
The following places were found to have paths that would require async disposal. These were being disposed synchronously.
After
The green classes in the spreadsheet are not disposed asynchronously.
We did not change
using
statements to `await using' statements in this PR. This touched over 20 files, so will be assessed and implemented in a different PR.To ensure .NET 4.8 code could compile and use
IAsyncDisposable
, we had to import theMicrosoft.Bcl.AsyncInterfaces
package.How to review this PR
Quality ✔️
Pre-requisites