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

Co-operatively cancel Tentacle Client Operations #730

Merged
merged 1 commit into from
Dec 11, 2023

Conversation

nathanwoctopusdeploy
Copy link
Contributor

This is the reverted PR #647 rebased

Background

[sc-58429]

Related to: OctopusDeploy/Halibut#525

Before Async Halibut was introduced, an RPC the was transferring (reading or writing over a TCP Connection) could not be cancelled in Halibut, only an RPC that was connecting could be cancelled.

Async Halibut, along with OctopusDeploy/Halibut#525 has added the ability to cancel RPC whether connecting or transferring. This allows the caller of Halibut to cancel an RPC with a cancellation token co-operatively.

Before Async Halibut, abandoning a Transferring RPC was the only way to "cancel" it. That is, run it as a different Task and walk away from the running Task if a cancellation was performed. While this method would allow the caller to regain control flow, it left the Task still executing and left IO operations on TCP Connection from Tentacle Client to Tentacle active.

Results

Before

Tentacle Client only supported the cancellation of connecting RPC. If an RPC was transferred, cancellation could not be performed, and the caller would have to wait for the RPC to complete.

Tentacle Client used the abandon technique for RPC Retry Timeouts, as it was the only reliable way to timeout the Retries and return control flow to the caller.

After

Tentacle Client will co-operatively cancel RPC to the Socket, stopping active reads and writes over TCP Connections.

Tentacle Client will co-operatively cancel RPC to the Socket when RPC Retries Timeout.

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.

@nathanwoctopusdeploy nathanwoctopusdeploy changed the base branch from main to sast/test-exception-contracts December 5, 2023 11:35
@nathanwoctopusdeploy nathanwoctopusdeploy force-pushed the sast/test-exception-contracts branch from 6b77e55 to e138114 Compare December 6, 2023 03:16
@nathanwoctopusdeploy nathanwoctopusdeploy force-pushed the sast/co-operativly-cancel-rpc-retries branch from 61593ed to 1d27a7b Compare December 6, 2023 03:52
@nathanwoctopusdeploy nathanwoctopusdeploy force-pushed the sast/test-exception-contracts branch from e138114 to e9dd8f0 Compare December 6, 2023 04:01
@nathanwoctopusdeploy nathanwoctopusdeploy force-pushed the sast/co-operativly-cancel-rpc-retries branch from 1d27a7b to f3a499b Compare December 6, 2023 04:03
@nathanwoctopusdeploy nathanwoctopusdeploy force-pushed the sast/test-exception-contracts branch 6 times, most recently from ea8c002 to 9b19f36 Compare December 6, 2023 06:49
@nathanwoctopusdeploy nathanwoctopusdeploy force-pushed the sast/co-operativly-cancel-rpc-retries branch from 501bda6 to a2e09aa Compare December 6, 2023 07:19
Base automatically changed from sast/test-exception-contracts to main December 6, 2023 08:52
return false;
}

// TODO: Make this type safe
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have tests that will catch if this stops working but it needs to be improved. This will be done in a future PR

return false;
}

// TODO: Make this type safe
Copy link
Contributor Author

@nathanwoctopusdeploy nathanwoctopusdeploy Dec 7, 2023

Choose a reason for hiding this comment

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

We have tests that will catch if this stops working but it needs to be improved. This will be done in a future PR

@nathanwoctopusdeploy nathanwoctopusdeploy force-pushed the sast/co-operativly-cancel-rpc-retries branch 2 times, most recently from 3138dea to acf9581 Compare December 7, 2023 06:31
@nathanwoctopusdeploy nathanwoctopusdeploy marked this pull request as ready for review December 7, 2023 06:46
@nathanwoctopusdeploy nathanwoctopusdeploy requested a review from a team as a code owner December 7, 2023 06:46
@@ -28,7 +28,7 @@
</Otherwise>
</Choose>
<ItemGroup>
<PackageReference Include="Halibut" Version="7.0.368" />
<PackageReference Include="Halibut" Version="7.0.393-pull-576" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO Bump when OctopusDeploy/Halibut#576 merges

@nathanwoctopusdeploy nathanwoctopusdeploy force-pushed the sast/co-operativly-cancel-rpc-retries branch from acf9581 to e41c315 Compare December 7, 2023 06:56
Copy link
Contributor

@sburmanoctopus sburmanoctopus left a comment

Choose a reason for hiding this comment

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

LGTM. Some small changes to the original PR, but they look legit 😁

@nathanwoctopusdeploy nathanwoctopusdeploy force-pushed the sast/co-operativly-cancel-rpc-retries branch from e41c315 to cc3a220 Compare December 11, 2023 01:35
@nathanwoctopusdeploy nathanwoctopusdeploy merged commit dfa3c4b into main Dec 11, 2023
2 checks passed
@nathanwoctopusdeploy nathanwoctopusdeploy deleted the sast/co-operativly-cancel-rpc-retries branch December 11, 2023 02:54
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.

2 participants