Skip to content
This repository has been archived by the owner on Jun 27, 2023. It is now read-only.

Is Future.map(on: worker) intentionally synchronous? #167

Open
bmhatfield opened this issue Jul 30, 2018 · 6 comments
Open

Is Future.map(on: worker) intentionally synchronous? #167

bmhatfield opened this issue Jul 30, 2018 · 6 comments

Comments

@bmhatfield
Copy link

When working with Vapor, I recently was surprised to learn that Future.map was a synchronous method that returns a future. I had been under the mistaken impression that it executed the closure on the event loop.

Compare: https://github.com/vapor/core/blob/master/Sources/Async/Future%2BGlobal.swift#L11-L21

with .submit: https://github.com/apple/swift-nio/blob/ae13d135f79e7b60281f9bf4c1cf1b3341a7339f/Sources/NIO/EventLoop.swift#L268-L280

Just checking that this wasn't an oversight, because the two methods appear to be very close in implementation.

@bmhatfield
Copy link
Author

PS: I'm happy to put up a PR to execute the promise on the event loop, but I wasn't sure what the intended behavior here was, hence this discussion issue first. If there's a better venue for this question, I am happy to redirect!

@MrMage
Copy link
Contributor

MrMage commented Jul 31, 2018 via email

@vzsg
Copy link
Member

vzsg commented Jul 31, 2018

@MrMage There's no previous Future here, the static Future.map method creates a new Future from the result of a synchronous function call.

The question is valid. The only difference between the NIO and the Async functions is which thread the synchronous function is executed on: as noted, Async's map runs it on the caller thread (and returns a Future which is resolved on the specified event loop); NIO's submit pushes the invocation to the event loop thread as well.

@bmhatfield
Copy link
Author

bmhatfield commented Jul 31, 2018

@MrMage: @vzsg has understood it right - I am asking about the static Future.map function, which I was using to create (simple) new futures until I learned that it doesn't actually execute that closure on the eventloop.

It's of course possible to use Promises directly to create new futures, but I found the static Future.map to be very convenient.

As mentioned - if it's an oversight, the PR here is very straightforward, should be backwards-compatible, and I am happy to make it.

@MrMage
Copy link
Contributor

MrMage commented Aug 1, 2018

Sorry for jumping in, I should have looked at the code 😅 Didn't mean to detract, just wanted to be helpful :-)

@tanner0101
Copy link
Member

@bmhatfield Future.map is meant as an easy way to wrap synchronous, throwing code into a Future. It's a tool to combat methods that return futures and throw. It is expected to be called when you are already on the event loop. Generally you should never not be on the event loop unless you are needing to interact with blocking code. In those special cases, you should create your own promises and background threads.

That said I don't think there would be a huge detriment to putting these calls onto the event loop. The only thing I would say is that we should only call EventLoop.submit if we are not already on the event loop. NIO has a static call for checking this on the EventLoop type.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants