OctopusDeploy / Halibut

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

Stop throwing an exception in the finally of WithTimeouts #563

Closed nathanwoctopusdeploy closed 9 months ago

nathanwoctopusdeploy commented 10 months ago

Background

Stop throwing an exception in the finally of WithTimeouts when the Stream is in an error state.

[sc-65700]

This has been run through a Tentacle build here https://github.com/OctopusDeploy/OctopusTentacle/pull/711

How to review this PR

Quality :heavy_check_mark:

Pre-requisites

shortcut-integration[bot] commented 10 months ago

This pull request has been linked to Shortcut Story #65700: NetworkTimeoutStream throws when setting timeouts on a closed stream, causing the withtimeout helpers to throw a new timeout exception when the stream was closed..

nathanwoctopusdeploy commented 10 months ago

I'd like to understand the question on timeoutsReverted and why we want to throw an exception first before approving.

There may be a better approach to this. Happy to pair and revisit @sburmanoctopus . The general idea was don't set the timeouts when we can't as we are in error but don't hide errors and allow this to be an area that could cause bugs.

I considered changing the NetworkTimeoutStream but I didn't feel this was safe to do. I think the logic we are using to change the timeout is the correct place, e.g. the caller that causes the error should be responsible for handling it.

We could make it more advanced and determine when / when we cannot change timeouts based on the state of a stream. I went for try/catch/ensure we are not hiding real issues.