Skip to content

Commit

Permalink
Fix race condition in pthread_create handle initialization
Browse files Browse the repository at this point in the history
  • Loading branch information
HertzDevil committed Sep 27, 2024
1 parent d58ede5 commit 1317cc6
Show file tree
Hide file tree
Showing 4 changed files with 19 additions and 28 deletions.
8 changes: 0 additions & 8 deletions spec/std/thread/condition_variable_spec.cr
Original file line number Diff line number Diff line change
@@ -1,11 +1,3 @@
{% if flag?(:musl) %}
# FIXME: These thread specs occasionally fail on musl/alpine based ci, so
# they're disabled for now to reduce noise.
# See https://github.com/crystal-lang/crystal/issues/8738
pending Thread::ConditionVariable
{% skip_file %}
{% end %}

require "../spec_helper"

# interpreter doesn't support threads yet (#14287)
Expand Down
8 changes: 0 additions & 8 deletions spec/std/thread/mutex_spec.cr
Original file line number Diff line number Diff line change
@@ -1,11 +1,3 @@
{% if flag?(:musl) %}
# FIXME: These thread specs occasionally fail on musl/alpine based ci, so
# they're disabled for now to reduce noise.
# See https://github.com/crystal-lang/crystal/issues/8738
pending Thread::Mutex
{% skip_file %}
{% end %}

require "../spec_helper"

# interpreter doesn't support threads yet (#14287)
Expand Down
8 changes: 0 additions & 8 deletions spec/std/thread_spec.cr
Original file line number Diff line number Diff line change
@@ -1,13 +1,5 @@
require "./spec_helper"

{% if flag?(:musl) %}
# FIXME: These thread specs occasionally fail on musl/alpine based ci, so
# they're disabled for now to reduce noise.
# See https://github.com/crystal-lang/crystal/issues/8738
pending Thread
{% skip_file %}
{% end %}

# interpreter doesn't support threads yet (#14287)
pending_interpreted describe: Thread do
it "allows passing an argumentless fun to execute" do
Expand Down
23 changes: 19 additions & 4 deletions src/crystal/system/unix/pthread.cr
Original file line number Diff line number Diff line change
Expand Up @@ -9,20 +9,35 @@ module Crystal::System::Thread
@system_handle
end

protected setter system_handle

private def init_handle
# NOTE: the thread may start before `pthread_create` returns, so
# `@system_handle` must be set as soon as possible; we cannot use a separate
# handle and assign it to `@system_handle`, which would have been too late
# NOTE: `@system_handle` needs to be set here too, not just in
# `.thread_proc`, since the current thread might progress first; the value
# of `LibC.pthread_self` inside the new thread must be equal to this
# `@system_handle` after `pthread_create` returns
ret = GC.pthread_create(
thread: pointerof(@system_handle),
attr: Pointer(LibC::PthreadAttrT).null,
start: ->(data : Void*) { data.as(::Thread).start; Pointer(Void).null },
start: ->Thread.thread_proc(Void*),
arg: self.as(Void*),
)

raise RuntimeError.from_os_error("pthread_create", Errno.new(ret)) unless ret == 0
end

def self.thread_proc(data : Void*) : Void*
th = data.as(::Thread)

# `#start` calls `#stack_address`, which might read `@system_handle` before
# `GC.pthread_create` updates it in the original thread that spawned the
# current one, so we also assign to it here
th.system_handle = current_handle

th.start
Pointer(Void).null
end

def self.current_handle : Handle
LibC.pthread_self
end
Expand Down

0 comments on commit 1317cc6

Please sign in to comment.