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
Use thread safe libtz instead of libc to get local timezone offset on unix. #543
Conversation
Codecov Report
@@ Coverage Diff @@
## main #543 +/- ##
=======================================
+ Coverage 95.3% 95.5% +0.3%
=======================================
Files 78 78
Lines 8563 8533 -30
=======================================
- Hits 8157 8151 -6
+ Misses 406 382 -24
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Fixes time-rs#293 without needing to disable the feature.
Presumably on systems where this is a problem, that won't help with In crates that adopt |
I don't think other threads doing C Definitely another thread using an unsafe C It would also possible to remove the lookup of the
In theory that should be possible. I'm not sure how to detect it though. Are you thinking some sort of explicit list of good OSes? I guess it's really the C library version, not the OS per se. Do you know of any implementations that are thread safe? |
FWIW, I suspect the Linux ecosystem is somewhat unusual in housing a menagerie of libc implementations. I expect most other UNIX environments (e.g., illumos, the BSDs, commercial/proprietary systems) provide The illumos implementations of tzset(3C), localtime_r(3C), setenv(3C), and getenv(3C), all have an MT-Level of Safe or MT-Safe; MT-Level and other attributes are described in attributes(7). We inherited them a decade ago when we forked from Solaris, so I would expect that Solaris is also thread safe for all of these routines. In addition to thread safety, our time zone configuration is a bit different from a Linux system, though most of this is hidden from a consuming process behind the standard entrypoints like A process determines the local time zone in roughly this way:
Iff a process does not have Historically systems like Java and Go and so on have created challenges for operators by ignoring the local time zone database, or at least some of the system facilities for configuring and refreshing it. It would be really great if we could avoid having that happen with Rust, where there are truly superlative facilities for using system library routines through FFI, etc! |
Interesting. Though it seems once it has the timezone name it still parses it the same way, either as a POSIX inline definition or as a tzdata file compiled by
I agree. FWIW libtz currently doesn't override the database, it uses system database files. But it does replace the system library, so I decided to poke around existing implementations that I can find:
It does appear from that list that really the only OSes that need to be worked-around are linux/glibc, freebsd, and openbsd. Of those only freebsd would have an inferior tz implementation (in the sense that the IANA libtz code doesn't reload itself ever). But I think I agree that this should probably be more of a surgical workaround than what it currently is. I'll see if I can come up with something tomorrow. |
While this approach in general is tenable, there are a number of problems here. I won't list the ones that I noticed within ~5 minutes of looking at this, as to be honest I think it would be somewhat rude (feel free to reach out privately). By far the most important, however, is that the repository you linked does not compile on my machine. I get a linker error, which is a dealbreaker right off the bat. My setup isn't anything crazy, either — Fedora on a ThinkPad. For this reason, I am closing this pull request. With regard to it being effectively unusable in async code, that is simply due to the safety requirements. The next release of |
And as a side note, if there are operating systems that have thread-safe environment mutation, please let me know with references. I should be able to opt specific OS's out of the multi-threaded check quite easily. |
It's mostly here in this message, albeit more verbosely, so I'll summarize here (and add illumos links): |
Thanks! I'll look that over and add opt-outs as appropriate. |
illumos, NetBSD, and MacOS now bypass the check on |
Fixes #293 without needing to disable the feature on multithreaded programs.
I wrote the Rust libtz/libtz-sys crates to address this problem. They wrap IANA's libtz (repo, homepage), which is maintained in the same repo with their timezone and leapsecond datasets.
The libtz-sys crate has 2 interfaces, one that wraps
localtime_r
and another that wrapslocaltime_rz
. Both should be thread safe:localtime_r
is built such that its call togetenv()
is routed to a small Rust function that wrapsstd::env::var_os()
which locks (against other Rust env manipulation).localtime_rz
is a NetBSD interface that doesn't callgetenv()
at all (you have to "alloc" a timezone storage struct, which takes the timezone name as a parameter).The libtz crate provides a slightly more idiomatic interface to the libtz-sys crate. Currently it only uses libtz-sys's
localtime_rz
interface because that seems a little cleaner (though it does usestd::env::var_os()
to read theTZ
env var, since that's the standard way to override the timezone for a process).I set up a repo with the multithreaded POC in the original bug report.
Its
Cargo.toml
is currently pointing to this pull request branch to show that it doesn't crash.Things I'm worried about:
Testing. I've only really run this on a Debian x86_64 box and an aarch64 Mac. The libtz code looks like it supports a lot of OSes, but I can't really vouch for it. I don't know how to test on a wide variety of systems easily. Recommendations are welcome.
Speed. The
localtime_tz
call that's currently being used requires calls totzalloc()
andtzfree()
which allocate and free the timezone data (it also reads and parses the binary tzdata file on alloc). Becauselocal_offset_at()
is a static function it has to alloc and free on every call.I am worried this might be slow. It looks like there is some benchmarking code in
time
. I've not done benchmarking in Rust before—is it easy to test this one section of the code?There are 2 potential fixes for this if it is a problem:
Surface the
localtime_t
interface in the Rust libtz crate and use that instead.The
localtime_t
call stores the timezone data in a static buffer (protected by a pthread mutex). This only requires it to parse the timezone binary file once.Change this PR to use something like
local_static
orthread_local!
to keep theTimezone
around to avoid the need to alloc and free it each time.I would love to get localtime support into multithreaded apps (pretty much anything async can't use it at the moment!).
Thoughts?