Skip to content

Add sendable annotation to userInfo closure #1979

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.

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
Feb 17, 2025

Conversation

bnbarham
Copy link
Contributor

@bnbarham bnbarham commented Feb 13, 2025

swift-foundation recently landed a change (in
swiftlang/swift-foundation#764) which requires any Sendable values in JSONEncoder.userInfo. This causes a build failure:

JSONRPCConnection.swift:370:50: error: type '(RequestID) -> Optional<any ResponseType.Type>' does not conform to the 'Sendable' protocol
368 |
369 |     // Setup callback for response type.
370 |     decoder.userInfo[.responseTypeCallbackKey] = { (id: RequestID) -> ResponseType.Type? in
    |                                                  |- error: type '(RequestID) -> Optional<any ResponseType.Type>' does not conform to the 'Sendable' protocol
    |                                                  `- note: a function type must be marked '@Sendable' to conform to 'Sendable'
371 |       guard let outstanding = self.outstandingRequests[id] else {
372 |         logger.error("Unknown request for \(id, privacy: .public)")

Make the closure sendable, which is safe as we're only reading from outstandingRequests (where all our writes are guarded by the same queue that the decoding is on).

@bnbarham bnbarham requested a review from ahoppen as a code owner February 13, 2025 05:13
@bnbarham
Copy link
Contributor Author

@swift-ci please test Windows platform

@bnbarham
Copy link
Contributor Author

(note: completely untested, just running Windows since Linux will fail with docc first)

Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Marking the closure as Sendable implies that JSONDecoder should be able to invoke it multiple times concurrently when deserializing the JSON at which point we would get concurrent accesses to outstandingRequests. I think that should be fine because they are all just reading, which should be fine (StackOverflow seems to imply so https://stackoverflow.com/questions/45666181/is-it-safe-to-access-not-modify-a-swift-dictionary-from-multiple-threads) because the lock on queue guarantees that there are no concurrent writes.

I think it would be good to include that rationale in a comment so that we don't accidentally start modifying outstandingRequests in that closure in the future.

@bnbarham bnbarham force-pushed the fix-sendable branch 2 times, most recently from 2589db2 to db0a67b Compare February 14, 2025 21:21
@bnbarham
Copy link
Contributor Author

swiftlang/swift#79380

@swift-ci please test

@ahoppen
Copy link
Member

ahoppen commented Feb 14, 2025

swiftlang/swift#79380

@swift-ci please test Windows

@bnbarham
Copy link
Contributor Author

@swift-ci please test Windows platform

swift-foundation recently landed a change (in
swiftlang/swift-foundation#764) which requires `any Sendable` values in
`JSONEncoder.userInfo`. This causes a build failure:
```
JSONRPCConnection.swift:370:50: error: type '(RequestID) -> Optional<any ResponseType.Type>' does not conform to the 'Sendable' protocol
368 |
369 |     // Setup callback for response type.
370 |     decoder.userInfo[.responseTypeCallbackKey] = { (id: RequestID) -> ResponseType.Type? in
    |                                                  |- error: type '(RequestID) -> Optional<any ResponseType.Type>' does not conform to the 'Sendable' protocol
    |                                                  `- note: a function type must be marked '@sendable' to conform to 'Sendable'
371 |       guard let outstanding = self.outstandingRequests[id] else {
372 |         logger.error("Unknown request for \(id, privacy: .public)")
```

Make the closure sendable, which is safe as we're only reading from
`outstandingRequests` (where all our writes are guarded by the same
queue that the decoding is on).
@bnbarham
Copy link
Contributor Author

@swift-ci please test

@bnbarham
Copy link
Contributor Author

@swift-ci please test Windows platform

1 similar comment
@bnbarham
Copy link
Contributor Author

@swift-ci please test Windows platform

@bnbarham
Copy link
Contributor Author

@swift-ci please test macOS platform

@bnbarham bnbarham merged commit e8e1972 into swiftlang:main Feb 17, 2025
3 checks passed
@bnbarham bnbarham deleted the fix-sendable branch February 17, 2025 22:17
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.

3 participants