Skip to content
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

Add default implementations for three default protocol conformances in the URLSessionDelegate family #5022

Merged
merged 1 commit into from
Aug 24, 2024

Conversation

jakepetroules
Copy link
Contributor

Implementing these callbacks without calling the completion handler causes hangs in cases where these methods are called. Add reasonable default behaviors for all of them, to prevent this.

This issue has been in place for at least 8 years for one of these callbacks.

@jakepetroules jakepetroules requested a review from parkera July 20, 2024 21:57
@jakepetroules
Copy link
Contributor Author

@swift-ci please test

@parkera parkera requested a review from jrflat July 21, 2024 16:16
@parkera
Copy link
Contributor

parkera commented Jul 21, 2024

@jrflat to review (and also to verify behavior compatibility with Darwin)

@parkera
Copy link
Contributor

parkera commented Jul 21, 2024

I think at the very least we need test cases to cover the desired new behavior.

Copy link
Contributor

@jrflat jrflat left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this! These default behaviors all LGTM and match the Darwin behavior. I just left one comment about needing to call both the task and session delegate.

Copy link
Contributor

@parkera parkera left a comment

Choose a reason for hiding this comment

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

Marking as request changes per @jrflat feedback.

…n the URLSessionDelegate family

Implementing these callbacks without calling the completion handler causes hangs in cases where these methods are called. Add reasonable default behaviors for all of them, to prevent this.

This issue has been in place for at least 8 years for one of these callbacks.
@jakepetroules jakepetroules force-pushed the urlsessiondelegate-default-implementations branch from d43a53f to 3420746 Compare July 30, 2024 04:25
@jakepetroules
Copy link
Contributor Author

Thanks for the review @jrflat! Does this look right, now?

@jakepetroules
Copy link
Contributor Author

@swift-ci please test

Copy link
Contributor

@jrflat jrflat left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@jrflat
Copy link
Contributor

jrflat commented Jul 31, 2024

@swift-ci please test Windows platform

@jakepetroules jakepetroules merged commit 018d8ef into main Aug 24, 2024
2 of 3 checks passed
@jakepetroules jakepetroules deleted the urlsessiondelegate-default-implementations branch August 24, 2024 17:14
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