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

fix: use documented synch APIs #36

Closed
wants to merge 1 commit into from

Conversation

rzhao271
Copy link

Downstream PR: zeromq/libzmq#4680

This PR would help Microsoft repositories indirectly consuming and rebuilding wepoll, such as microsoft/zeromq-prebuilt, to get into compliance with internal policies by using APIs only documented on learn.microsoft.com.

However, this PR also brings in a dependency on synchronization.lib. I'm also unsure whether there's a test suite I could run these changes against. So far, I have run the libzmq tests with these changes and confirmed they passed.

@piscisaureus
Copy link
Owner

A couple of remarks:

  • To my best knowledge, WaitOnAddress is allowed to spuriously wake up, which the existing code base does expect and this PR does not anticipate.
  • Wepoll supports versions of windows that go pretty far back (Windows 7) and a bunch of non-msvc toolchains (e.g. MinGW). Does this PR change that?
  • Wepoll relies broadly on undocumented APIs, including interaction with the AFD driver. Should I expect patches for those too?

PS: this PR was made against the "dist" branch which contains the amalgamated source code. The un-amalgamated source code, including tests, can be found here: https://github.com/piscisaureus/wepoll/tree/master

@rzhao271
Copy link
Author

Thanks for the quick review!

The WaitOnAddress docs mention that the minimum supported client version for WaitOnAddress is Windows 8. WakeByAddressSingle has the same requirement.

As for the undocumented APIs, this PR addresses all of the in-scope ones, though I'd like to try out my changes on the master branch later this week.

@rzhao271
Copy link
Author

Closing this PR and re-opening one for the master branch.

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