Skip to content
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

Allowing ConnectionManager and ConnectionPool to be disposed of both synchronously and asynchronously #460

Merged
merged 4 commits into from
Aug 22, 2023

Conversation

sburmanoctopus
Copy link
Contributor

@sburmanoctopus sburmanoctopus commented Aug 22, 2023

[sc-53245]

Background

When we tried using the latest Halibut in Octopus Server, tests would time out.

Results

Related to OctopusDeploy/Issues#8266

Before

Octopus Server would dispose of the Halibut runtime via Autofac, which would sense that the IHalibutRuntime implemented IAsyncDisposable.

But this completely ignores whether the feature toggle is turned on.

As a result, it would try to dispose a 'synchronous' Halibut 'asynchronously'.

This would cause the ConnectionManager to be disposed of asynchronously, which throws an exception if that occurs.

After

We have now made both ConnectionManager and ConnectionPool be disposable asynchronously.

This solves the issue, but also, it is probably a good idea to allow both paths of disposal to be allowed.

While here, we also noticed that CreateNewConnection is never called (so it was deleted).

We also had to reinstate OSSupportsWebSockets, as this is used in Octopus Server.

How to review this PR

Quality ✔️

Pre-requisites

  • I have read How we use GitHub Issues for help deciding when and where it's appropriate to make an issue.
  • I have considered informing or consulting the right people, according to the ownership map.
  • I have considered appropriate testing for my change.

…synchronously and asynchronously, so that Octopus Server can dispose of this regardless of what the feature toggle is set to.
@shortcut-integration
Copy link

@sburmanoctopus sburmanoctopus changed the title Allowing ConnectionManager and ConnectionPool to be disposed of both … Allowing ConnectionManager and ConnectionPool to be disposed of both synchronously and asynchronously Aug 22, 2023
@sburmanoctopus sburmanoctopus marked this pull request as ready for review August 22, 2023 02:36
@sburmanoctopus sburmanoctopus requested a review from a team as a code owner August 22, 2023 02:36
Copy link
Contributor

@nathanwoctopusdeploy nathanwoctopusdeploy left a comment

Choose a reason for hiding this comment

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

LGTM

@sburmanoctopus sburmanoctopus merged commit 0d230ef into main Aug 22, 2023
11 of 14 checks passed
@sburmanoctopus sburmanoctopus deleted the sast/SupportDisposingAsyncAndSync branch August 22, 2023 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants