Skip to content

Update WorkQueue.queue to be a reference type instead of an array. #194

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Update WorkQueue.queue to be a reference type instead of an array. #194

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
4 changes: 2 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,14 @@ 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
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add Invoke-Program swift test -c release below this line so we have release coverage for Windows as well?

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
29 changes: 25 additions & 4 deletions Sources/Subprocess/Thread.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,12 +77,33 @@ private struct BackgroundWorkItem {
// We can't use Mutex directly here because we need the underlying `pthread_mutex_t` to be
// exposed so we can use it with `pthread_cond_wait`.
private final class WorkQueue: Sendable {
private nonisolated(unsafe) var queue: [BackgroundWorkItem]
// Queue needs to be a reference type because we pass it inout
final class Queue: Sendable {
internal nonisolated(unsafe) var queue: [BackgroundWorkItem]
Copy link

Choose a reason for hiding this comment

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

This is incorrect because this type isn't Sendable. It allows mutation without synchronousiarion.

furthermore, the reference gets passed inout.

We should also add a test that shows where precisely you need reference semantics and why

Copy link
Contributor Author

@iCharlesHu iCharlesHu Oct 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This is incorrect because this type isn't Sendable. It allows mutation without synchronousiarion.

This type is exclusively used under a pthread_mutex_t. It is for that reason @unchecked Sendable.

We should also add a test that shows where precisely you need reference semantics and why

I don't quite understand what do you mean. Could you suggest a test?

Copy link

Choose a reason for hiding this comment

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

This is incorrect because this type isn't Sendable. It allows mutation without synchronousiarion.
This type is exclusively used under a pthread_mutex_t. It is for that reason @unchecked Sendable.

I get that this is intent but we should prove this to the compiler and not use @unchecked, that's unsafe. We have helpers like NIOLockedValueBox, OSAllocatedUnfairLock and Mutex which can make this safe. And we should use those

I don't quite understand what do you mean. Could you suggest a test?

I assume that the reason for this pull request is that something didn't work as expected. The test should test that before it doesn't work as expected but now (with this change) it does.

Copy link
Contributor Author

@iCharlesHu iCharlesHu Oct 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I get that this is intent but we should prove this to the compiler and not use @unchecked, that's unsafe. We have helpers like NIOLockedValueBox, OSAllocatedUnfairLock and Mutex which can make this safe. And we should use those

We can't use OSAllocatedUnfairLock nor Mutex directly here because we need access to the underlying pthread_mutex_t to pass to pthread_cond_wait. Because of this, I deliberately made WorkQueue NOT a generic LockedBox type. Instead, we use Mutex wherever we can and only use WorkQueue for very specific purposes. On the other hand, what's wrong with using @unchecked Sendable this way? Isn't this the canonical use case for @unchecked Sendable (when you explicitly use external synchronization instead of relying on Swift itself)?

I assume that the reason for this pull request is that something didn't work as expected. The test should test that before it doesn't work as expected but now (with this change) it does.

This change resolves issue #182. Currently, the run() function hangs on release builds on Darwin, and it no longer hangs on release builds with this PR. To prevent future regressions, I have added swift test -c release to the CI pipeline.

Copy link

Choose a reason for hiding this comment

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

Okay, but if you build a general purpose work queue, then I'd suggest

  • put it into its own file
  • Make it actually Sendable (currently it is not and just disables compiler checking)
  • Add some real tests for it

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at this code, it looks like Queue isn't actually Sendable like @weissi pointed out. The callsites however can use it safely as long as it is always guarded by a pthread_mutex_t.

Is my understanding correct? If so, then I tend to agree that marking this type Sendable is misleading. This code would still work fine even if we remove Sendable from Queue since you already declared the var as nonisolated(unsafe) below:

private nonisolated(unsafe) var queue: Queue

Copy link

@weissi weissi Oct 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

My position is: Anything @unchecked/unsafe needs to be avoided. That's just switching off compiler checking.

A narrow exception is if you're creating new, reusable components (such as a generic queue) with an extensive test suite that cannot be used unsafely through their API, then that's fine.

For example, AsyncProcess doesn't contain @uncheckeds at all


var isEmpty: Bool { self.queue.isEmpty }

func append(_ item: BackgroundWorkItem) {
self.queue.append(item)
}

func removeFirst() -> BackgroundWorkItem {
return self.queue.removeFirst()
}

func removeAll() { self.queue.removeAll() }

init() {
self.queue = []
}
}

private nonisolated(unsafe) var queue: Queue
internal nonisolated(unsafe) let mutex: UnsafeMutablePointer<MutexType>
internal nonisolated(unsafe) let waitCondition: UnsafeMutablePointer<ConditionType>

init() {
self.queue = []
self.queue = Queue()
self.mutex = UnsafeMutablePointer<MutexType>.allocate(capacity: 1)
self.waitCondition = UnsafeMutablePointer<ConditionType>.allocate(capacity: 1)
#if canImport(WinSDK)
Expand All @@ -94,14 +115,14 @@ private final class WorkQueue: Sendable {
#endif
}

func withLock<R>(_ body: (inout [BackgroundWorkItem]) throws -> R) rethrows -> R {
func withLock<R>(_ body: (inout Queue) throws -> R) rethrows -> R {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the point of using inout anymore if it's a reference type?

try withUnsafeUnderlyingLock { _, queue in
try body(&queue)
}
}

private func withUnsafeUnderlyingLock<R>(
_ body: (UnsafeMutablePointer<MutexType>, inout [BackgroundWorkItem]) throws -> R
_ body: (UnsafeMutablePointer<MutexType>, inout Queue) throws -> R
) rethrows -> R {
#if canImport(WinSDK)
EnterCriticalSection(self.mutex)
Expand Down