Skip to content

Comments

Add LockIsolated For Recursive Registration#21

Closed
sean7218 wants to merge 1 commit intospotify:mainfrom
sean7218:sean7218/recursive-handler-registration
Closed

Add LockIsolated For Recursive Registration#21
sean7218 wants to merge 1 commit intospotify:mainfrom
sean7218:sean7218/recursive-handler-registration

Conversation

@sean7218
Copy link
Contributor

@sean7218 sean7218 commented Jul 18, 2025

Refactor BSPMessageHandler to use LockIsolated for state management and add recursive registration test
- fix the build error in Package.swift
- use NSRecursiveLock to fix the nonisolated state

…nd add recursive registration test

    - fix the build error in Package.swift
    - use NSRecursiveLock to fix the nonisolated state
@sean7218 sean7218 requested a review from a team as a code owner July 18, 2025 05:16
@sean7218 sean7218 changed the title Refactor BSPMessageHandler to use LockIsolated Add LockIsolated For Recursive Registration Jul 18, 2025

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

@sean7218 sean7218 closed this Jul 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants