-
Notifications
You must be signed in to change notification settings - Fork 255
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.
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
🚧 Add dial timeout and cleanup ResolveEndpoint #278
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Lets test it a bit more
} | ||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was getting a panic above on errStr = err.Error()
since err
was nil
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, you need to refactor the test to have an error
instead of an errStr
in the test case definition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we now have errors.Equal
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Dial
calls. 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 aValidateEndpoint
and let the regular dialer do its thing.)I originally tried to enable a timeout by adding a
context.Timeout
to 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