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

Question abount flatMapThrowing #1559

Closed
t-ae opened this issue Jun 18, 2020 · 6 comments
Closed

Question abount flatMapThrowing #1559

t-ae opened this issue Jun 18, 2020 · 6 comments
Labels
question Further information is requested

Comments

@t-ae
Copy link

t-ae commented Jun 18, 2020

ELF has map/flatMap/flatMapThrowing.
I wonder:

  1. why flatMapThrowing behaves like map rather than flatMap?
  2. why there's no throwing flatMap?

There are some people confused like me.

flatMapThrowing is introduced in #760.
The author @weissi said:

weissi on 21 Jan 2019 Author Member

@Lukasa so thenThrowing is badly named and there's nothing in Result that does the same thing. I wanted to rename thenThrowing in a separate PR so we can bike shed the name with the community. I will open as soon as this is merged, is that okaY?

weissi on 21 Jan 2019 Author Member

I thought I replied to this but apparently I haven't: I want to do this in a separate PR because there's no prior art for thenThrowing and the name is bad. So I want a PR to just do this and bike shed the name with the community (after this has been merged).

#760 (comment)

But he renamed it in that PR and I counldn't find further discussion.
I'd like to know how current design was decided on.

@weissi
Copy link
Member

weissi commented Jun 18, 2020

  • why flatMapThrowing behaves like map rather than flatMap?

It depends on the view point. 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.

maps usually stays 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 of you throw, we'll go from the success path into the failure path).

Lots of people get confused by this, if I could choose again, I'd probably go for something like combineThrowing or so?

  1. why there's no throwing flatMap?

Functions that have this prototype: f() throws -> EventLoopFuture<Void> are in my view a bit user hostile. They can fail in two different ways:

  1. by throwing
  2. by returning a failed future

That means a user needs to look for errors in two different places. It's usually hard enough to get error handling right if errors happen in only one way.

Therefore, I don't think such functions should exist which is why NIO doesn't offer a flatMap which handles that.
On top of that: If an asynchronous function throws synchronously, is it really asynchronous?

I'm pretty sure you'd also be confused seeing a function that looks like:

func readFile() throws -> Result<String, Error>

There's very little reason to do so: Usually you want to use either throws OR Result as your failure handling mechanism. The same applies to futures.

If you have a specialised application where that would make sense, feel free to add your own function flatMapMaybeThrowing or so.

@weissi weissi added the question Further information is requested label Jun 18, 2020
@t-ae
Copy link
Author

t-ae commented Jun 18, 2020

Functions that have this prototype: f() throws -> EventLoopFuture are in my view a bit user hostile.

I was thinking very same thing when trying Vapor 3 (There were tons of that). But that helped making code simpler, so I decided to use it.
Now, migrating to Vapor 4, I have to rewrite all of them😂

If you have a specialised application where that would make sense, feel free to add your own function flatMapMaybeThrowing or so.

I have some code like below:

return fetchDB().flatMapMaybeThrowing { row -> EventLoopFuture<Void> in
    let result = try handleFileSynchronously(row)
    return row.updateDB(with: result)
}

Synchronous func appears in asynchronous context.
AFAIK current API cannot handle such case as simple as this(Using flatMap with do-catch or tryFuture?).

@weissi
Copy link
Member

weissi commented Jun 18, 2020

Functions that have this prototype: f() throws -> EventLoopFuture are in my view a bit user hostile.

I was thinking very same thing when trying Vapor 3 (There were tons of that). But that helped making code simpler, so I decided to use it.
Now, migrating to Vapor 4, I have to rewrite all of them😂

If you have a specialised application where that would make sense, feel free to add your own function flatMapMaybeThrowing or so.

I have some code like below:

return fetchDB().flatMapMaybeThrowing { row -> EventLoopFuture<Void> in
    let result = try handleFileSynchronously(row)
    return row.updateDB(with: result)
}

Synchronous func appears in asynchronous context.
AFAIK current API cannot handle such case as simple as this(Using flatMap with do-catch or tryFuture?).

You could add your own version thenThrowing that does that and deprecate it. Then your code will work but you'll get warnings and you can fix them at the end.

If you have a specialised application where that would make sense, feel free to add your own function flatMapMaybeThrowing or so.

I have some code like below:

return fetchDB().flatMapMaybeThrowing { row -> EventLoopFuture<Void> in
    let result = try handleFileSynchronously(row)
    return row.updateDB(with: result)
}

Synchronous func appears in asynchronous context.
AFAIK current API cannot handle such case as simple as this(Using flatMap with do-catch or tryFuture?).

This should do it

extension EventLoopFuture {
    func flatMapThrowingMaybe<NewValue>(_ body: @escaping (Value) throws -> EventLoopFuture<NewValue>) -> EventLoopFuture<NewValue> {
        return self.flatMapThrowing { (value: Value) throws -> EventLoopFuture<NewValue> in
            try body(value) // This synchronously returns a future or fails
        }.flatMap {
            $0 // this now continues with the result of the future (unless we failed above)
        }
    }
}

As you can see, this is essentially a two step process:

  1. run the function synchronously just to see if it throws. It returns us a future
  2. combine the future with "our" future

@t-ae
Copy link
Author

t-ae commented Jun 18, 2020

You could add your own version thenThrowing that does that and deprecate it. Then your code will work but you'll get warnings and you can fix them at the end.

Unfortunately I was usingflatMap of Vapor 3.

This should do it

I know the way to define flatMapThrowingMaybe.
I meant to say synchronous func in asynchronous context is common case, so flatMapThrowingMaybe is worth having in API.

@weissi
Copy link
Member

weissi commented Jun 18, 2020

You could add your own version thenThrowing that does that and deprecate it. Then your code will work but you'll get warnings and you can fix them at the end.

Unfortunately I was usingflatMap of Vapor 3.

This should do it

I know the way to define flatMapThrowingMaybe.
I meant to say synchronous func in asynchronous context is common case, so flatMapThrowingMaybe is worth having in API.

Well, if your function is fully synchronous (does not return a future), then flatMapThrowing is your function. If your function is asynchronous, it shouldn't throw (it should return eventLoop.makeFailedFuture(error) if something goes wrong).

If you have a throws -> EventLoopFuture<Void> kind of function, then I think you should fix that function to choose whether it wants to be synchronous or asynchronous. I don't think we want to add functionality for discouraged behaviour like that.

@t-ae
Copy link
Author

t-ae commented Jun 19, 2020

I'll rewrite all my throws -> EventLoopFuture<Void>s at first. Perhaps I can get some enlightenment from that.
Thank you @weissi.

@t-ae t-ae closed this as completed Jun 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants