Rename flatMapThrowing & introduce a REAL throwing flatMap

Related Projects SwiftNIO
1

Before you ask: Yes, I did search on this topic and yes, I did find dozens of places (on Twitter, GitHub, the forums etc.) where people are asking the same question and people are responding that this topic was already discussed and that the current design choice for the flatMapThrowing function is by design.

But this is ridiculous, obviously, people are getting confused by its name (me included) and I can totally understand why:

  1. flatMapThrowing is NOT a flatMap but instead a map
  2. there is no throwing counterpart for flatMap, it just doesn't exist, despite the existence of flatMapThrowing

Here's a quote from Johannes Weiss (SwiftNIO core team member) who explains the rationale behind this naming (source):

map s usually stay on the success path, no matter what they do. flatMap can go from the success path into the success path or the failure path. Therefore we named it flatMapThrowing (because if you throw , we'll go from the success path into the failure path).

So if I get this right, the main concern with the name mapThrowing was that users might think that because of the map part, it always succeeds. I'm repeating: A function, which has Throwing in its name, could be expected to always succeed. Honestly? Who would think that? At least for me the Throwing in the name of a function is like if someone was screaming at me "Be cautious, this thing culd go wrong! It will NOT always succeed!".

Then I also read another reason for the name flatMapThrowing, Johannes stated:

Try implementing flatMapThrowing from map and you'll see that it's not expressible as a map. It works more more like a flatMap, you can implement flatMapThrowing from just flatMap.

So, if I understand this correctly, he's basically saying that the implementation of flatMapThrowing makes use of flatMap and is not possible to be implemented with using a map internally. But why does that matter? Isn't it the very first fundemantal goal of any good Swift API Design to have clarity at the point of use? In other words: The implementation details of an API shouldn't influence the API naming if it makes it less clear at the point of use which is clearly the case for flatMapThrowing given that many people are getting confused by it.

By the way, Johannes himself mentions in the same source linked above that the API should have probably named differently, he suggests combineThrowing. Note that I don't understand why that would be in any way a better name, maybe @johannesweiss can explain that himself. But it shows that he agrees that we should probably rename it.

Then he also explains in the same comment linked above why he thinks that the missing actual flatMap that can throw is actually a good thing and a non-goal for the simple reason that it would then be able to fail in two ways, either by throwing or by resulting in an error Result case. He then goes on to say that he doesn't think it's a common use case and for the rare cases this might be needed, users could implement such a function themselves.

Please note that I've just started implementing my first real application using Vapor (which is using SwiftNIO) and that I am running into this problem with every model I have in my API & on 50% of my API functions for each of them for the simple reason that I have separate Request and Response types for each of my models. From my own experience, this is a very common pattern, e.g. consider a User model which doesn't need an id on request objects (when creating them, they don't need one, when updating they are part of the query URL) but always has an id in response objects.

I am currently working around this problem calling flatMapThrowing followed by flatMap { $0 } like in this sample code:

func fetch(req: Request) throws -> EventLoopFuture<UserResponse> {
    User.find(req.parameters.get("userID"), on: req.db)
        .unwrap(or: Abort(.notFound))
        .flatMapThrowing { try $0.convertToResponse(on: req.db) }
        .flatMap { $0 }
}

// the function declaration of `convertToResponse`:
func convertToResponse(on db: Database) throws -> EventLoopFuture<UserResponse>

Note that the reason for convertToResponse returning an EventLoopFuture is because sometimes I need to make asynchronous database calls to get their relations if I want to include them on some responses, e.g. the username field of the author relation on an object of type Comment.

This code is not only adding boilerplate code (two calls instead of one), but is also very confusing (why do I need to call flatMap if I just called flatMap already?). I need to write a helper function now at the very beginning of my project and I plan to actually add this to a library which I will be reusing on every subsequent Vapor project as I expect to run into this problem with every single NIO based project.

So, what do you think about doing a proper renaming here and also rethinking if we shouldn't actually introduce a throwing flatMap? I personally have implemented the following helper types which appear to be most clear to me, feel free to make other suggestions though:

extension EventLoopFuture {
    public func mapThrowing<NewValue>(_ callback: @escaping (Value) throws -> NewValue) -> EventLoopFuture<NewValue> {
        flatMap { value in
            do {
                return self.eventLoop.makeSucceededFuture(try callback(value))
            } catch {
                return self.eventLoop.makeFailedFuture(error)
            }
        }
    }

    public func flatMapThrowing<NewValue>(_ callback: @escaping (Value) throws -> EventLoopFuture<NewValue>) -> EventLoopFuture<NewValue> {
        mapThrowing(callback).flatMap { $0 }
    }
}
3 Likes
2

100% agree, almost every time i used nio futures so far i've gotten confused by this and missed the actual flatMapThrowing.

I have to say this is, for me, the exact reason to use a flatMap that can throw, usually when I use a throwing get method on optional, or validate some other input prior to starting a request that can then fail later on it's own time.

3

Before going any further, it'd be really nice if you could tone down the hyperbole a couple of notches. I understand you're frustrated, but this conversation will go a lot better if we can try to remove some of the heat from your frustration. In this case, it's usually a good idea to consider that not everyone has your background or your approach to programming. In this case it seems that there is an existence proof of at least a few people who feel differently to you, given that the API got named the way it did!

Sure, there's no disagreement on this point. We can rename it. What we cannot do is remove the old name, or use flatMapThrowing for any other purpose. There's a huge amount of working code that uses that name and it must persist for the foreseeable future, at least until we do a NIO 3.

It is also clear that Johannes objects to the name mapThrowing. Given that adding that function would also risk introducing ambiguous call site errors, that's probably a name we can't use anyway. So we'll have to find something else.

There's substantially stronger NIO feeling on this topic. We should not introduce a throwing flatMap. I appreciate that you're bumping into issues where it would be helpful, but we should again re-iterate why this function does not exist. Let's use your example:

func convertToResponse(on db: Database) throws -> EventLoopFuture<UserResponse>

This function is very confused, because it reports errors in two places. To confirm whether this function behaved correctly or not you have to check not one but two error reporting paths.

convertToResponse should be rewritten to not throw. This is easily done: take whatever its body is now, and wrap the throwing statements with ones that return failed EventLoopFutures.

As a general rule of API design, if you must sometimes return Futures you should always return Futures. This forces a symmetry in your call sites and makes it much clearer how control is supposed to flow through the code.

5 Likes
4

So you want me to write lots of do-catch statements everywhere where Vapor for some reasons makes the code throwing for the general purpose but in my cases I never expect it to throw? Like you want me to rewrite this code:

extension User {
    func convertToResponse(on db: Database) throws -> EventLoopFuture<UserResponse> {
        db.eventLoop.makeSucceededFuture(
            UserResponse(
                id: try requireID(),
                countryCode: countryCode,
                subTerritoryCode: subTerritoryCode
            )
        )
    }
}

And make it look more like that code?

extension User {
    func convertToResponse(on db: Database) -> EventLoopFuture<UserResponse> {
        do {
            let userResponse = UserResponse(
                id: try requireID(),
                countryCode: countryCode,
                subTerritoryCode: subTerritoryCode
            )
            return db.eventLoop.makeSucceededFuture(userResponse)
        } catch {
            return db.eventLoop.makeFailedFuture(error)
        }
    }
}

For me, dropping the do-catch in this situation was a conscious decision much like I sometimes decide to actually use force unwrapping (!) on an Optional because I know for a reason that in this given situation, the Optional should never be nil and if it is, I actually want the app to crash so I'm made aware of a serious problem which needs fixing immediately. Similarly, I don't expect the try requireID() call here to ever throw an error and if it did, I'd see this as a serious issue. My code (without the do-catch) expresses this to me by making the entire function throwing and explicitly not putting the try into a do.

But what you are basically suggesting is that for every single place, I should always provide the do-catch because of some design choices you made in NIO and you are not wanting me (or others) to use the features "throwing functions" and "Result-failing functions" as distinct things with different use cases or meanings. You want us to always use "Result-failing functions" instead. Why is that? I don't get how using the non-throwing do-catch variant above make my code more clear. It looks to me like I actually expect try requireID() to fail and I see no way of communicating to the reader of that code that it actually should never fail here.

1 Like
5

That exact code? No, I want you to rewrite it to remove the returned Future. Code that unconditionally returns a completed Future in all paths should be rewritten to be synchronous. Then you can happily use try all you want.

That's fine, you can use try!, which is the exact analogy of operator !.

No, that is the opposite of what I said. I said:

My proposal is that all functions should use one error reporting path. That path is either throws or -> EventLoopFuture. I don't mind which you use, but you should pick one for each function you write, based on whether it will actually need to return a Future or not.

try!

1 Like
6

There needs to be a clear distinction here between what NIO what designed for and what NIO is being used for.

NIO is a low-level library designed to write nice, clear, semantically correct APIs for users to use. Having a function that can both return a failed future and throw is confusing because as a user you have to check for the error in both cases. It's like a function that both returns a Result and throws.

The main issue here is how Vapor uses NIO and builds directly on top of it. There's no disagreement that end users may end up writing code that needs a throwing version of flatMap but it's not NIO's responsibility to provide that - the onus should be on Vapor to provide it. It's come up several times before and would be fairly easy to add to AsyncKit, it's just waiting on someone to do the PR essentially.

Ideally Vapor would provide a wrapper around EventLoopFuture and all it's associated values. This hasn't happened for a couple of reasons - first the goal of Vapor 4 was to provide as close an integration to NIO as possible to allow it to make full use of the ecosystem and make it easy to switch between NIO packages and Vapor packages. Second, the compiler has issues disambiguating between various cases and especially closures with the same signature with the only difference between one that can throw and one that can't. We tried hard to get a .then function in that would accepted either a future or non future (flatMap and map) and throw or not. However, it wasn't possible with the compiler at the time so got parked.

Of course the ideal solution is for Swift to get a first class concurrency model and then a lot of these issues would go away. Thankfully we're starting to see the building blocks for that emerge in the language

3 Likes
7

As for the naming of those two functions, I've already expressed that I'm open for other name suggestions, including the rationale behind combineThrowing. I do not have a strong opinion on mapThrowing and flatMapThrowing and can totally understand if you don't want to use those names for preventing confusion.

But I did want to express two things:

  1. For me, the names mapThrowing and flatMapThrowing are the most clear names.
  2. The argument mapThrowing doesn't look like it can fail is simply ridiculous to me. I can understand other arguments against it, but not that one. And please let me have the freedom to express that I find something ridiculous if I do so. I didn't mean anything personally here, I was just trying to make my point, sorry if someone took this personally.
8

Okay, you got me there, I will use try! then, didn't think of that to be honest. :sweat_smile:

But the reason why I'm returning a future there is because I'm using a Vapor feature that forces me to return a future as I can't call wait() here which would block the thread. So @0xTim might be right that the actual issue here is how Vapor is using NIO and not NIO itself. If that's the case, we should probably move this to the Vapor section, but probably it still makes sense to rename flatMapThrowing in NIO?

9

I appreciate that, and I'm not (at this time) disputing it. What I'm trying to do is to set expectations. Those names already exist and have function signatures in NIO. To use those names for something else requires us to ship a Semver major update. We will do this, but we aren't going to do it anytime soon, so regardless of whether those names are clearest we do need to find alternatives that we can safely introduce. This is not a matter of confusion but of compatibility.

Talking specifically about the code you used here, where is that feature? The code you wrote above doesn't have any Futures in it at all except the one being returned by you. I'd expect to see a callback chain somewhere.

10

Here's an example from the Project model which has a to-many relation to Task:

func convertToResponse(on db: Database) throws -> EventLoopFuture<ProjectResponse> {
    $tasks.query(on: db).count().mapThrowing {
        ProjectResponse(
            id: try self.requireID(),
            createdAt: try self.requireCreatedAt(),
            updatedAt: try self.requireUpdatedAt(),
            deletedAt: self.deletedAt,
            name: self.name,
            tasksCount: $0
        )
    }
}
11

This function needn't be marked throws: nothing in it throws. You could remove the throws annotation from it trivially. Everything that throws is inside mapThrowing, meaning those errors will be reported as failed futures.

12

That's because I'm using a protocol all my models comply to like this:

import Fluent
import Vapor

protocol ClientServerContent: Model {
    associatedtype ResponseType: ServerResponse
    associatedtype RequestType: Content

    func updateContents(request: RequestType) throws
    func convertToResponse(on db: Database) throws -> EventLoopFuture<ResponseType>
}

If one of my models needs convertToResponse to be throwing, then I need to mark all of them as such. But I will review my code with the try! suggestion you've made earlier to check if it's still needed once I have some time (I develop the project in my free time).

I brought up that example to show the need for the EventLoopFuture, not the throws.

13

I agree with the design principle that a function shouldn't throw and return a future. On the other hand I also find myself reaching out to something like combineThrowing in Vapor, pretty much along the lines of the examples @Jeehut provided. This thread gave me some ideas on how to avoid reaching out to it but the fact that many keep bumping into this is a symptom of a problem that should be eventually addressed either by each individual library or NIO itself. I understand that the latter would be much harder now that NIO is used in production code.

14

Also keep in mind that the futures might go away from user level code once we get async and friends.

3 Likes
15

FWIW, Combine seems to call them tryMap.

1 Like