search

sshnet / SSH.NET

SSH.NET is a Secure Shell (SSH) library for .NET, optimized for parallelism.
http://sshnet.github.io/SSH.NET/
MIT License
3.88k stars 917 forks source link

Limit TimeSpan timeouts to Int32 MaxValue #1321

Closed jscarle closed 4 months ago

jscarle commented 4 months ago

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

Resolves #14.

jscarle commented 4 months ago

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

jscarle commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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

image

jscarle commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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

jscarle commented 4 months ago

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