Task initializer with throwing closure swallows error

Using Swift
21

If this would be a quick-fix I'd even suggest to make this first version of the 3 provided alternatives into

_ = Task {
  let thing = try await someThing()
  print(thing)
}

as it fits to the usual way we discard things that are not @discardableResult and it's just 4 little characters of "boilerplate".

Logically, I second (or x-th) the idea that the Task initializers should only be @discardable when Success == Void and Failure == Never.

Small tangent & input about what's easier to grasp for a newbie (skip if you want):

I haven't run into this in production code (thankfully) yet, but while giving a quick, ill-prepared demo I gave to "advertise" for Swift's concurrency features. There's two issues at play here:

One is obviously the type inference combined with @discardableResult. Developers new to Swift, and perhaps also to concurrency in general, are more likely to view Task as a keyword and not an actual type whose values can be stored and used later. You do not get a "thing" that you can option-click to see what the compiler deduces for you in terms of type.
Instead it just looks like a special "scope notation", just like your first main method or the top level in a playground. However, in a "normal scope" you get a compiler-note if you don't properly handle errors and it's not natural for somebody to deduce "ah, the compiler deduces a Task<Void, Error> type here, then puts any errors in the discarded value of that type", so the error is actually handled, all the convenient "implicitness" of it just hides it. That's only apparent for somebody who's already intricately familiar with Swift.

We may have become a little "operationally blind" to the fact that no matter how well we design the language and what tools we provide, concurrency is hard. Yes, requiring a more explicit discarding of errors adds "boilerplate" for all those who just know that errors inside a simple Task { ... } can be swallowed and if they are actually interested in the error they will catch or await it somehow, but I think that strides to far from Swift's safe-ness philosophy. With quick-fixes and a minimal _ = in front of those Tasks we'd fare much better, I think.

For me Swift has been an excellent tool not just to learn about more advanced stuff myself, but also to teach concepts such as concurrency (another would be value- vs reference-semantics). Every time I need to explain "ah, this is a non-obvious special case where these 5 things play together" it loses a little of teachability, though.

4 Likes
22

I'm reviving this thread because there's been some suggestions from the community on how we'd like this improved, without much in the way of acknowledgement from maintainers.

So then would you agree that the Task initializers should've been defined such that the initializer is only discardable when Success == Void and Failure == Never ?

This seems like the most obvious solution to this problem, and it's frustrating to me that we haven't really had any feedback from maintainers since the start of the thread where it looks like the general problem being described wasn't initially understood.

People have made efforts since then to describe the problem in more detail, with possible solutions and general expectations from Swift as a language based on historical language decisions, so it would be good to have some sort of conclusion from this rather than just a dead end.

If there is another avenue we should take to raise, resolve or close this issue then that's fine we just need to know what to do since some of us will be new to contributing to Swift.

11 Likes
23

People are looking for workarounds, and are creative :) https://chaos.social/@ole/110062590498696349

Workarounds are not a fix, though - it would be cool if the language stopped ignoring Task errors.

7 Likes
24

I agree that this is dangerous and unexpected; try? is right there if you don't care about the result.

In the meantime, I added a SwiftLint rule, unhandled_throwing_task. It's available in 0.52.0, and uses SwiftSyntax, so should be fairly robust. Let me know if you run into any issues.

8 Likes
25

@kylebshr I see that SwiftLint now issues an error but how do you suggest we make our code conform to SwiftLint if we indeed want to get the Task .value to catch errors?
Here is a very popular solution by Sundell about automatically retrying an asynchronous Swift Task. He's using the value of the Task as he should, however in this situation the linter will complain.

Instead of

Task {
           [...]

            try Task<Never, Never>.checkCancellation()
            return try await operation()
        }

I would gladly write this to make my intent clear:

Task(priority: priority) {
           [...]

            do {
                try Task<Never, Never>.checkCancellation()
                return try await operation()
            } catch {
                throw error
            }
        }

but the linter still complains.

1 Like
26

I've been thrown off by this a few times recently as well. When I've run into it my first thought has been "this doesn't seem very Swift-like" to let me forget about dealing with the error.

1 Like
29

The idea there is to enforce Never, which I like…

struct 💣: Error { }

Task<_, Never> { throw 💣() } // No exact matches in call to initializer

…but it also gave me the idea that the intention to disregard errors would be much better expressed as

Task<_, Error> { throw 💣() }

rather than

_ = Task { throw 💣() }

I'm still on-board with at least part of the conditional discardableResult idea, but I think _ is better reserved for when Success is not Void.

_ = Task { "🫎" }

This is redundant, but if _ = were necessary, I think I would throw the <_, Error> in anyway. So I'm questioning the potential loss of discardableResult for Task<Void, (some) Error>.

_ = Task<_, Error> { throw 💣() }
30

Just to provide an example of what this unexpected behavior can mean for developers: Today I was trying to figure out why some requests in a WKWebView were not being sent. It came down to this line of code:

Task { [weak self, weak viewModel] in
    let accessToken = try await viewModel?.accessToken(forHost: host)
    self?.continueRequestInWebView(token: accessToken, host: host)
}

It was absolutely not obvious to me that the issue was that if there is no access token, the scope would be exited before the request could continue. There was no indication about this from the compiler.

Let me say that I've been using Swift since it was first released, and this behavior is definitely not what I would expect. I believe this has the potential to trip up new and experienced developers alike.

Some warning from the compiler would definitely be very welcome in this case.

4 Likes
31

I have run into exactly this today. I don't think Task is the only place where errors are silently ignored, but this definitely seems like something that should be an error when compiling. Force me to catch the error, or use try?, or something. This seems like a gaping hole in the language.

4 Likes
32

I have run into this today myself, looking for a bug in some old code where it was hard to spot a try in the body of a very verbose function body when you are not looking for one.

func importFoo() async throws -> AsyncThrowingStream<ImportState, Error>
{
    return AsyncThrowingStream<ImportState, Error> { continuation in
        
        Task {
            
            continuation.yield(ImportState.loading(progress: Progress()))
            ...LOT OF CODE... 
            let firstPage = try await importService.importStep()
            ...LOT OF CODE...

No warnings or errors of an unhandled try or unused Task result and catching. And nothing happening at runtime.

I talked with my colleagues and none was aware of this "quirk". And not everybody is yet using the new concurrency model at its full potential, i believe when this will happen the number of people complaining and falling for this will explode. And i believe this should really be addressed by the maintainers. Imho a compile time warning in the cases as suggested a few posts above would be an optimal solution.

4 Likes