Stop throwing an exception in the finally of WithTimeouts #563
Conversation
…am is in an error state and potentially hide / rethrow the original error causing confusion about the source of the error.
| stream.WriteTimeout = currentWriteTimeout; | ||
| try | ||
| { | ||
| stream.ReadTimeout = currentReadTimeout; |
There was a problem hiding this comment.
🤔 Just wondering, do other native .NET streams throw an exception when adjusting the Read/Write timeouts?
If not, then I wonder if an alternative would be to instead change the NetworkTimeoutStream to not throw an exception when changing ReadTimeout or WriteTimeout.
The current solution will be more generally safe, as it guarantees any read/write timeout alteration won't get in the way. But I couldn't help but wonder if we were guarding against something that shouldn't happen (and we got read/write timeout properties in NetworkTimeoutStream wrong instead).
There was a problem hiding this comment.
If not, then I wonder if an alternative would be to instead change the NetworkTimeoutStream to not throw an exception when changing ReadTimeout or WriteTimeout.
It's up to the underlying stream and if it's a network stream, possibly the OS (maybe, I don't really know ;) )? I think they do throw if the stream is not usable, and you throw an unrelated, side effect error, hence why we wrapped it and rethrow the real error.
There was a problem hiding this comment.
If NetworkTimeoutStream doesn't throw, then do we risk getting into a situation where we swallow real errors and end up not correctly changing timeouts, but we think we have.
| } | ||
| } | ||
|
|
||
| if (!timeoutsReverted) |
There was a problem hiding this comment.
Is timeoutsReverted what we want?
If await func() succeeds, but Read/Write timeout throw an exception, then timeoutsReverted will be false, and we will throw an InvalidOperationException here. But I thought the point of this PR was to stop throwing an exception in the finally. I.e., aren't we still throwing an exception, just a different one?
I'm likely not understanding something...
There was a problem hiding this comment.
This should not happen.
This was a fail safe to make sure it is working as I expected e.g. we throw so it doesn't matter that the timeouts were not reverted.
I didn't want to swallow errors setting the timeouts when it was a real issue, e.g. the actual call has not thrown, but for some reason the set timeouts throws. That should never happen.
There was a problem hiding this comment.
If await func() succeeds, but Read/Write timeout throw an exception
Should this happen? Is it valid? Will it leave us in a weird state if it did happen
There was a problem hiding this comment.
Ah, I see. The subtilty is that if we set the read/write timeout, and it throws a timeout exception, then this only really happens because it would have already thrown an exception with await func().
Yep, that makes sense 🙂
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. |
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 OctopusDeploy/OctopusTentacle#711
How to review this PR
Quality ✔️
Pre-requisites