Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

futures should have an always or something #665

Closed
weissi opened this issue Nov 27, 2018 · 20 comments
Closed

futures should have an always or something #665

weissi opened this issue Nov 27, 2018 · 20 comments
Labels
kind/enhancement Improvements to existing feature.

Comments

@weissi
Copy link
Member

weissi commented Nov 27, 2018

often you need to do some cleanup that needs to always happen, why not

extension EventLoopFuture {
    func always(_ body: () -> Void) -> EventLoopFuture<T>
}

that always runs?

@weissi weissi added the kind/enhancement Improvements to existing feature. label Nov 27, 2018
@Lukasa
Copy link
Contributor

Lukasa commented Nov 27, 2018

Why does always return a T?

@weissi
Copy link
Member Author

weissi commented Nov 27, 2018

because it's too early in the morning 😂. fixing it :P

@normanmaurer
Copy link
Member

@weissi so this is something that will be executed no matter if the future is completed because of a failure or success ?

@Lukasa
Copy link
Contributor

Lukasa commented Nov 27, 2018

@weissi Is this any different to:

public func whenComplete(_ callback: @escaping () -> Void)

Because we already have that.

@Lukasa
Copy link
Contributor

Lukasa commented Nov 27, 2018

Oh, I see, it chains. Sure, why not.

@normanmaurer
Copy link
Member

If we do this then we should most likely just mark whenComplete as deprecated and introduce always but mark its return value as @discardableResult.

@weissi
Copy link
Member Author

weissi commented Nov 27, 2018

@normanmaurer need both chaining and non-chaining variants

@normanmaurer
Copy link
Member

@weissi why ?

@weissi
Copy link
Member Author

weissi commented Nov 27, 2018

@normanmaurer because otherwise you can't write this:

func runDatabaseQuery(_ query: String, database: DB) -> EventLoopFuture<[Row]> {
    return database.openConnetion { conn in
        conn.runQuery(query).always {
            conn.close()
        }
    }
}

You want to always close the DB connection. Currently you need to spell this

func runDatabaseQuery(_ query: String, database: DB) -> EventLoopFuture<[Row]> {
    return database.openConnetion { conn in
        let f = conn.runQuery(query)
        f.whenComplete {
            conn.close()
        }
        return f
    }
}

which is super ugly

@normanmaurer
Copy link
Member

@weissi I may need more coffee but I still dont get why we could not only have always at the end ?

@Lukasa
Copy link
Contributor

Lukasa commented Nov 27, 2018

Yeah, I think @normanmaurer is right, we can just have always with a discardable result.

@weissi
Copy link
Member Author

weissi commented Nov 27, 2018

@Lukasa we'll still allocate an extra, unnecessary future. We also have then & whenSuccess, thenIfError & whenFailure, but nothing & whenSuccess. I just want to fill the nothing-hole :)

@normanmaurer
Copy link
Member

@weissi why ? Couldn't it just return self in this case ?

@weissi
Copy link
Member Author

weissi commented Nov 27, 2018

@normanmaurer true

@Lukasa
Copy link
Contributor

Lukasa commented Nov 27, 2018

Bear in mind that the semantics of that are slightly different than the semantics of returning a new future, though only slightly.

@normanmaurer
Copy link
Member

@Lukasa but does it matter at all ?

@Lukasa
Copy link
Contributor

Lukasa commented Nov 27, 2018

Depends if you're trying to be really certain about future execution. Probably not though.

@Palleas
Copy link
Contributor

Palleas commented Apr 23, 2019

@normanmaurer @Lukasa @weissi Is that something we'd still want to add? 🤓

@weissi
Copy link
Member Author

weissi commented Apr 23, 2019

I'm still very much in favour of having this, every time I use NIO as a user I want to have it :). Sure, it's pretty straightforward to work around but I often have something I need to close, a connection, a file, whatever and of course I want to do it always if there's an error or not.

Palleas pushed a commit to Palleas/swift-nio that referenced this issue Apr 27, 2019
Motivation:

apple#665

Modifications:

* Add `always` in EventLoopFuture.swift
* Add tests

Result:

NIO Users will be able to use `.always` to, well, always run an action
wether the future succeed or not.
Palleas pushed a commit to Palleas/swift-nio that referenced this issue Apr 27, 2019
Motivation:

apple#665

Modifications:

* Add `always` in EventLoopFuture.swift
* Add tests

Result:

NIO Users will be able to use `.always` to, well, always run an action
wether the future succeed or not.
Palleas pushed a commit to Palleas/swift-nio that referenced this issue Apr 29, 2019
Motivation:

apple#665

Modifications:

* Add `always` in EventLoopFuture.swift
* Add tests

Result:

NIO Users will be able to use `.always` to, well, always run an action
wether the future succeed or not.
Palleas pushed a commit to Palleas/swift-nio that referenced this issue Apr 29, 2019
Motivation:

apple#665

Modifications:

* Add `always` in EventLoopFuture.swift
* Add tests

Result:

NIO Users will be able to use `.always` to, well, always run an action
wether the future succeed or not.
Palleas pushed a commit to Palleas/swift-nio that referenced this issue Apr 30, 2019
Motivation:

apple#665

Modifications:

* Add `always` in EventLoopFuture.swift
* Add tests

Result:

NIO Users will be able to use `.always` to, well, always run an action
wether the future succeed or not.
Palleas pushed a commit to Palleas/swift-nio that referenced this issue Apr 30, 2019
Motivation:

apple#665

Modifications:

* Add `always` in EventLoopFuture.swift
* Add tests

Result:

NIO Users will be able to use `.always` to, well, always run an action
wether the future succeed or not.
Palleas pushed a commit to Palleas/swift-nio that referenced this issue Apr 30, 2019
Motivation:

apple#665

Modifications:

* Add `always` in EventLoopFuture.swift
* Add tests

Result:

NIO Users will be able to use `.always` to, well, always run an action
wether the future succeed or not.
weissi pushed a commit that referenced this issue May 15, 2019
Add an `always` method on EventLoopFuture

Motivation:

#665

Modifications:

* Add `always` in EventLoopFuture.swift
* Add tests

Result:

NIO Users will be able to use `.always` to, well, always run an action
wether the future succeed or not.
@weissi
Copy link
Member Author

weissi commented May 20, 2019

Fixed by #981

@weissi weissi closed this as completed May 20, 2019
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
kind/enhancement Improvements to existing feature.
Projects
None yet
Development

No branches or pull requests

4 participants