Skip to content

Conversation

@mattt
Copy link
Contributor

@mattt mattt commented Apr 14, 2025

Resolves #35
Alternative to #72

The current send() method returns async throws -> M.Result which means the request is already executing when the method is called. This makes it impossible to:

  1. Cancel in-flight requests
  2. Execute multiple requests concurrently with timeouts
  3. Group multiple requests together with different error handling strategies

This PR changes send() to return Task<M.Result, Swift.Error> instead of using async throws. This gives callers more control over request lifecycle:

// Before: No way to cancel or timeout
let result = try await client.send(request)

// After: Full task control
let task = client.send(request)
// Can cancel
task.cancel()
// Can timeout
try await withThrowingTaskGroup { group in
    group.addTask { try await task.value }
    group.addTask {
        try await Task.sleep(for: .seconds(5))
        task.cancel()
        throw TimeoutError()
    }
}

This would be a breaking change to the public API - callers would do try await client.send(request).value (emphasis added) to get the same behavior as before.

I think this makes sense for the send() method, as it's lower-level than the request specific methods, like ping() (which itself calls send()). What I'm still wondering is, should those higher-level APIs should also be updated to return Task? Or is it reasonable to ask API consumers to use the lower-level API if they want more control?

@mattt mattt force-pushed the mattt/client-send-task branch from eb9a270 to c81abff Compare April 14, 2025 13:02
@mattt
Copy link
Contributor Author

mattt commented Apr 15, 2025

@mattmassicotte As someone well-versed in Swift concurrency primitives and API design, do you think this is a reasonable approach? Any suggestions to making this more flexible / ergonomic / conventional for API consumers?

@mattmassicotte
Copy link

mattmassicotte commented Apr 15, 2025

Timeout is a very common problem with concurrency in general, and one that isn't well-supported by built in stuff today.

But in this case, couldn't a consumer just wrap the original call in a Task if they wanted to? Also, with this change I think you no longer have the automatic cancellation propagation you'd get with a structured call.

@mattt
Copy link
Contributor Author

mattt commented Apr 15, 2025

@mattmassicotte Thanks for weighing in!

But in this case, couldn't a consumer just wrap the original call in a Task if they wanted to?

Yes, that's right. I don't have a good mental model for the overhead introduced by wrapping a task in another tasks; conceptually it feels wasteful, but maybe that's an emotional response.

Taking another look not and checking my priors, I think a lot of this was motivated by a thought I had that returning a Task would lend itself to easier composition with TaskGroup, but that's not the case. It's the opposite, actually (right?) — addTask takes a closure argument, not a Task. So we're back to wrapping tasks in tasks, again.

So the only possible argument I can make for this approach is that returning a Task could (maybe... somehow...) lead to a nicer API for retries and cancellation. But I don't think there's much evidence for that.

Also, with this change I think you no longer have the automatic cancellation propagation you'd get with a structured call.

Fair point!


Can you point to any projects that you think do a really good job at API design for a client like this? Something that supports cancellation and retries.

@mattmassicotte
Copy link

I think the intention is that structured concurrency (async functions and whatnot) already has built-in cancellation support. All these constructs must be used within a Task, and that can be cancelled. So, I think it is true that this change could make cancellation more difficult in the general case, but possibly a little bit easier in some specific ones. And that could be a trade-off that makes sense!

Reties is an interesting question. My gut is that such a concept has to be internalized somehow. Ultimately you have to either return from an async function or throw.

Timeout support is a well-known and frequently encountered gap in the current system. Modelling it with a group, like you did, is a very common approach. But I find it quite a bit of boilerplate and a little on the error-prone side. Unfortunately, I have not encountered any great specific examples that I can point to. But, here are few that I have noted. I've never used any, but perhaps there's good stuff in there.

https://github.com/ph1ps/swift-concurrency-deadline
https://github.com/ph1ps/swift-concurrency-retry
https://github.com/swhitty/swift-timeout

@mattt
Copy link
Contributor Author

mattt commented Apr 15, 2025

@mattmassicotte

I think the intention is that structured concurrency (async functions and whatnot) already has built-in cancellation support.

That's what I really needed to hear. I think coming most recently from Go, Python, and Rust, my mental model of concurrency has involved explicit cancellation. So I just need to reset my expectations for structured concurrency.

Timeout support is a well-known and frequently encountered gap in the current system. Modelling it with a group, like you did, is a very common approach. But I find it quite a bit of boilerplate and a little on the error-prone side.

Thanks for confirming that. Good to know I wasn't missing something obvious.

At the risk of getting in the business of providing low-level concurrency primitives, I wonder if it'd be reasonable for this SDK to provide some basic helper methods for the most common use cases.

I quite like the implementations you linked by @ph1ps. If I do add helpers, my plan would be to inline those with attribution as internal functions, and then export public static methods on Client that use them.

@mattmassicotte
Copy link

I wonder if it'd be reasonable for this SDK to provide some basic helper methods for the most common use cases.

I think this really comes down to how common a problem this will be for clients. If a first-party solution ever materializes, it would undoubtedly cause churn. Finding a balance here is tricky...

@mattt
Copy link
Contributor Author

mattt commented Apr 27, 2025

Alright, thanks to the insights from @mattmassicotte in this discussion, I believe there are better ways to address the problems we identified than what this PR does, so I'm closing this for now and opened #93 to track the implementation of an alternative approach.

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.

Add timeout parameter to Client.send method

3 participants