Limit TimeSpan timeouts to Int32 MaxValue

[jscarle]

Added guard clauses to various timeouts to ensure they don't exceed an Int32 in milliseconds.

Resolves #14.

[jscarle]

@WojciechNagorski @Rob-Hague Ready for review and merge.

[Rob-Hague]

I'm not seeing what problem this solves. If an underlying API doesn't accept a certain range of TimeSpan, aren't we going to get an error either way? Isn't it better to let the API decide?

[jscarle]

I'm not seeing what problem this solves. If an underlying API doesn't accept a certain range of TimeSpan, aren't we going to get an error either way? Isn't it better to let the API decide?

I'm not sure what you mean by "the API".

But basically, the issue boils down to the fact that the timeouts are injested by SSH.NET's public methods as TimeSpans but then used internally for timeouts that int based. The goal is to bring validation to the outer edge of the public methods of SSH.NET to avoid OutOfRange exceptions that could come from the internals of SSH.NET.

[WojciechNagorski]

How to reproduce the error or how does it manifest itself? I don't know what's improved here either?

Can you add a test that didn't work before but now works?

[jscarle]

@Rob-Hague @WojciechNagorski The latest version that's in develop gives me a ton of IDE0005 build errors:

[jscarle]

How to reproduce the error or how does it manifest itself? I don't know what's improved here either?

Can you add a test that didn't work before but now works?

I'll take care of it.

[Rob-Hague]

I'm not sure what you mean by "the API".

I meant, if we're forwarding the TimeSpan to a method on e.g. Socket or Monitor, then those methods will perform their own validation and throw if they don't accept a certain value, so there should be no need to try and presume what values are acceptable or not.

I guess I felt like I was missing some context or motivation for this change. But I think it's OK in principal

The latest version that's in develop gives me a ton of IDE0005 build errors

That sounds like VS weirdness. It's OK for me (although sometimes it feels like the IDE doesn't know what to do with so many analyzers)

[jscarle]

I'm not sure what you mean by "the API".

I meant, if we're forwarding the TimeSpan to a method on e.g. Socket or Monitor, then those methods will perform their own validation and throw if they don't accept a certain value, so there should be no need to try and presume what values are acceptable or not.

I guess I felt like I was missing some context or motivation for this change. But I think it's OK in principal

Generally speaking, it is considered good design that libraries validate and throw early in public API methods. As an example of this, you can take analyzer rule CA1062 for null checks. The original reason I looked into this was because of #14, which although at the surface may seem superfluous, is actually valid.

[jscarle]

The latest version that's in develop gives me a ton of IDE0005 build errors

That sounds like VS weirdness. It's OK for me (although sometimes it feels like the IDE doesn't know what to do with so many analyzers)

I'm using Rider and dotnet build with SDK 8.0.200. So I don't think this has anything to do with VS.

[jscarle]

The latest version that's in develop gives me a ton of IDE0005 build errors

That sounds like VS weirdness. It's OK for me (although sometimes it feels like the IDE doesn't know what to do with so many analyzers)

I'm using Rider and dotnet build with SDK 8.0.200. So I don't think this has anything to do with VS.

I added IDE0005 to the .editorconfig.

[jscarle]

@WojciechNagorski @Rob-Hague Ready for final review and merge.

[jscarle]

@WojciechNagorski I fixed the reference you mentioned. I must have been really tired, that slipped right past me.

[WojciechNagorski]

Can you revert this? I think after restarting VS (or restarting your computer) everything should work.

This is the last message and then I will be able to merge this PR.

[Rob-Hague]

I am seeing this locally as well, the errors look real. I think it could be related to upgrading the SDK but I have no idea why we haven't seen it before.

I am trying to fix it with dotnet format but not having much luck :-(

[WojciechNagorski]

I was wrong, I don't know what's going on.

[WojciechNagorski]

LGTM!