Skip to content
This repository has been archived by the owner on Jan 7, 2025. It is now read-only.

Handle failure of current_local_offset without panicking #15

Closed

Conversation

jannic
Copy link
Contributor

@jannic jannic commented Oct 19, 2023

No description provided.

@jkarneges
Copy link
Member

My only concern with this is we might log as UTC when a valid timezone was set and expected to work. Getting UTC logs on a local machine is weird.

It could be that some Mac dependency is spawning threads before main() runs. For example, I noticed this happens when linking against Qt on an Intel Mac. If I call current_local_offset() as the first line of code, it is still too late. In the case of condure, I wonder if some other dependency like openssl is causing it, or if it's libc or some foundational Mac lib that varies by arch.

My feeling is that if it's impossible to safely get the timezone in a modern Mac app, or at least when linking to popular dependencies, then it's reasonable to get the timezone unsafely, especially if it's the first line of code, otherwise nobody would be able to have timezones and that would be crazy.

@jannic
Copy link
Contributor Author

jannic commented Oct 20, 2023

Yes, I think getting the timezone unsafely is a reasonable alternative:

It's not really current_local_offset() that's inherently unsafe in multi-threaded code. What makes it dangerous is that safe rust could call std::env::set_var in parallel, which could trigger UB.

As long as we know that we (or anything else in the process) won't alter an environment variable in parallel to this call, using the officially discouraged time::util::local_offset::set_soundness shouldn't cause any unsoundness.

Also, time-rs/time#610 indicates that there is no soundness issue at all in recent versions of MacOS, as the operation system seems to implement sufficient locking.

@jannic
Copy link
Contributor Author

jannic commented Oct 20, 2023

Another note: current_local_offset() could fail for other reasons. The docs just say "If the offset cannot be determined, an error is returned."
IMHO in such a situation it would still better to start condure with UTC logging, instead of panicking. So this change would still be valuable even if we also called set_soundness(Soundness::Unsound).

@jkarneges
Copy link
Member

Since condure development has moved to the pushpin repo, I've made the changes over there instead: fastly/pushpin#47826

We could consider making similar changes in this repo, although if we do that we should look at merging all changes and not just this one.

@jkarneges jkarneges closed this Dec 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants