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

More checks for task cancellation and tests #44

Merged
merged 1 commit into from
Dec 11, 2023

Conversation

simonjbeaumont
Copy link
Collaborator

Motivation

In our fallback, buffered implementation, we did not use a task cancellation handler so were not proactively cancelling the URLSession task when the Swift concurrency task was cancelled. Additionally, while we did have a task cancellation handler in the streaming implementation, so the URLSession task would be cancelled, we were not actively checking for task cancellation as often as we could.

Modifications

  • Added more cooperative task cancellation.
  • Added tests for both implementations that when the parent task for the client request is cancelled that we get something sensible. Note that in some cases, the request will succeed. In the cases where the request fails, it will surface as a ClientError to the user where the underlyingError is either Swift.CancellationError or URLError with code == .cancelled.

Result

More cooperative task and URLSession task cancellation and more thorough tests.

Test Plan

Added unit tests.

@simonjbeaumont simonjbeaumont force-pushed the sb/cancellation branch 2 times, most recently from 9a11003 to 4056b84 Compare December 11, 2023 17:13
@simonjbeaumont simonjbeaumont enabled auto-merge (squash) December 11, 2023 17:15
@simonjbeaumont simonjbeaumont merged commit aac0a82 into apple:main Dec 11, 2023
5 checks passed
@czechboy0 czechboy0 added the semver/patch No public API change. label Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants