Skip to content
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

Bugfix/dont include winsock2 #3681

Merged
merged 2 commits into from
Sep 13, 2019
Merged

Bugfix/dont include winsock2 #3681

merged 2 commits into from
Sep 13, 2019

Conversation

TobiSchluter
Copy link

@TobiSchluter TobiSchluter commented Sep 13, 2019

Including winsock2.h in zmq.h leads to ordering issues because different versions of headers or inclusions with intermediate defines can lead to collisions. In particular, inclusion of winsock2.h after windows.h can be troublesome. Since windows.h is a system header whereas zmq.h is a library header, the common pattern where system headers are included before additional library headers is thus broken.

This patch fixes it: the inclusion of winsock2.h in zmq.h is only needed for the definition of SOCKET, which is a pointer-wide integer, i.e. unsigned int (on 32bit) or unsigned __uint64 (on 64bit). Replace SOCKET with this, remove inclusion.

Tested by running ctests on 32bit and 64bit builds on Visual Studio 2019.

Solution: don't include winsock2.h, replace its only use (reference to SOCKET) by explicitly naming underlying type.
@TobiSchluter
Copy link
Author

TobiSchluter commented Sep 13, 2019

I see test_monitor timing out on a few of the windows builds. I could reproduce this failure locally on a busy system, but not on an idle system. I therefore assume the failure is a fluke.

I also see a number of other failures but these seem to be common across all builds of current revisions.

@sigiesec
Copy link
Member

I see test_monitor timing out on a few of the windows builds. I could reproduce this failure locally on a busy system, but not on an idle system. I therefore assume the failure is a fluke.

I also see a number of other failures but these seem to be common across all builds of current revisions.

Yes, this can't be related to your change. Getting rid of the include is a good thing. Unfortunately, the windows SDK headers are really messy :(

@sigiesec sigiesec merged commit d766640 into zeromq:master Sep 13, 2019
@TobiSchluter
Copy link
Author

Thanks!

@TobiSchluter
Copy link
Author

I don't mean to be pushy, but I guess after half a year it's fine to ask: when will there be a release that incorporates this change? Is there a way in which I can help?

I've so far held off porting my project to use zmq because I didn't want to have to jump through hoops especially when it comes to other people building the code. But it's becoming painful.

Is there a policy when there are releases? I looked through the past and the spacing between releases seems fairly random, rarely more than six months but I also see that 4.2.3 took almost a year :( LIkewise I cannot deduce the criteria for releases from the changelog.

@TobiSchluter
Copy link
Author

ps I see that other zmq packages are introducing workarounds for exactly the issue this patch is fixing, e.g. https://github.com/zeromq/cppzmq/blame/a3e5b54c3cad9dc1c4e86555e47365c2d3950e63/zmq.hpp#L30 (discussed here: zeromq/cppzmq#391 (comment) ). In other words, this is not an esoteric problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants