addition of assertSuccess() and assertFailure() on EventLoopFuture #2417

dkz2 · 2023-05-03T05:48:39Z

Added assertSuccess, assertFailure, preconditionSuccess and preconditionFailure methods to EventLoopFuture for safer and more explicit error handling.

Motivation:

Enhance developer experience by providing a clear way to handle expected success or failure of futures.

Closes #2359.

Modifications:

Implemented assertSuccess, assertFailure, preconditionSuccess and preconditionFailure for EventLoopFuture.
Added corresponding test cases to verify behavior of the new methods.

Result:

Developers will now have access to assertSuccess, assertFailure, preconditionSuccess and preconditionFailure methods onEventLoopFuture.

Lukasa

Thanks for this! I am inclined to want to say that these methods do slightly too much. Having the callback as an argument muddys the water about their actual semantic. If these methods instead behave like a regular future function (where they return a new future) then we can allow them to compose with the rest of the future handling functions, which might be nicer.

@weissi I'm also curious whether you think assertion is sufficient, or whether we need precondition variants too.

Sources/NIOCore/EventLoopFuture.swift

weissi · 2023-05-03T10:52:28Z

Thanks for this! I am inclined to want to say that these methods do slightly too much. Having the callback as an argument muddys the water about their actual semantic. If these methods instead behave like a regular future function (where they return a new future) then we can allow them to compose with the rest of the future handling functions, which might be nicer.

Agreed!

@weissi I'm also curious whether you think assertion is sufficient, or whether we need precondition variants too.

I'd think we should have both precondition* and assert* variants

…lure

dkz2 · 2023-05-03T15:34:53Z

Thanks for this! I am inclined to want to say that these methods do slightly too much. Having the callback as an argument muddys the water about their actual semantic. If these methods instead behave like a regular future function (where they return a new future) then we can allow them to compose with the rest of the future handling functions, which might be nicer.

Agreed!

@weissi I'm also curious whether you think assertion is sufficient, or whether we need precondition variants too.

I'd think we should have both precondition* and assert* variants

Added precondition* variants, also removed all unneeded requirements.

Lukasa · 2023-05-03T16:22:13Z

Sources/NIOCore/EventLoopFuture.swift

+            case .success(let value):
+                promise.succeed(value)
+            case .failure(let error):
+                assertionFailure("Expected success, but got failure: \(error)")


Can we pass file and line through from the line that calls assertSuccess? That'll make the crash more generally useful.

oh sorry, you already commented with that. Please ignore my extra SPAM 🙈

Lukasa · 2023-05-03T16:22:24Z

Sources/NIOCore/EventLoopFuture.swift

+        self.whenComplete { result in
+            switch result {
+            case .success(let value):
+                assertionFailure("Expected failure, but got success: \(value)")


Same note here on file and line.

Lukasa · 2023-05-03T16:22:41Z

Sources/NIOCore/EventLoopFuture.swift

+    ///
+    /// If the original future fails, it triggers a precondition failure, causing a runtime error during development.
+    @inlinable
+    public func ensureSuccess() -> EventLoopFuture<Value> {


I think using precondition instead of ensure on these two methods is better, as it fits with our existing prior art.

I ran into an issue where preconditionFailure as the method name conflicts with the global preconditionFailure function in Swift, i could rename it to be something like preconditionOnSuccess potentially?

You can disambiguate by using Swift.preconditionFailure(...) to refer to the global one.

Sounds good making the changes!

Lukasa · 2023-05-03T16:22:51Z

Sources/NIOCore/EventLoopFuture.swift

+            case .success(let value):
+                promise.succeed(value)
+            case .failure(let error):
+                preconditionFailure("Expected success, but got failure: \(error)")


Can we pass file and line through?

yes to all!

Lukasa · 2023-05-03T16:23:00Z

Sources/NIOCore/EventLoopFuture.swift

+        self.whenComplete { result in
+            switch result {
+            case .success(let value):
+                preconditionFailure("Expected failure, but got success: \(value)")


Can we pass file and line through?

glbrntt · 2023-05-04T07:03:04Z

Sources/NIOCore/EventLoopFuture.swift

+    /// If the original future fails, it triggers an assertion failure, causing a runtime error during development.
+    @inlinable
+    public func assertSuccess() -> EventLoopFuture<Value> {
+        let promise = self.eventLoop.makePromise(of: Value.self)


No need to create a promise in any of these functions; you can use always instead:

return self.always { result in switch result { case .success: () case .failure(let error): assertionFailure("Expected success, but got failure: \(error)") } }

Indeed, but we should pass file and line to assertionFailure such that it shows where the user put .assertX and not always this line

oh okay i see no need to create a new promise / or complete it bc always can take care of that for us, working on the changes. Thank you!

changes commited!

glbrntt · 2023-05-04T07:07:32Z

Sources/NIOCore/EventLoopFuture.swift

+    ///
+    /// If the original future fails, it triggers a precondition failure, causing a runtime error during development.
+    @inlinable
+    public func ensureSuccess() -> EventLoopFuture<Value> {


You can disambiguate by using Swift.preconditionFailure(...) to refer to the global one.

weissi · 2023-05-04T11:11:29Z

Sources/NIOCore/EventLoopFuture.swift

+    ///
+    /// If the original future fails, it triggers an assertion failure, causing a runtime error during development.
+    @inlinable
+    public func assertSuccess() -> EventLoopFuture<Value> {


here and in the other functions we should take file: ... = #file, line = ... #line and pass that to the (assertion/precondition)Failure functions

please use #fileID and not #file

Done to all!

… line for debug

…tps://github.com/dkz2/swift-nio into dkz2-EventLoopFuture-assertSuccess-assertFailure

…lure

Lukasa

Nice one, this LGTM.

Lukasa · 2023-05-04T15:14:47Z

@swift-server-bot test this please

dnadoba

Thanks for taking care of this!

dkz2 · 2023-05-04T15:45:27Z

Thank you all for the help and guidance @weissi @Lukasa @glbrntt @dnadoba, very much appreciated!

dkz2 added 2 commits May 3, 2023 00:23

added assertSuccess() and assertFailure()

382a99b

test functions for assertSuccess() and assertFailure()

ac4ea51

Lukasa added the semver/minor Adds new public API. label May 3, 2023

Lukasa requested changes May 3, 2023

View reviewed changes

dnadoba reviewed May 3, 2023

View reviewed changes

Sources/NIOCore/EventLoopFuture.swift Outdated Show resolved Hide resolved

dkz2 force-pushed the dkz2-EventLoopFuture-assertSuccess-assertFailure branch 2 times, most recently from 37fe5c5 to ac4ea51 Compare May 3, 2023 15:06

dkz2 and others added 3 commits May 3, 2023 10:14

removed callbacks approach and add precondition variants

c03d9bf

added test for precondition variants

6106e2c

Merge branch 'main' into dkz2-EventLoopFuture-assertSuccess-assertFai…

548dd1d

…lure

Lukasa reviewed May 3, 2023

View reviewed changes

glbrntt reviewed May 4, 2023

View reviewed changes

weissi reviewed May 4, 2023

View reviewed changes

dkz2 and others added 4 commits May 4, 2023 08:58

self.always to handle future w/o creating a promise, added fileID and…

4bd88eb

… line for debug

Merge branch 'dkz2-EventLoopFuture-assertSuccess-assertFailure' of ht…

5484bf0

…tps://github.com/dkz2/swift-nio into dkz2-EventLoopFuture-assertSuccess-assertFailure

updated test for asserts on EventLoopFuture

42e98e1

Merge branch 'main' into dkz2-EventLoopFuture-assertSuccess-assertFai…

7d99aa2

…lure

Lukasa approved these changes May 4, 2023

View reviewed changes

dnadoba self-requested a review May 4, 2023 15:21

dnadoba approved these changes May 4, 2023

View reviewed changes

Lukasa merged commit 7f4d10b into apple:main May 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

addition of assertSuccess() and assertFailure() on EventLoopFuture #2417

addition of assertSuccess() and assertFailure() on EventLoopFuture #2417

dkz2 commented May 3, 2023 •

edited

Loading

Lukasa left a comment

weissi commented May 3, 2023

dkz2 commented May 3, 2023

Lukasa May 3, 2023

weissi May 4, 2023

Lukasa May 3, 2023

Lukasa May 3, 2023

dkz2 May 3, 2023

glbrntt May 4, 2023

dkz2 May 4, 2023

Lukasa May 3, 2023

dkz2 May 4, 2023

Lukasa May 3, 2023

glbrntt May 4, 2023

weissi May 4, 2023

dkz2 May 4, 2023

dkz2 May 4, 2023

glbrntt May 4, 2023

weissi May 4, 2023

dnadoba May 4, 2023

dkz2 May 4, 2023

Lukasa left a comment

Lukasa commented May 4, 2023

dnadoba left a comment

dkz2 commented May 4, 2023

addition of assertSuccess() and assertFailure() on EventLoopFuture #2417

addition of assertSuccess() and assertFailure() on EventLoopFuture #2417

Conversation

dkz2 commented May 3, 2023 • edited Loading

Motivation:

Modifications:

Result:

Lukasa left a comment

Choose a reason for hiding this comment

weissi commented May 3, 2023

dkz2 commented May 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lukasa left a comment

Choose a reason for hiding this comment

Lukasa commented May 4, 2023

dnadoba left a comment

Choose a reason for hiding this comment

dkz2 commented May 4, 2023

dkz2 commented May 3, 2023 •

edited

Loading