Allow radius response to timeout

[ofirm93]

Enhancement: currently after RADIUS request is sent (via Client.Exchange()) the response to the requests blocks.
In order to allow the response to timeout, a ReadTimeout can be set on the connection.

[ghost]

This is already supported with context:

ctx, done := context.WithTimeout(context.Background(), time.Second)
defer done()
resp, err := client.Exchange(ctx, req, server.Addr)

[ofirm93]

I saw this option before I opened this issue.
Although it is possible solution, there are some problems with it:

First, the current default is insensible, as it doesn't set any timeout which means it will block forever. This is a real problem for protocol such as UDP where no acknowledgement is promised. This caused a bug in my program and it was pretty hard to trace it. I think that to make the library more user friendly and require less internal knowledge of the library internals it would be better to set reasonable timeout and let the user optimise it if needed.

Second, the context setting is for the whole function call, and the response specific timeout is more granular.

[ofirm93]

For compatibility, I set the default client timeout to 0 (no timeout) it should be changed to a reasonable value.

If there is some versioning scheme in use, the default should be changed in the next minor version (and probably mentioned in the changes of that version)

[ghost]

First, the current default is insensible, as it doesn't set any timeout which means it will block forever. This is a real problem for protocol such as UDP where no acknowledgement is promised. This caused a bug in my program and it was pretty hard to trace it. I think that to make the library more user friendly and require less internal knowledge of the library internals it would be better to set reasonable timeout and let the user optimise it if needed.

This is only because context.Background() was used, no? Would have adding a sensible timeout to the context passed to the function solved this issue for you?

Second, the context setting is for the whole function call, and the response specific timeout is more granular.

Is there a real-world scenario why such a granular setting would be required? (I would simply like to avoid making the API surface larger if it can be helped).

[ofirm93]

First, the current default is insensible, as it doesn't set any timeout which means it will block forever. This is a real problem for protocol such as UDP where no acknowledgement is promised. This caused a bug in my program and it was pretty hard to trace it. I think that to make the library more user friendly and require less internal knowledge of the library internals it would be better to set reasonable timeout and let the user optimise it if needed.

This is only because context.Background() was used, no? Would have adding a sensible timeout to the context passed to the function solved this issue for you?

I used a context without that timeout property. When I added timeout it solved the issue, yet that feature is not that well documented in the library. Digging into the code discovered it. I think that a less experienced developer will tackle this and wouldn't find how to unblock the function call.

Second, the context setting is for the whole function call, and the response specific timeout is more granular.

Is there a real-world scenario why such a granular setting would be required? (I would simply like to avoid making the API surface larger if it can be helped).

Right now I have reason to believe it is enough, yet I think that just like retry count the timeout of web request should be set to some default and be tuned in case it is required.

The real issue here is the missing default and, in my opition, it would be harder to enforce default value as context property. Following other libraries convention seems to me a good idea.

[ghost]

Same default infinite read timeout bit our project as well. My mistake to assume a sensible default read timeout to be set by DefaultClient.

Attempted to set one myself by looking for standard deadline functions like net.Conn provides but Client has none. The only option is to brute-force murder the underlying net.Conn by specifying a deadline on the context. Not nice.

This is only because context.Background() was used, no? Would have adding a sensible timeout to the context passed to the function solved this issue for you?

The client example in documentation also uses context.Background(). The presence of MaxPacketErrors and Retry would suggest Exchange() will eventually fail but this is not the case, making it very easy for users to fall into this trap. I'm not sure why a failure to read anything whatsoever isn't considered a packet error.