🚧 Add dial timeout and cleanup ResolveEndpoint #278
Conversation
| } | ||
| if got, want := errStr, c.errStr; got != want { | ||
| t.Fatalf("got error %q want %q", got, want) | ||
| if got, want := errStr, c.errStr; got != want { |
There was a problem hiding this comment.
This handles both the nil and not-nil case without the if statement.
if got, want := err, tt.err; !reflect.DeepEqual(got, want) {
t.Fatalf("got error %v want %v", got, want)
}There was a problem hiding this comment.
I was getting a panic above on errStr = err.Error() since err was nil.
There was a problem hiding this comment.
yes, you need to refactor the test to have an error instead of an errStr in the test case definition.
|
I am not sure what the proper value should be for this:
|
|
Maybe by default we should not time out. |
There was a problem hiding this comment.
Looks good; the biggest comment is how we can expose the timeout to the user. This is especially needed if we remove the default timeout as @magiconair suggested.
| @@ -36,11 +38,18 @@ func nextid() uint32 { | |||
|
|
|||
| func Dial(ctx context.Context, endpoint string) (*Conn, error) { | |||
| debug.Printf("Connect to %s", endpoint) | |||
| network, raddr, err := ResolveEndpoint(endpoint) | |||
|
|
|||
| ctx, cancel := context.WithTimeout(ctx, DefaultDialTimeout) | |||
There was a problem hiding this comment.
How can users set a custom timeout? Maybe we pass the the timeout as a parameter? Since nobody should be importing uacp directly we could modify the Client to have a new Option for the timeout.
There was a problem hiding this comment.
Currently the only way would be to set uacp.DefaultDialTimeout first. What I'd rather have is to be able to use a context with timeout here: https://github.com/Intelecy/opcua/blob/net-ctx/client.go#L158-L189.
But that requires more refactoring as the incoming context is used for the monitoring routine.
There was a problem hiding this comment.
Right, but uacp.DefaultDialTimeout is const so it can't be configured at runtime. Changing it to be non-const means users would need to import uacp and modify a global variable to change the setting.
I agree that putting it in the client is the right spot which is why I think having uacp.Dial just accept a timeout parameter (or making a uacp.DialWithTimeout method and have Dial be a wrapper for it with the default timeout in place) could work. Then the client's Dial method just changes to:
conn, err := uacp.DialWithTimeout(ctx, c.endpointURL, c.cfg.DialTimeout) and your changes still work (replacing DefaultDialTimeout with the parameter)
Alternatively, it could be possible to create a second context in the the client's Dial method with a timeout as long as it's not used as the parent for the monitor routine's context and that shouldn't cause issues.
eg:
func (c *Client) Dial(ctx context.Context) error {
...
dialCtx, dialCancel := context.WithTimeout(ctx, c.cfg.DialTimeout)
defer dialCancel()
conn, err := uacp.Dial(dialCtx, c.endpointURL)
if err != nil {
return err
}
...
ctx, c.cancelMonitor = context.WithCancel(ctx)
go c.monitorChannel(ctx)
...While this doesn't seem as clear to me, it might make having no timeout simpler.
This PR adds a default timeout to the underlying
Dialcalls. This affects both the name resolution and the actual TCP setup. I also cleaned up the resolve logic. I left the logic in place that explicitly resolves a host name to a single IP address because I don't know the original reasons (I don't see any need for it. I would rather just create aValidateEndpointand let the regular dialer do its thing.)I originally tried to enable a timeout by adding a
context.Timeoutto the call toClient.Dial(https://github.com/Intelecy/opcua/blob/net-ctx/client.go#L158-L189), but that backfired as the incoming context is also used for signaling, and not just the dial itself (as the name would imply).TODO: more testing as I've only tested this in one local configuration