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.

Sign up for GitHub

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

Stop throwing an exception in the finally of WithTimeouts #563

Merged
merged 1 commit into from
Dec 4, 2023
Merged

Stop throwing an exception in the finally of WithTimeouts #563

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 66 additions & 8 deletions source/Halibut/Transport/Protocol/StreamTimeoutExtensionMethods.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ public static async Task WithTimeout(this Stream stream, SendReceiveTimeout? tim

var currentReadTimeout = stream.ReadTimeout;
var currentWriteTimeout = stream.WriteTimeout;
var timeoutsReverted = false;

try
{
Expand All @@ -26,8 +27,20 @@ public static async Task WithTimeout(this Stream stream, SendReceiveTimeout? tim
}
finally
{
stream.ReadTimeout = currentReadTimeout;
stream.WriteTimeout = currentWriteTimeout;
try
{
stream.ReadTimeout = currentReadTimeout;
Copy link
Contributor

@sburmanoctopus sburmanoctopus Dec 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

🤔 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).

Copy link
Contributor Author

@nathanwoctopusdeploy nathanwoctopusdeploy Dec 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

stream.WriteTimeout = currentWriteTimeout;
timeoutsReverted = true;
}
catch
{
}
}

if (!timeoutsReverted)
Copy link
Contributor

Choose a reason for hiding this comment

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

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@nathanwoctopusdeploy nathanwoctopusdeploy Dec 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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 🙂

{
throw new InvalidOperationException("Could not revert the Timeouts. This should not happen.");
}
}

Expand All @@ -40,17 +53,33 @@ public static async Task<T> WithTimeout<T>(this Stream stream, SendReceiveTimeou

var currentReadTimeout = stream.ReadTimeout;
var currentWriteTimeout = stream.WriteTimeout;
var timeoutsReverted = false;
T result;

try
{
stream.SetReadAndWriteTimeouts(timeout);
return await func();
result = await func();
}
finally
{
stream.ReadTimeout = currentReadTimeout;
stream.WriteTimeout = currentWriteTimeout;
try
{
stream.ReadTimeout = currentReadTimeout;
stream.WriteTimeout = currentWriteTimeout;
timeoutsReverted = true;
}
catch
{
}
}

if (!timeoutsReverted)
{
throw new InvalidOperationException("Could not revert the Timeouts. This should not happen.");
}

return result;
}

public static async Task WithReadTimeout(this Stream stream, TimeSpan timeout, Func<Task> func)
Expand All @@ -63,6 +92,7 @@ public static async Task WithReadTimeout(this Stream stream, TimeSpan timeout, F
}

var currentReadTimeout = stream.ReadTimeout;
var timeoutsReverted = false;

try
{
Expand All @@ -71,7 +101,19 @@ public static async Task WithReadTimeout(this Stream stream, TimeSpan timeout, F
}
finally
{
stream.ReadTimeout = currentReadTimeout;
try
{
stream.ReadTimeout = currentReadTimeout;
timeoutsReverted = true;
}
catch
{
}
}

if (!timeoutsReverted)
{
throw new InvalidOperationException("Could not revert the Timeouts. This should not happen.");
}
}

Expand All @@ -83,16 +125,32 @@ public static async Task<T> WithReadTimeout<T>(this Stream stream, TimeSpan time
}

var currentReadTimeout = stream.ReadTimeout;
var timeoutsReverted = false;
T result;

try
{
stream.SetReadTimeouts(timeout);
return await func();
result = await func();
}
finally
{
stream.ReadTimeout = currentReadTimeout;
try
{
stream.ReadTimeout = currentReadTimeout;
timeoutsReverted = true;
}
catch
{
}
}

if (!timeoutsReverted)
{
throw new InvalidOperationException("Could not revert the Timeouts. This should not happen.");
}

return result;
}

public static void SetReadAndWriteTimeouts(this Stream stream, SendReceiveTimeout? timeout)
Expand Down