Skip to content

Replace TransactionBody with NIOAsyncSequenceProducer #677

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 11 commits into from
Apr 12, 2023

Conversation

dnadoba
Copy link
Collaborator

@dnadoba dnadoba commented Apr 5, 2023

Motivation

We want to properly respect task cancelation during the call to HTTPClientResponse.Body.AsyncIterator.next().

Modification

Replace our custom AsyncSequence implementation by delegate most of the work to NIOAsyncSequenceProducer from NIOCore

Result

We fully support cancelation throughout the stack and get the performance benefits of NIOAsyncSequenceProducer.

dnadoba added 6 commits March 31, 2023 16:44
Conflicts:
	Sources/AsyncHTTPClient/AsyncAwait/HTTPClientResponse.swift
	Sources/AsyncHTTPClient/AsyncAwait/Transaction.swift
	Sources/AsyncHTTPClient/AsyncAwait/TransactionBody.swift
which currently fails because of apple/swift-nio#2398
@dnadoba dnadoba added the 🆕 semver/minor Adds new public API. label Apr 5, 2023
@dnadoba
Copy link
Collaborator Author

dnadoba commented Apr 12, 2023

We now also throw CancellationError instead of HTTPClientError.cancelled if the task is cancelled while awaiting the response head.

Copy link
Collaborator

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

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

I think this looks good but I'm not super familiar with AHC.

@Lukasa Lukasa merged commit e18db27 into swift-server:main Apr 12, 2023
@dnadoba dnadoba deleted the dn-response-body-cancelation branch April 12, 2023 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🆕 semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants