Fix connect()-related Appveyor/win test failures #140
Conversation
On windows, connect() call fails when using the return of get_last_endpoint() after a wild-card bind, "tcp://0.0.0.0:<PORT>". Instead, we must use the form "tcp://127.0.0.1:<PORT>". This behavior is seen in the libzmq library[1] itself. [1]: https://github.com/zeromq/libzmq/blob/905286261969d4e4b1b956f2befacb8c1eae5e35/tests/test_unbind_wildcard.cpp#L45-L49
| @@ -18,7 +29,7 @@ fn create_socketpair() -> (Socket, Socket) { | |||
| receiver.set_rcvtimeo(1000).unwrap(); | |||
|
|
|||
| receiver.bind("tcp://*:*").unwrap(); | |||
There was a problem hiding this comment.
Great! Would it be simpler to use tcp://127.0.0.1:xxxx here?
There was a problem hiding this comment.
Actually never mind. If the upstream tests do this, then let's just copy them.
There was a problem hiding this comment.
Yes, "127.0.0.1:*" would be just as effective/simpler; but once I dug into libzmq to verify the behavior was underlying and saw they had the same approach I figured it "made sense" here too, if only in that it provides some documentation of this behavior on windows. Although it may be worth adding such documentation to get_last_endpoint() itself explicitly?
There was a problem hiding this comment.
I'd rather avoid having platform-specificality in the test suite, if
it can be avoided. However, I'm not what the "proper" solution looks
like, or even if there is one. I'll have to look into this more
closely.
There was a problem hiding this comment.
Fundamentally, the issue is on windows one cannot connect() to the any address "0.0.0.0", as stated here:
If the address member of the structure specified by the name parameter is filled with zeros, connect will return the error WSAEADDRNOTAVAIL.
This is the address that is returned from get_last_endpoint() after binding the wildcard address "tcp://*:*".
If you'd like to avoid this in the test, then simply switching the bind to "127.0.0.1:*" works, and is arguably an equally effective test.
I don't think there is an effective way to bubble the "fix" into the library itself (i.e. into get_last_endpoint()); doing the replace fix done in this patch inside the actual library seems like a potentially unpleasant unexpected/hidden behavior...
There was a problem hiding this comment.
See my comment in #141; it seems the issue is not with get_last_endpoint(), which behaves identically (and sanely, IMHO) on both platforms, IIUC. The question is rather if a connect() to INADDR_ANY really is well-defined even across POSIX-like systems.
What about not using the wildcard address in the tests at all? I see
only downsides of binding any sockets in the test suite to the wildcard address:
- It allocates resources more broadly than needed on the machine running the tests.
- It has security implications (also binding to the loopback has security implications, see e.g. CVE-2016-8606. Of course the attack surface is much smaller here ;-).
- It makes us deal with platform-specifica -- namely how to find out the peer's IP address for no good reason.
Even when the libzmq test suite does it with #ifdef and wildcard addresses, we don't have to emulate that ;-p.
Anyway, this is a rather minor point, the important thing is you found the underlying cause of (some of?) our Windows test suite failures, @pfernie!.
There was a problem hiding this comment.
I think that makes sense; the "wildcard" functionality is not an implementation detail of rust-zmq, but rather libzmq itself, so contorting the rust-zmq tests around undocumented/defined behavior is probably unneeded.
The added documentation to get_last_endpoint() for windows users as in #141 might still be nice for users, however.
I've submitted PR 142 which simplifies the tests again and just binds 127.0.0.1:* instead.
|
Thanks @pfernie! |
|
@erickt Shouldn't this have gone on the |
On windows, connect() call fails when using the
return of get_last_endpoint() after a wild-card
bind, "tcp://0.0.0.0:".
Instead, we must use the form
"tcp://127.0.0.1:". This behavior is seen
in the libzmq library[1] itself.
[1]: https://github.com/zeromq/libzmq/blob/905286261969d4e4b1b956f2befacb8c1eae5e35/tests/test_unbind_wildcard.cpp#L45-L49