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

Windows Backend (IOCP) #42

Merged
merged 5 commits into from
Sep 21, 2023
Merged

Conversation

Corendos
Copy link
Contributor

@Corendos Corendos commented Apr 18, 2023

Description

This PR brings support for Windows using the I/O Completion Port.

Closes #10

Notes for reviewers

Unfortunately, this is a pretty beefy PR. I tried to clean the git history so that it's kind of reviewable commit by commit though.

My approach was similar to what @mitchellh suggested me: first the backend core, then the watchers building on top of it.

I also added support for Windows tests in GH Actions as he told me he didn't have access to a Windows machine.

Please note that this work is far from perfect, it was my first time playing with IOCP so expect some weirdness/mistakes.

Known pain-points/limitations

Timers
Timings rely on GetQueuedCompletionStatusEx timeout parameter. I read multiple times that timings on Windows were, well, slightly inaccurate (plot twist: that's a euphemism). I'm not confident enough to deep dive into this rabbit hole, so if somebody has more knowledge on that, feel free to improve my implementation.

Async
I have the feeling that there is a better way to implement the Async watchers compared to what I did. I couldn't come up with a better way than inspiring myself from the WASI backend.

The part I struggled the most with was the fact that Async can be notified before being linked to a Loop. This requires to store the fact that it's notified inside the Async struct, but then it opens up a lot of other questions and failed to find a satisfying solution taking care of all of them.

I'll try to come back to it with a fresh mind but I'm open to suggestions in the meantime 😁

Completion port HANDLE registering
In order to be able to wait for asynchronous I/O, HANDLE needs to be linked to a Completion Port. However, you can only register it once otherwise, the linking call fails. I first tried to let the user handle that by giving the possibility to link a handle to a Loop, but that ended up being unreliable and cumbersome when implementing Watchers.

My final decision was to ignore the failure and suppose that it always works. Win32 documentation is not what we could call exhaustive, so I don't if it's possible for an association to fail on the first call to CreateIoCompletionPort with a newly created HANDLE. Worst scenario is that it fails silently and asynchronous wait on this HANDLE never completes. If anyone has an idea as to how we could properly solve that, don't hesitate 😁

UDP Benchmark tweaks
This is described in #38

Process support
I didn't implement support for Process watchers yet. I feel like it's not required to merge this (already big) PR. I'll see if adding support is possible in the future.

@Corendos Corendos marked this pull request as ready for review April 23, 2023 11:17
@Corendos Corendos force-pushed the feat/iocp-backend branch 3 times, most recently from 88ad694 to 540a642 Compare September 4, 2023 22:49
@mitchellh
Copy link
Owner

I had someone else verify this works, so I'm just going to merge it. I'm still working on getting a Windows dev machine and CI setup.

@mitchellh mitchellh merged commit 69c3ae0 into mitchellh:main Sep 21, 2023
10 checks passed
@mitchellh
Copy link
Owner

Thank you so much @Corendos!! This is so amazing.

@mitchellh
Copy link
Owner

Oh you did the windows CI too, so that covers that. Thank you!

@mitchellh mitchellh mentioned this pull request Sep 21, 2023
@gedw99
Copy link

gedw99 commented Sep 22, 2023

if you need to test on a Windows box and your on a Mac I found UTM to be seamless.

https://github.com/utmapp/UTM/releases/tag/v4.3.5

@Corendos Corendos deleted the feat/iocp-backend branch December 17, 2023 17:33
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.

Windows backend
3 participants