Skip to content

Add data(from:delegate:) method. #4705

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

Closed

Conversation

b1ackturtle
Copy link
Contributor

Related issue: #3205

@b1ackturtle
Copy link
Contributor Author

@parkera
Could you please give me your feedback?

@parkera parkera requested a review from jrflat February 27, 2023 17:34
@b1ackturtle
Copy link
Contributor Author

@jrflat
Could you please review?

@MaxDesiatov
Copy link
Contributor

@swift-ci test

@@ -28,7 +28,7 @@ private class Bag<Element> {

/// A cancelable object that refers to the lifetime
/// of processing a given request.
open class URLSessionTask : NSObject, NSCopying {
open class URLSessionTask : NSObject, NSCopying, @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.

Has this class been audited for thread-safety to get this Sendable annotation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In Foundation on Apple platforms, URLSessionTask has already conformed to Sendable. So, I naturally assume that the URLSessionTask in Core Foundation could also conform to Sendable. Do you think any detailed audits are necessary?

If we need additional audits, I'd like to contribute to it. However, I'm not sure how to audit for thread-safety of this class. Could you please give me your advice?

Copy link
Contributor

Choose a reason for hiding this comment

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

In Foundation on Apple platforms, URLSessionTask has already conformed to Sendable. So, I naturally assume that the URLSessionTask in Core Foundation could also conform to Sendable.

URLSessionTask is implemented in Foundation proper, it's not present in CoreFoundation. You can confirm that by searching the CoreFoundation directory.

Do you think any detailed audits are necessary?

Yes, of course. If something is marked as Sendable in Foundation on Darwin platforms, it means that an audit was done and necessary changes that make it thread-safe applied if needed on it. Since these are different codebases, it has to be done separately in this repository too.

If we need additional audits, I'd like to contribute to it. However, I'm not sure how to audit for thread-safety of this class. Could you please give me your advice?

I don't think there's anything specific to this repository when compared to a general thread-safety audit on an arbitrary codebase. As a bare minimum one would have to confirm that mutable state accessible from different threads is either synchronized or made immutable. If there's a class hierarchy involved, this has to be done to mutable state inherited from superclasses too. In Swift I'd probably start with making a type Sendable (not @unchecked Sendable) and then auditing and adding matching sendability annotations to types involved in those warnings.

@jrflat
Copy link
Contributor

jrflat commented Jun 4, 2024

@b1ackturtle Sorry for the slow review here, thanks for your work on this! I built on top of your commit in #4970 to add the remaining async methods and address the Sendability requirement without marking URLSessionTask as @unchecked Sendable. Any feedback is welcome. Thanks again!

@b1ackturtle
Copy link
Contributor Author

@jrflat
Thank you for your review.
I also appreciate your additional commits to address the @unchecked Sendable conformance.
I hadn't thought of this solution when creating the PR, so your approach has been very educational for me.

@DougGregor
Copy link
Member

Should we close this PR now that #4970 has landed?

@parkera parkera closed this Jul 2, 2024
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.

6 participants