Skip to content

Conversation

@rzhao271
Copy link

@rzhao271 rzhao271 commented May 10, 2024

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, which raises the minimum required Windows version to Windows 8.
This PR uses condition variables, bringing the minimum required Windows version to Windows Vista.

Older version of this PR that was for the dist branch: #36

@rzhao271

This comment was marked as outdated.

rzhao271

This comment was marked as outdated.

@rzhao271

This comment was marked as outdated.

@rzhao271
Copy link
Author

I rebuilt the change on a new device and noticed that test-reflock does not pass anymore. I'll investigate my changes further.

@dhanalla
Copy link

Thanks @rzhao271 for creating this PR.
As part of our security compliance initiative, we are also working to address undocumented APIs, as these are utilized within the OpenJDK repository.

@rzhao271
Copy link
Author

I have switched to using condition variables and added another bitflag, giving the following advantages:

  • No need for synchronization.lib
  • The minimum required OS version is Windows Vista or Windows Server 2008
  • Using two condition variables per reflock and two bitflags makes it clearer that the signal and await functions actually wait for each other
  • Spurious wakeups are handled for both signal and await functions with the while loops and bitflags
  • Tests pass even after I increment TEST_ITERATIONS in test-reflock.c to 200
  • No noticeable performance impact between this version and the keyed event one with TEST_ITERATIONS=200; both take around a minute each on my device.

@AlanBateman
Copy link

Is there any possibility of getting the changes merged here? Ideally OpenJDK wouldn't have a fork with the changes.

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.

3 participants