Skip to content
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

Fix Scheduler implementation for ILP32 platforms such as watchOS #1137

Merged
merged 1 commit into from
Sep 5, 2023

Conversation

christopherweems
Copy link
Contributor

Fix Scheduler implementation for ILP32 platforms such as watchOS on arm64_32

Motivation:

Fix a bug in the Scheduler where converting the UInt64 nanoseconds value from a Duration value to the DispatchTimeInterval's Int associated value causes an uncaught overflow and crash on devices with 32-bit Int values.

Modifications:

Add DispatchTime.init(nowDelayedBy:), which computes the deadline and avoids casting to Int
Replace adhoc computations of Scheduler deadline with designated DispatchTime.init(nowDelayedBy:)

Result:

Prevents crash on ILP32 platforms when time delay exceeds a value that fits within a 32-bit Int

@ktoso
Copy link
Member

ktoso commented Sep 5, 2023

@swift-server-bot test this please

@ktoso
Copy link
Member

ktoso commented Sep 5, 2023

Unrelated test hang on 5.8 only: #1138

@ktoso
Copy link
Member

ktoso commented Sep 5, 2023

Thanks, though I'm very surprised about using the cluster on a watch -- that's not a great ideas since it needs to perform heartbeats and active connections which would drain power quickly etc. It's not really a platform we support with the cluster.

Fix looks okey though, thank you!

@ktoso ktoso merged commit dd358de into apple:main Sep 5, 2023
@christopherweems
Copy link
Contributor Author

Ahh my novice is showing -- thanks for taking a look. I'll look towards moving to another actor system, or rolling one, but it the mean time its really nice to be able to connect to the cluster system while I figure this stuff out. Thanks again!

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.

2 participants