-
Notifications
You must be signed in to change notification settings - Fork 56
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
#85 Added a DialContext method to all sockets. #98
Conversation
Made Dial use the sockets default timeout (uses DialContext now). Addjusted some tests to use DialContext.
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.
thanks for the PR.
(and apologies for the belated "review")
I have a couple of comments, see below.
@@ -32,6 +35,9 @@ type Socket interface { | |||
// Dial connects a remote endpoint to the Socket. | |||
Dial(ep string) error | |||
|
|||
// Dial connects a remote endpoint to the Socket. | |||
DialContext(ctx context.Context, ep string) error |
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.
the doc string of the Dial
method should be updated to reflect the implied semantics.
the csocket
implementation (for integration w/ cgo-zmq4) should be updated as well.
alternatively, we could just merge the 2 methods together, documenting that the context.Context
would be wrapped with the socket's configured timeout.
I think I'd err on the latter.
the csocket
implementation would still need to be updated, though :)
) | ||
|
||
var ( | ||
errInvalidAddress = errors.New("zmq4: invalid address") | ||
|
||
ErrBadProperty = errors.New("zmq4: bad property") | ||
ErrBadProperty = errors.New("zmq4: bad property") | ||
ErrUnknownTransport = errors.New("zmq4: unknown transport") |
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.
instead of a value, I'd rather have it as a type, TransportError
or something, so we could provide the name of the unknown transport.
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.
Make it a feature request.
Not related to this PR at all.
ping? |
Fixes #85 Made Dial use the sockets default timeout (uses DialContext now).
Addjusted some tests to use DialContext.
Added TestNullHandshakeRRFail.
Added DialContext method to Socket interface and all sockets.
The timeout is now based on the context, simplified the waiting.