fix timestamp_local()

[dimbleby]

cf #43

[Techcable]

Strangely I get an indeterminate offset error on my local machine as well.

I will have to look into this more (also thank you for the PR 😄 )

[ohmyarch]

The time displayed now is actually UTC, which should be a bug.

[Techcable]

I asked why this is failing in time-rs/time#455 and got this response:

The method will fail on Unix systems when multiple threads are running. This is because it is otherwise unsound. For libraries, there is no workaround because you have no way of knowing what the end user will do with regard to threads.

So unfortunately it appears we are either: A. Stuck with UTC B. Have a programatic API to explicitly specify the timezone offset (working around the multiple threads problem)

[Techcable]

I think we could override the use_local_timestamp method to accept a user-specified time offset.

This would allow workaround the nasty soundness issues of the underlying system methods.

Then we could just have UTC for time values (by default).

Technically requiring an explicit offset is a breaking change. However it isn't really breaking because the code relying on system offset was never sound before. I am not sure if the previous impl was silently unsound or always gave an error. I will have to go back and check......

[yaozongyou]

after some dig, the time crate using libc's localtime_r function to convert a timestamp to the user's specified timezone localtime, and as the manual suggests: for portable code, tzset(3) should be called before localtime_r(). And tzset function settting environment variable is not thread safe, so this is the problem: https://github.com/time-rs/time/blob/500f8e4517cb89f0aa80087130ed2f8b8349f162/time/src/sys/local_offset_at/unix.rs#L147

Maybe we can switch depency from time to chrono, the chrono has no such limitations, the chrono do not using localtime_r, it gets localtime's timezone with TZ environment variable and fall back to /etc/localtime config.

[Jarsop]

Hello, regardless of the fact that time uses a method that is not thread safe, a simple test (single threaded) highlights the conversion problem.

Code:

fn main() {
    println!(
        "Into::<time::OffsetDateTime>::into(std::time::SystemTime::now()): {:?}",
        Into::<time::OffsetDateTime>::into(std::time::SystemTime::now())
    );
    println!(
        "time::OffsetDateTime::now_utc(): {:?}",
        time::OffsetDateTime::now_utc()
    );
    println!(
        "time::OffsetDateTime::now_local(): {:?}",
        time::OffsetDateTime::now_local().unwrap()
    );
}

Output:

Into::<time::OffsetDateTime>::into(std::time::SystemTime::now()): 2023-09-28 21:07:56.911696843 +00:00:00
time::OffsetDateTime::now_utc(): 2023-09-28 21:07:56.91171041 +00:00:00
time::OffsetDateTime::now_local(): 2023-09-28 23:07:56.911713926 +02:00:00

So II think the method used so far to handle local time doesn't work :)

[Techcable]

I just had an idea on how to fix the default time formatting without breaking backwards compatibility.

The new default formatting will include an explicit UTC timezone specifier, so it will be clear it's not in local time.

This will avoid the unsafety of getting the localtime when multiple threads are involved.