Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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: 4 additions & 0 deletions Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,10 @@ let package = Package(
name: "BazelProtobufBindings",
dependencies: [
.product(name: "SwiftProtobuf", package: "swift-protobuf")
],
exclude: [
"README.md",
"analysis_v2.proto",
]
),
.testTarget(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,30 +37,32 @@ final class BSPMessageHandler: MessageHandler {

// We currently use a single-threaded setup for simplicity,
// but we can eventually reply asynchronously if we find a need for it.
private let lock: OSAllocatedUnfairLock<Void> = .init()
// FIXME: Can't put state into the lock for now because of recursiveness in the registration.
nonisolated(unsafe) private var state: State = State()
private let state = LockIsolated<State>(.init())

init() {}

func register<Request: RequestType>(
requestHandler: @escaping BSPRequestHandler<Request>
) {
state.requestHandlers[Request.method] = AnyRequestHandler(
handler: requestHandler
)
state.modify { state in
state.requestHandlers[Request.method] = AnyRequestHandler(
handler: requestHandler
)
}
}

func register<Notification: NotificationType>(
notificationHandler: @escaping BSPNotificationHandler<Notification>
) {
state.notificationHandlers[Notification.method] = AnyNotificationHandler(
handler: notificationHandler
)
state.modify { state in
state.notificationHandlers[Notification.method] = AnyNotificationHandler(
handler: notificationHandler
)
}
}

func handle<Notification: NotificationType>(_ notification: Notification) {
lock.withLockUnchecked {
state.read { state in
logger.info(
"Received notification: \(Notification.method)"
)
Expand All @@ -78,7 +80,7 @@ final class BSPMessageHandler: MessageHandler {
id: RequestID,
reply: @escaping (LSPResult<Request.Response>) -> Void
) {
lock.withLockUnchecked {
state.read { state in
logger.info(
"Received request: \(Request.method)"
)
Expand Down Expand Up @@ -132,3 +134,24 @@ final class BSPMessageHandler: MessageHandler {
return handler
}
}

final class LockIsolated<Value>: @unchecked Sendable {
private var value: Value
private let lock = NSRecursiveLock()
Copy link
Member

Choose a reason for hiding this comment

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

Let me do some thinking on this. The thing is that OSAllocatedUnfairLock is much faster than NSRecursiveLock , and recursive lock in general are a sign of wrong design on our end. We might want to move things around a bit more instead or go for Swift concurrency.

Copy link
Member

Choose a reason for hiding this comment

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

Package change is 👍 however, if you want to open that separatedly we can merge that in already

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If performance is a key factor, NSRecursiveLock would be bad choice. It might be an easy way out to just use OSAllocatedUnfairLock with an internal state as to track the lock procession. Since it is only two level recursion for registration during the init request, the subsequent read calls should be just as fast.

Another approach is to use AsyncQueue from SourceKitLSP. The nice thing about this design is the task being run sequentially with Serial: DependencyTracker. That means we are moving into the Swift Concurrency world which has its own added benefits/upkeep


init(_ value: Value) {
self.value = value
}

func read<T>(_ closure: (Value) throws -> T) rethrows -> T {
lock.lock()
defer { lock.unlock() }
return try closure(value)
}

func modify<T>(_ closure: (inout Value) throws -> T) rethrows -> T {
lock.lock()
defer { lock.unlock() }
return try closure(&value)
}
}
47 changes: 47 additions & 0 deletions Tests/SourceKitBazelBSPTests/BSPMessageHandlerTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -127,4 +127,51 @@ import Testing

#expect(initialized)
}

@Test
func testRecursiveRegistration() async throws {
// setup mocks
let sourcesRequest = BuildTargetSourcesRequest(targets: [])
let sourcesResponse = BuildTargetSourcesResponse(items: [])
let initRequest = InitializeBuildRequest(
displayName: "mockInitRequest",
version: "1.0",
bspVersion: "2.0",
rootUri: URI(filePath: "/Users/BSP", isDirectory: false),
capabilities: .init(languageIds: [])
)
let initResponse = InitializeBuildResponse(
displayName: "mockInitResponse",
version: "1.0",
bspVersion: "2.0",
capabilities: .init()
)

// recursive registering
let registry = BSPMessageHandler()

registry.register { (request: InitializeBuildRequest, id: RequestID) in
registry.register { (request: BuildTargetSourcesRequest, id: RequestID) in
return sourcesResponse
}
return initResponse
}

// validating handler registered
registry.handle(initRequest, id: .number(1)) { result in
if case let .success(resp) = result {
#expect(resp.displayName == "mockInitResponse")
} else {
Issue.record("initRequest failed due to handler not found")
}
}

registry.handle(sourcesRequest, id: .number(2)) { result in
if case let .success(resp) = result {
#expect(resp.items.count == 0)
} else {
Issue.record("sourcesRequest failed due to handler not found")
}
}
}
}