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

offer Structured Concurrency API that supports HTTP response streaming #807

Open
weissi opened this issue Feb 5, 2025 · 4 comments
Open

Comments

@weissi
Copy link
Contributor

weissi commented Feb 5, 2025

Currently, most of HTTPClient's API surface only supports Structured Concurrency violating functions. Baby steps to rectify this have been done in #806. But the elephant in the room is of course the unstructured HTTP request API.

We need to also provide a

try await httpClient.execute(request) { status, responseBodyStream in
   ...
}

API that fulfils Structured Concurrency by supporting cancellation and tearing everything down when the body function returns or throws without ineffective and ugly hacks like deinit (which still violates Structured Concurrency because it doesn't follow the structure of the code).

@FranzBusch
Copy link
Collaborator

FranzBusch commented Feb 5, 2025

I think we need slightly more since request can also be streamed and AsyncSequence is not enough for that. I'm thinking gRPC got it right here:

swift

try await httpClient.execute { requestWriter in
  try await requestWriter.write()
  return [:] // trailers
} onResponse: { status, responseBodyStream, trailers in
   print(status)
   for try await responseData in responseBodyStream { ... }
   try await trailers.value()
}

@weissi
Copy link
Contributor Author

weissi commented Feb 5, 2025

I think we need slightly more since request can also be streamed and AsyncSequence is not enough for that.

Possibly, but currently we do need an AsyncSequence<BB>, right? So this is will legal

try await httpClient.post("https://foo.com", body: myAsyncSequence)

Don't need any with* stuff here.

@FranzBusch
Copy link
Collaborator

I think we need slightly more since request can also be streamed and AsyncSequence is not enough for that.

Possibly, but currently we do need an AsyncSequence<BB>, right? So this is will legal

try await httpClient.post("https://foo.com", body: myAsyncSequence)

Don't need any with* stuff here.

Yes if we just wrap the current AHC APIs than this is enough. In general this API should always be legal it is just a convenience on top of the bi-di streaming API with a writer.

@weissi
Copy link
Contributor Author

weissi commented Feb 5, 2025

Yes if we just wrap the current AHC APIs than this is enough. In general this API should always be legal it is just a convenience on top of the bi-di streaming API with a writer.

Gotcha, yes. So I think there are (at least) two parts to this:

  1. For any existing AHC API offer one that does the same but adds Structured Concurrency
  2. Offer other, more general APIs

I was thinking (1) for this ticket and you added (2). That's fine, I'm happy with this, just to check we're on the same page.

weissi added a commit that referenced this issue Feb 6, 2025
At the moment, `HTTPClient`'s entire API surface violates Structured
Concurrency. Both the creation & shutdown of a HTTP client as well as
making requests (#807) doesn't follow Structured Concurrency. Some of
the problems are:

1. Upon return of methods, resources are still in active use in other
threads/tasks
2. Cancellation doesn't always work

This PR is baby steps towards a Structured Concurrency API, starting
with start/shutdown of the HTTP client.

Co-authored-by: Johannes Weiss <[email protected]>
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

No branches or pull requests

2 participants