Skip to content

Require ThreadSafeBox.T to be Sendable #1394

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
Jun 3, 2024
Merged

Require ThreadSafeBox.T to be Sendable #1394

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
2 changes: 1 addition & 1 deletion Sources/SKSupport/AsyncUtils.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ public extension Task where Failure == Never {
///
/// If the task executing `withCancellableCheckedThrowingContinuation` gets
/// cancelled, `cancel` is invoked with the handle that `operation` provided.
public func withCancellableCheckedThrowingContinuation<Handle, Result>(
public func withCancellableCheckedThrowingContinuation<Handle: Sendable, Result>(
_ operation: (_ continuation: CheckedContinuation<Result, any Error>) -> Handle,
cancel: @Sendable (Handle) -> Void
) async throws -> Result {
Expand Down
7 changes: 6 additions & 1 deletion Sources/SKSupport/ThreadSafeBox.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ extension NSLock {
/// A thread safe container that contains a value of type `T`.
///
/// - Note: Unchecked sendable conformance because value is guarded by a lock.
public class ThreadSafeBox<T>: @unchecked Sendable {
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, what issues are being caused by it not being Sendable?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the main issue that I noticed was that some of the request handles in TestSourceKitLSPClient weren’t sendable. I didn’t technically hit any bug from it but I was wondering why I could modify local variables from the surrounding context from within the request handler, which generally isn’t safe.

public class ThreadSafeBox<T: Sendable>: @unchecked Sendable {
/// Lock guarding `_value`.
private let lock = NSLock()

Expand All @@ -41,6 +41,11 @@ public class ThreadSafeBox<T>: @unchecked Sendable {
_value = newValue
}
}
_modify {
lock.lock()
defer { lock.unlock() }
yield &_value
}
}

public init(initialValue: T) {
Expand Down
4 changes: 2 additions & 2 deletions Sources/SKTestSupport/TestSourceKitLSPClient.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ extension SourceKitLSPServer.Options {
/// that the server sends to the client.
public final class TestSourceKitLSPClient: MessageHandler {
/// A function that takes a request and returns the request's response.
public typealias RequestHandler<Request: RequestType> = (Request) -> Request.Response
public typealias RequestHandler<Request: RequestType> = @Sendable (Request) -> Request.Response

/// The ID that should be assigned to the next request sent to the `server`.
/// `nonisolated(unsafe)` is fine because `nextRequestID` is atomic.
Expand Down Expand Up @@ -72,7 +72,7 @@ public final class TestSourceKitLSPClient: MessageHandler {
///
/// `isOneShort` if the request handler should only serve a single request and should be removed from
/// `requestHandlers` after it has been called.
private nonisolated(unsafe) var requestHandlers: ThreadSafeBox<[(requestHandler: Any, isOneShot: Bool)]> =
private nonisolated(unsafe) var requestHandlers: ThreadSafeBox<[(requestHandler: Sendable, isOneShot: Bool)]> =
ThreadSafeBox(initialValue: [])

/// A closure that is called when the `TestSourceKitLSPClient` is destructed.
Expand Down
6 changes: 6 additions & 0 deletions Sources/SourceKitD/SourceKitD.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,12 @@ import Dispatch
import Foundation
import SKSupport

#if compiler(>=6)
extension sourcekitd_api_request_handle_t: @retroactive @unchecked Sendable {}
#else
extension sourcekitd_api_request_handle_t: @unchecked Sendable {}
#endif

/// Access to sourcekitd API, taking care of initialization, shutdown, and notification handler
/// multiplexing.
///
Expand Down