Skip to content

Don't suspend/resume the thread while holding an exclusive inout reference #198

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 16, 2025
+30 −14
Merged

Don't suspend/resume the thread while holding an exclusive inout reference #198

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions .github/workflows/pull_request.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,15 @@ jobs:
# Test dependencies
yum install -y procps
fi
linux_build_command: 'swift-format lint -s -r --configuration ./.swift-format . && swift test && swift test --disable-default-traits'
linux_build_command: 'swift-format lint -s -r --configuration ./.swift-format . && swift test && swift test -c release && swift test --disable-default-traits'
windows_swift_versions: '["6.1", "nightly-main"]'
windows_build_command: |
Invoke-Program swift test
Invoke-Program swift test -c release
Invoke-Program swift test --disable-default-traits
enable_macos_checks: true
macos_xcode_versions: '["16.3"]'
macos_build_command: 'xcrun swift-format lint -s -r --configuration ./.swift-format . && xcrun swift test && xcrun swift test --disable-default-traits'
macos_build_command: 'xcrun swift-format lint -s -r --configuration ./.swift-format . && xcrun swift test && xcrun swift test -c release && xcrun swift test --disable-default-traits'
enable_linux_static_sdk_build: true
linux_static_sdk_versions: '["6.1", "nightly-6.2"]'
linux_static_sdk_build_command: |
Expand Down
16 changes: 14 additions & 2 deletions Sources/Subprocess/Platforms/Subprocess+Windows.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1166,8 +1166,20 @@ extension Configuration {
DeleteProcThreadAttributeList(attributeList)
}

var handles = Array(inheritedHandles)
handles.withUnsafeMutableBufferPointer { inheritedHandlesPtr in
let handles = Array(inheritedHandles)

// Manually allocate an array instead of using withUnsafeMutableBufferPointer, since the
// UpdateProcThreadAttribute documentation states that the lpValue pointer must persist
// until the attribute list is destroyed using DeleteProcThreadAttributeList.
let handlesPointer = UnsafeMutablePointer<HANDLE>.allocate(capacity: handles.count)
defer {
handlesPointer.deinitialize(count: handles.count)
handlesPointer.deallocate()
}
handlesPointer.initialize(from: handles, count: handles.count)
let inheritedHandlesPtr = UnsafeMutableBufferPointer(start: handlesPointer, count: handles.count)

do {
_ = UpdateProcThreadAttribute(
attributeList,
0,
Expand Down
23 changes: 13 additions & 10 deletions Sources/Subprocess/Thread.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,8 @@ private final class WorkQueue: Sendable {
}

private func withUnsafeUnderlyingLock<R>(
_ body: (UnsafeMutablePointer<MutexType>, inout [BackgroundWorkItem]) throws -> R
condition: (inout [BackgroundWorkItem]) -> Bool = { _ in false },
body: (UnsafeMutablePointer<MutexType>, inout [BackgroundWorkItem]) throws -> R
) rethrows -> R {
#if canImport(WinSDK)
EnterCriticalSection(self.mutex)
Expand All @@ -114,20 +115,22 @@ private final class WorkQueue: Sendable {
pthread_mutex_unlock(mutex)
}
#endif
if condition(&queue) {
#if canImport(WinSDK)
SleepConditionVariableCS(self.waitCondition, mutex, INFINITE)
#else
pthread_cond_wait(self.waitCondition, mutex)
Copy link

Choose a reason for hiding this comment

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

This doesn't look right. pthread_cont_wait and pretty much all other low-level condition variable implementations I have used can wake up spuriously. So usually they need to be in a loop. I.e. while condition(&queue) { instead of if condition(&queue)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that alone would be correct in this particular case. If I change this to while, it'll hang during shutdown(), because shutdown() clears the queue (making it empty) and then wakes the condition variable. It must proceed in that case rather than go back to sleep.

That said, I think you may be right that something is generally amiss because where dequeue() is called it looks like it could end up terminating the work queue early if the condition variable was woken spuriously, since returning nil can't distinguish "no work items right now" versus "entered shutdown mode".

Copy link

Choose a reason for hiding this comment

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

Yeah, that doesn't seem good. Condition variables without a while are almost never right.

And yes, you'll need to add some sentinel to distinguish shutdown vs. empty

#endif
}
return try body(mutex, &queue)
}

// Only called in worker thread. Sleeps the thread if there's no more item
func dequeue() -> BackgroundWorkItem? {
return self.withUnsafeUnderlyingLock { mutex, queue in
if queue.isEmpty {
// Sleep the worker thread if there's no more work
#if canImport(WinSDK)
SleepConditionVariableCS(self.waitCondition, mutex, INFINITE)
#else
pthread_cond_wait(self.waitCondition, mutex)
#endif
}
return self.withUnsafeUnderlyingLock { queue in
// Sleep the worker thread if there's no more work
queue.isEmpty
} body: { mutex, queue in
guard !queue.isEmpty else {
return nil
}
Expand Down