Skip to content

Conversation

@ktopley-apple
Copy link
Contributor

Fixes overflows in DispatchTime/DispatchWallTime/DispatchTimeInterval.

Examples:

DispatchTime.now() + Date.distantFuture.timeIntervalSinceNow // traps
DispatchTime.now() + Date.distantPast.timeIntervalSinceNow. // traps
let t = DispatchTimeInterval.seconds(Int.max)
t == t // Traps due to conversion to nanoseconds.

Also added tests for these and similar cases.

@ktopley-apple
Copy link
Contributor Author

@swift-ci please smoke test

1 similar comment
@ktopley-apple
Copy link
Contributor Author

@swift-ci please smoke test

Copy link
Contributor

@jrose-apple jrose-apple left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why these properties are desirable. You're going to say that DispatchTimeInterval.seconds(Int.max) == .milliseconds(Int.max), which is silly. And the other traps do seem like they should trap.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use multiplyWithOverflow for this instead…

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean multipliedReportingOverflow()? I didn't know about that :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the change overall, the general rule in libdispatch is that operations on dispatch_time_t that overflow are clamped to DISPATCH_TIME_FOREVER. The intent of this change is to make the same true in Swift. For the change to DispatchTimeInterval, you are right that it might seem odd that DispatchTimeInterval.seconds(Int.max) == .milliseconds(Int.max), but DispatchTimeInterval is used as an offset to form a DispatchTime or DispatchWallTime and the result of using either of these values is going to be clamped to Dispatch[Wall]Time.distantFuture. Currently, you can't say "DispatchTime.now() + DispatchTimeInterval.seconds(Int.max)" because it traps. Of course Int.max is a pathological example - in reality, there have been cases where large values have been used that have caused traps and this had to be worked around by checking the value before passing it to dispatch and using DispatchTime.distantFuture instead. I probably should add some comments to DispatchTimeInterval that explain this behavior.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

…and I'm not sure why this isn't just a transformation on a single Double.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain what you mean by that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like it's equivalent to toInt64Clamped(m1 * m2).

@ktopley-apple
Copy link
Contributor Author

ktopley-apple commented Sep 14, 2017

@swift-ci please smoke test

@ktopley-apple
Copy link
Contributor Author

Updated based on Jordan's comments.

@ktopley-apple
Copy link
Contributor Author

@swift-ci please smoke test

Copy link
Contributor

@jrose-apple jrose-apple Sep 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I realized this check still isn't quite correct because Double doesn't have enough precision to represent Int64.max. One way to do it would be to check against Double(bitPattern: Double(Int64.max).bitPattern-1) instead, but that's black magic. @stephentyrone, what's the right way to write this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it's not 100% correct, but it I think it is the closest I can get without "black magic". Maybe a version of this that can use the black magic should be added to the standard library. Anyway, it should be OK as it stands because we don't need to be completely precise - we just need to catch times that are far enough in the future that we should treat them as "forever".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add a clamping init from Int to Double, but that's outside the scope of this change. As a temporary hack, it suffices to make this if value >= Double(Int64.max); that does the right thing without black magic.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jrose-apple More generally, instead of Double(bitPattern: x.bitPattern-1) use x.nextDown if you know x is positive.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I was looking for nextDown and didn't see it. Thanks, Steve.

@ktopley-apple
Copy link
Contributor Author

@swift-ci please smoke test

1 similar comment
@ktopley-apple
Copy link
Contributor Author

@swift-ci please smoke test

@ktopley-apple
Copy link
Contributor Author

@swift-ci please smoke test

@ktopley-apple
Copy link
Contributor Author

Squashed commits.

@ktopley-apple
Copy link
Contributor Author

@swift-ci please smoke test

@ktopley-apple ktopley-apple merged commit 0dc6201 into swiftlang:master Sep 20, 2017
@jrose-apple
Copy link
Contributor

Looks like we've still got a problem here on a 32-bit iOS device.

02:22:19 [ RUN      ] DispatchAPI.DispatchTime.addSubtract
02:22:19 stderr>>> CRASHED: SIGTRAP
02:22:19 the test crashed unexpectedly
02:22:19 [     FAIL ] DispatchAPI.DispatchTime.addSubtract

Let me know if you want more information.

@jrose-apple
Copy link
Contributor

(This is rdar://problem/34751238, by the way.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants