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

Pure-swift hang() and tests. #741

Merged
merged 2 commits into from Dec 8, 2017

Conversation

2 participants
@abl
Contributor

abl commented Nov 30, 2017

Hi! I've been writing some commandline tools in Swift and wanted to leverage promises; I need to ensure the command doesn't terminate before the promises resolve. After reading #423, here's a ~port of PMKHang to Swift.

Tested using Xcode 8.3.3 / Swift 3.1 on macOS and Swift 4.0.2 on Ubuntu 14.04.

@mxcl

This comment has been minimized.

Show comment
Hide comment
@mxcl

mxcl Nov 30, 2017

Owner

PMK5 (recently released has wait())

https://github.com/mxcl/PromiseKit/blob/master/Sources/Promise.swift#L73-L98

Now, it operates differently, and I don't know which approach is better.

Also, do you use PMK5 and not 4? I haven't pushed 5 to CocoaPods yet is why I ask. I planned to write a blog post and document it better first.

Anyway, this is much appreciated. What do you think? Do you want this in 4, maybe also in 5? Do you think the approach you have taken with runloops is the more sensible choice?

I'm not sure really, I added wait() to PMK5 as an experiment, so I don't have firm feelings on this matter.

Owner

mxcl commented Nov 30, 2017

PMK5 (recently released has wait())

https://github.com/mxcl/PromiseKit/blob/master/Sources/Promise.swift#L73-L98

Now, it operates differently, and I don't know which approach is better.

Also, do you use PMK5 and not 4? I haven't pushed 5 to CocoaPods yet is why I ask. I planned to write a blog post and document it better first.

Anyway, this is much appreciated. What do you think? Do you want this in 4, maybe also in 5? Do you think the approach you have taken with runloops is the more sensible choice?

I'm not sure really, I added wait() to PMK5 as an experiment, so I don't have firm feelings on this matter.

@abl

This comment has been minimized.

Show comment
Hide comment
@abl

abl Nov 30, 2017

Contributor

PMK5 (recently released has wait())

Whoops, I forgot to mention that I tried wait() first and it deadlocked the main thread - for example:

func testWait() {
    let ex = expectation(description: "block executed")
    after(seconds: 0.02).done {
        ex.fulfill()
    }.wait()
    waitForExpectations(timeout: 0)
}

This makes sense; it's possible to remove the deadlock by using done(on:, ...) but if any part of your chain hits the main queue you're sunk...and the GCD API doesn't seem to offer a clean solution to this. (dispatch_get_current_queue is missed.)

Also, do you use PMK5 and not 4?

Yes, I'm using PMK5 - figured it'd be best to start acclimating early. :)

Do you want this in 4, maybe also in 5?

Haven't tested this revision against 4 - I had something similar that worked in 4 but was less elegant. Would be happy to have it in 5 and if you think it'd be valuable to backport to 4 I can revisit my original attempt.

Do you think the approach you have taken with runloops is the more sensible choice?

I'm just copying your approach from PMKHang.m - I prefer your approach in wait(). wait() is conceptually cleaner with less overhead; hang() just happens to be useful when invoked from a non-runloop application.

Contributor

abl commented Nov 30, 2017

PMK5 (recently released has wait())

Whoops, I forgot to mention that I tried wait() first and it deadlocked the main thread - for example:

func testWait() {
    let ex = expectation(description: "block executed")
    after(seconds: 0.02).done {
        ex.fulfill()
    }.wait()
    waitForExpectations(timeout: 0)
}

This makes sense; it's possible to remove the deadlock by using done(on:, ...) but if any part of your chain hits the main queue you're sunk...and the GCD API doesn't seem to offer a clean solution to this. (dispatch_get_current_queue is missed.)

Also, do you use PMK5 and not 4?

Yes, I'm using PMK5 - figured it'd be best to start acclimating early. :)

Do you want this in 4, maybe also in 5?

Haven't tested this revision against 4 - I had something similar that worked in 4 but was less elegant. Would be happy to have it in 5 and if you think it'd be valuable to backport to 4 I can revisit my original attempt.

Do you think the approach you have taken with runloops is the more sensible choice?

I'm just copying your approach from PMKHang.m - I prefer your approach in wait(). wait() is conceptually cleaner with less overhead; hang() just happens to be useful when invoked from a non-runloop application.

@mxcl

This comment has been minimized.

Show comment
Hide comment
@mxcl

mxcl Nov 30, 2017

Owner

K cool, then I think we need to offer both, so I'll figure out how to merge this.

Owner

mxcl commented Nov 30, 2017

K cool, then I think we need to offer both, so I'll figure out how to merge this.

@mxcl

This comment has been minimized.

Show comment
Hide comment
@mxcl

mxcl Nov 30, 2017

Owner

Tests fail for iOS, tvOS and watchOS.

Owner

mxcl commented Nov 30, 2017

Tests fail for iOS, tvOS and watchOS.

@mxcl

This comment has been minimized.

Show comment
Hide comment
@mxcl

mxcl Dec 4, 2017

Owner

Will merge, when CI passes.

Owner

mxcl commented Dec 4, 2017

Will merge, when CI passes.

@abl

This comment has been minimized.

Show comment
Hide comment
@abl

abl Dec 6, 2017

Contributor

Sorry, I've been out sick.

Investigating iOS et al - practically, there's no reason to call hang() over wait() as all iOS/tvOS/watchOS apps will have runloops.

Contributor

abl commented Dec 6, 2017

Sorry, I've been out sick.

Investigating iOS et al - practically, there's no reason to call hang() over wait() as all iOS/tvOS/watchOS apps will have runloops.

@abl

This comment has been minimized.

Show comment
Hide comment
@abl

abl Dec 6, 2017

Contributor

Ok, the issue is that Apple-y platforms (that is, not os(Linux)) use a different CFRunLoop setup. After this tweak the ATV and iOS tests pass locally - the watchOS tests won't build with/without my changes for some reason on Xcode 9.2.

Long term this may introduce other issues - other platforms being added to Swift (e.g. Haiku, Fuchsia) will have to be added to this conditional.

Contributor

abl commented Dec 6, 2017

Ok, the issue is that Apple-y platforms (that is, not os(Linux)) use a different CFRunLoop setup. After this tweak the ATV and iOS tests pass locally - the watchOS tests won't build with/without my changes for some reason on Xcode 9.2.

Long term this may introduce other issues - other platforms being added to Swift (e.g. Haiku, Fuchsia) will have to be added to this conditional.

@mxcl mxcl merged commit 4742e2e into mxcl:master Dec 8, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment