Add EventLoopFuture fold function #269

karim-elngr · 2018-04-03T13:27:10Z

Motivation:

#184 Add primitive for folding futuristic values.

Modifications:

Add a new EventLoopFuture extension containing the implementation of
fold<U>(futures:combiningFunction).

Result:

An API for folding futures.

swift-nio-bot · 2018-04-03T13:27:12Z

Can one of the admins verify this patch?

Lukasa · 2018-04-03T13:53:02Z

Sources/NIO/EventLoopFuture.swift

 }

+extension EventLoopFuture {
+    /// Returns a new `EventLoopFuture` that fires only when this `EventLoopFuture` and all the provided futures complete.


This description doesn't really adequately capture what this method is for. It's not wrong at all, it just misses what this method is actually for. Mind rewording it?

Lukasa · 2018-04-03T14:01:01Z

Sources/NIO/EventLoopFuture.swift

+    ///     - eventLoop: The `EventLoop` on which the new `EventLoopFuture` callbacks will fire.
+    ///     - with: The combining function used to produce partial results.
+    /// - returns: A new `EventLoopFuture`.
+    public func fold<U>(_ futures: [EventLoopFuture<U>], eventLoop: EventLoop, with combiningFunction: @escaping (EventLoopFuture<T>, EventLoopFuture<U>) -> EventLoopFuture<T>) -> EventLoopFuture<T> {


Hrm, is this exactly what we want this method to be? This method is not far from being a trivial wrapper over reduce, and there's a limited amount of utility here. I appreciate we may have been talking past each other here, but I was thinking we'd have something a bit closer to the Haskell construct, where the signature of the combiningFunction is (T, U) -> EventLoopFuture<T>.

I have no particular objection to the function as written, but I suspect the construct is of limited utility (you already have the array of futures, you can already call reduce on them, all fold does is wrap that into a new promise you just allocated).

Does that make sense?

Ah I totally agree now that I think about it, I think I missed that point (T, U) -> EventLoopFuture<T>.
I will modify this to reflect what we want it to be.

👍 Thanks

karim-elngr · 2018-04-03T16:35:49Z

@Lukasa Ok I updated the changes

Lukasa

one minor note, but so far this looks great.

Lukasa · 2018-04-03T16:44:22Z

Sources/NIO/EventLoopFuture.swift

+    ///
+    /// The returned `EventLoopFuture` will fail with the first error encountered, as soon as this
+    /// `EventLoopFuture` or any of the provided futures fail: otherwise, it will succeed only when all
+    /// of them do.


Ok, so let's address this comment to clarify exactly what will happen here. We should reword to:

The returned EventLoopFuture will fail as soon as the a failure is encountered in any of the futures (or in this one). However, the failure will not occur until all preceding EventLoopFutures have completed. At the point the failure is encountered, all subsequent EventLoopFuture objects will no longer be waited for. This function therefore fails fast: once a failure is encountered, it will immediately fail the overall EventLoopFuture.

karim-elngr · 2018-04-03T17:22:35Z

Sources/NIO/EventLoopFuture.swift

+                return combiningFunction(f1Value, f2Value)
+            })
+        }
+        return body.hopTo(eventLoop: eventLoop)


@Lukasa do we need to hopTo the specified eventLoop, I am feeling like this could be removed as the combiningFunction should take care of that, right?! and will ensure that the final EventLoopFuture is hopTo the eventLoop of self. And combiningFunction will allow the caller to hopTo a different one if desired.

The combining function will not take care of that, which is actually a really good point.

I think all the combiningFunction invocations should occur on the same event loop, and that event loop should be the one that self belongs to. This means we actually need a hopTo on the future returned by the combiningFunction to make it continue to execute on self's event loop.

weissi · 2018-04-04T10:01:13Z

Sources/NIO/EventLoopFuture.swift

+    ///     - eventLoop: The `EventLoop` on which the new `EventLoopFuture` callbacks will fire.
+    ///     - with: A function that will be used to fold the values of two `EventLoopFuture`s and return a new value wrapped in an `EventLoopFuture`.
+    /// - returns: A new `EventLoopFuture`.
+    public func fold<U>(_ futures: [EventLoopFuture<U>], eventLoop: EventLoop, with combiningFunction: @escaping (T, U) -> EventLoopFuture<T>) -> EventLoopFuture<T> {


instead of the eventLoop parameter, you can just use self.eventLoop

karim-elngr · 2018-04-04T13:13:16Z

Sources/NIO/EventLoopFuture.swift

+            let newFuture = f1.and(f2).then({ (args: (T, U)) -> EventLoopFuture<T> in
+                let (f1Value, f2Value) = args
+                assert(self.eventLoop.inEventLoop)
+                return combiningFunction(f1Value, f2Value)


@Lukasa Do we really need a hopTo before we return the EventLoopFuture from the combiningFunction? I think then accepts a callback whose returned future runs on any eventLoop and executes this callback on its caller's eventLoop and always returns a future on its caller's eventLoop (the result of and) which already (because of and) hopTo its caller's eventLoop, so if we start the reduce with self, it will always return a future whose callbacks run on self.eventLoop and always executes the callbacks on self.eventLoop.

Ah, you're right, the EventLoopFuture returned from then does not need to be hopped: it is tied to the loop of f1. The assert is sufficient.

Want to remove the parentheses from the then call? They shouldn't be needed.

karim-elngr · 2018-04-04T15:45:52Z

@Lukasa Ok any further notes?

Lukasa

LGTM, @weissi mind reviewing as well?

weissi

looks pretty good already, small changes needed though

weissi · 2018-04-04T16:47:15Z

Tests/NIOTests/EventLoopFutureTest.swift

        XCTAssertTrue(f.isFulfilled)
    }
+
+    func testFoldWithSuccessAndAllSuccesses() throws {


could we have a test case that folds EventLoopFutures from different actual event loops (not EmbeddedEventLoops)?

Yes sure let me add one

weissi · 2018-04-04T16:48:16Z

Sources/NIO/EventLoopFuture.swift

+        let body = futures.reduce(self) { (f1: EventLoopFuture<T>, f2: EventLoopFuture<U>) -> EventLoopFuture<T> in
+            let newFuture = f1.and(f2).then { (args: (T, U)) -> EventLoopFuture<T> in
+                let (f1Value, f2Value) = args
+                assert(self.eventLoop.inEventLoop)


but what if we're dealing with different event loops?

@weissi It doesn't matter. and's then will execute on f1's event loop, and the first f1 is self, and then the future returned from combiningFunction will be moved over to the f1 loop (because that's how then works.

ah yes, that makes sense. Thank you!

karim-elngr · 2018-04-04T17:58:45Z

Tests/NIOTests/EventLoopFutureTest.swift

        XCTAssertTrue(f.isFulfilled)
    }
+
+    func testFoldWithMultipleEventLoops() throws {


@weissi Does this test suffice for multiple eventLoops?

yes, thanks!

Motivation: Add primitive for folding futuristic values. Modifications: Add a new EventLoopFuture extension containing the implementation of `fold<U>(futures:combiningFunction)`. Result: An API for foding futures.

weissi

thanks very much @Lukasa and @karim-elngr , looks great!

weissi · 2018-04-04T18:17:43Z

Sources/NIO/EventLoopFuture.swift

+        let body = futures.reduce(self) { (f1: EventLoopFuture<T>, f2: EventLoopFuture<U>) -> EventLoopFuture<T> in
+            let newFuture = f1.and(f2).then { (args: (T, U)) -> EventLoopFuture<T> in
+                let (f1Value, f2Value) = args
+                assert(self.eventLoop.inEventLoop)


ah yes, that makes sense. Thank you!

weissi · 2018-04-04T18:18:44Z

@swift-nio-bot test this please

karim-elngr · 2018-04-05T12:44:41Z

@weissi and @Lukasa, thanks guys for all the help! Time to celebrate my first merged PR!! 😄

Lukasa requested changes Apr 3, 2018

View reviewed changes

karim-elngr commented Apr 3, 2018

View reviewed changes

weissi reviewed Apr 4, 2018

View reviewed changes

karim-elngr commented Apr 4, 2018

View reviewed changes

Lukasa approved these changes Apr 4, 2018

View reviewed changes

weissi requested changes Apr 4, 2018

View reviewed changes

karim-elngr commented Apr 4, 2018

View reviewed changes

Add EventLoopFuture fold function

95b0bea

Motivation: Add primitive for folding futuristic values. Modifications: Add a new EventLoopFuture extension containing the implementation of `fold<U>(futures:combiningFunction)`. Result: An API for foding futures.

weissi approved these changes Apr 4, 2018

View reviewed changes

Merge branch 'master' into thenreduce

20c6d39

Lukasa added the 🆕 semver/minor Adds new public API. label Apr 5, 2018

Lukasa added this to the 1.4.0 milestone Apr 5, 2018

Lukasa merged commit d58fa23 into apple:master Apr 5, 2018

karim-elngr deleted the thenreduce branch April 5, 2018 12:38

Add EventLoopFuture fold function #269

Add EventLoopFuture fold function #269

Uh oh!

Conversation

karim-elngr commented Apr 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swift-nio-bot commented Apr 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karim-elngr commented Apr 3, 2018

Uh oh!

Lukasa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karim-elngr Apr 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karim-elngr commented Apr 4, 2018

Uh oh!

Lukasa left a comment

Choose a reason for hiding this comment

Uh oh!

weissi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weissi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weissi commented Apr 4, 2018

Uh oh!

karim-elngr commented Apr 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

karim-elngr commented Apr 3, 2018 •

edited

Loading

karim-elngr Apr 4, 2018 •

edited

Loading