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

Implement multithreading primitives on Windows #11647

Merged

Conversation

HertzDevil
Copy link
Contributor

Adds Win32 bindings for Thread, Thread::Mutex, and Thread::ConditionVariable. They represent system threads, so they should work independently from concurrency features, even without -Dpreview_mt. (Why aren't they namespaced under Crystal::System if they are internal platform-dependent types...?)

Related: #5430

@HertzDevil HertzDevil added kind:feature platform:windows Windows support based on the MSVC toolchain / Win32 API topic:multithreading labels Dec 23, 2021
thrdaddr: Pointer(LibC::UInt).null)

if @th.null?
raise RuntimeError.from_errno("_beginthreadex")
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, raising on null both here and in beginthreadex seems a bit redundant.

Copy link
Member

Choose a reason for hiding this comment

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

Indeed.

@straight-shoota straight-shoota self-requested a review May 5, 2022 17:16
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.

The Windows job is failing, can you run tests locally?

thrdaddr: Pointer(LibC::UInt).null)

if @th.null?
raise RuntimeError.from_errno("_beginthreadex")
Copy link
Member

Choose a reason for hiding this comment

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

Indeed.

@HertzDevil
Copy link
Contributor Author

I cannot reproduce the CI failure locally; the same compiler_spec.exe executable runs properly on my laptop when downloaded as an artifact. I will look into this later.

@HertzDevil
Copy link
Contributor Author

It seems Windows CI is working again, all that's left is fixing the WASI one.

straight-shoota
straight-shoota previously approved these changes Jun 8, 2022
src/crystal/system/win32/thread.cr Outdated Show resolved Hide resolved
@HertzDevil
Copy link
Contributor Author

After countless pushes I have identified that std_spec breaks only when Thread::Mutex#finalize is defined. It doesn't matter if the body really deletes the critical section object. Other than that I am still clueless as to why it happens only on the virtual environment.

straight-shoota pushed a commit that referenced this pull request Jul 15, 2022
This enables the fiber-safe `Mutex` on Windows and removes the stub implementation. It is mostly unrelated to [`Thread::Mutex`](#11647).

The standard library uses `Mutex` in `Log::SyncDispatcher`; it does not seem to break. (On the other hand, the specs for `Log::AsyncDispatcher` require non-blocking pipes, which are not available yet.)
@HertzDevil HertzDevil added the pr:needs-work A PR requires modifications by the author. label Aug 29, 2022
@straight-shoota straight-shoota self-requested a review December 3, 2022 15:33
@straight-shoota straight-shoota added this to the 1.7.0 milestone Dec 10, 2022
@straight-shoota straight-shoota removed the pr:needs-work A PR requires modifications by the author. label Dec 10, 2022
@straight-shoota straight-shoota merged commit a3f9199 into crystal-lang:master Dec 11, 2022
@straight-shoota
Copy link
Member

FTR: This needed work because CI previously failed, despite working locally. Now CI was green and I considered it ready to be merged.
This doesn't mean the issue that caused CI to fail is fixed. But we'll have to see if problems appear.

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

Successfully merging this pull request may close these issues.

4 participants