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: Event loop based on IOCP #12149

Conversation

straight-shoota
Copy link
Member

This patch implements the event loop for windows, based on IOCP. It follows up on #10784 which introduced overlapped operations.

It consists of:

  • The original implementation from WIP: Win32 event loop #11776, rebased on current master
  • @wonderix' refactoring to move WSAOVERLAPPED on the heap and keep it allocated until the cancellation is completed.
  • Some minor code enhancements (no functional changes)
  • Skip unix socket specs on win32, so now you can successfully run (most) socket specs via .\bin\crystal spec .\spec\std\socket\

Cudos to @wonderix for digging into this and finding the flaw in how we used the Win32 API, with thanks to @yxhuvud and @HertzDevil for helping out along the way (see discussion in #11776). Thanks to @neatorobito who laid the groundwork in #9957, as well as @kubo and @cfsamson for their input in #6957. 🙇

Digging through all these discussions shows it's been a long road.

But it looks like we now have a pretty much functional event loop on Windows 🚀

Closes #9957
Closes #11776

@straight-shoota straight-shoota added kind:feature topic:stdlib:concurrency platform:windows Windows support based on the MSVC toolchain / Win32 API labels Jun 22, 2022
@HertzDevil

This comment was marked as resolved.

class OverlappedOperation
enum State
INITIALIZED
STARTED
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my first implementation, I enqueued the OverlappedOperation inside the start method. For this, I needed a separate state STARTED for this. This is no longer required since the enqueuing only happens when the operation is cancelled.
I think STARTED could be removed.

Copy link
Member Author

@straight-shoota straight-shoota Jun 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I believe there can be some more refactoring of the state. But I'd leave it as is for now and do that in a follow-up later. The current code is working and I'd prefer to get it into master soon and then work on improving it further.

@straight-shoota

This comment was marked as resolved.

@HertzDevil

This comment was marked as resolved.

@straight-shoota

This comment was marked as resolved.

@Sija
Copy link
Contributor

Sija commented Jun 23, 2022

They're here - #12146

Copy link
Member

@beta-ziliani beta-ziliani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two improvements for the future:

  • The pending spec.
  • A bit of comments to help understand the needed pointer arithmetic (or a patch that avoids it, if there can be such thing).

But let's get it rolling 🚀 Thanks ppl for solving this important issue! 🙇

@@ -191,7 +191,7 @@ describe "select" do
x.should eq 2
end

it "stress select with send/receive in multiple fibers" do
pending_win32 "stress select with send/receive in multiple fibers" do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FTR in theory nothing is preventing this to work, all of the ingredients are there, but it's still failing.

@beta-ziliani beta-ziliani added this to the 1.5.0 milestone Jun 27, 2022
@beta-ziliani
Copy link
Member

(PS: I'm adding to the 1.5 milestone even when we're in the freeze period because it doesn't affect any of the supported platforms)

@beta-ziliani beta-ziliani mentioned this pull request Jun 28, 2022
22 tasks
@straight-shoota straight-shoota merged commit 632a86b into crystal-lang:master Jun 29, 2022
@straight-shoota straight-shoota deleted the feature/win32-event_loop-rebased branch June 29, 2022 17:11
@shayneoneill
Copy link

Guys, I know this is a work page (and a closed one), but my dudes take a moment to pat yourselves on the pat, because you beautiful maniacs deserve that pat on the back.
The event loop is a huge leap forward, and personally every attempt I've made in the part to try working with the windows port was thwarted by it. It works! Your all amazing!

We're now one step closer to being able to use my favorite language, anywhere its needed. And thats a glorious thing.

Thankyou!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:feature platform:windows Windows support based on the MSVC toolchain / Win32 API topic:stdlib:concurrency
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants