emabee / flexi_logger

A flexible logger for rust programs that can write to stderr, stdout, and/or to log files
Apache License 2.0
307 stars 50 forks source link

flexi_logger 0.19.6 has utc time #99

Closed incker closed 2 years ago

incker commented 2 years ago

flexi_logger 0.19.6 has utc time flexi_logger 0.19.5 had local time

Maybe it is not about flexi_logger crate. But chrono and time ones But how to switch back to local?

By the way trying to do UtcOffset::current_local_offset().unwrap();

Cause panic

thread 'main' panicked at 'called Result::unwrap() on an Err value: IndeterminateOffset', src/main.rs:264:58

emabee commented 2 years ago

0.19.6 switched form chrono to the current version of time, due to an unhandled CVE (see https://github.com/emabee/flexi_logger/issues/97).

I was hoping that this change would not impact users. What exactly did you do to encounter problems?

incker commented 2 years ago

0.19.5 -> 0.19.6 migration makes some breaking changes

image

logger
    .format(custom_format)
    .start()
    .unwrap();
incker commented 2 years ago

But I can not handle why I have utc time instead of local..

emabee commented 2 years ago

0.19.5 -> 0.19.6 migration makes some breaking changes Correct, I overlooked that, sorry.

But I can not handle why I have utc time instead of local.. What environment are you running in? Plain linux?

incker commented 2 years ago

What environment are you running in? Plain linux?

linux mint

emabee commented 2 years ago

So in your environment, will OffsetDateTime::now_local().unwrap() panic?

I'm asking because DeferredNow::now() calls OffsetDateTime::now_local().unwrap_or_else(|_| OffsetDateTime::now_utc()), thus silently ignores errors from OffsetDateTime::now_local()

incker commented 2 years ago

Have asked a question here also:

https://github.com/time-rs/time/discussions/389

I'm asking because DeferredNow::now() calls OffsetDateTime::now_local().unwrap_orelse(|| OffsetDateTime::now_utc()), thus silently ignores errors from OffsetDateTime::now_local()

True, that is why I have utc time

I do not know why, but 0.19.5 version with chrono had local time

emabee commented 2 years ago

Would you please try out if their unsound_local_offset pseudo-feature helps? It is described in the last section of https://docs.rs/time/0.3.4/time/#feature-flags.

incker commented 2 years ago

Yes, using RUSTFLAGS="--cfg unsound_local_offset" gives correct local time

Looks like chrono and time have same issue Chrono: https://github.com/chronotope/chrono/issues/499 Time: https://github.com/time-rs/time/issues/293

So reading changelog https://github.com/emabee/flexi_logger/blob/master/CHANGELOG.md#0196---2021-10-26 nearly nothing have changed)

But just let me know which crate you'll use in flexi_logger in future releases? time or chrono?

emabee commented 2 years ago

Thanks for checking. The situation is really bad, we're caught between a rock and a hard place, either we face the risk of unexpected crashes (I'm not sure how likely they are, though) or we fall back to UTC unexpectedly unless end consumers use this RUSTFLAGS="--cfg unsound_local_offset" hack.

I could (1) try factoring out the time vs chrono use, and allow choosing between them via a feature switch. Or we could (2) hope that the fix for time comes fast, which is not very likely. So let me see how (1) works out.

incker commented 2 years ago

Just want to say that features in crates does not have to be "mutually exclusive" I mean that adding feature time + chrono in a same time does not have cause compile errors in your crate

incker commented 2 years ago

Maybe it will be better solution to give choise UTC or "local time on your own risk" not sure

And time will show which crate time or chrono is more maintained and actual

emabee commented 2 years ago

I published 0.20.0 now, which uses the new time 0.3.5, which should allow using local time in single-threaded programs. I changed the time use such that I fetch the UTC offset only once, when flexi_logger is initialized, and then fetch only local time and add the remembered offset explicitly. This should then also work if the program switches to multi-threading after the logger is initialized,

emabee commented 2 years ago

Please let me know if this works for you.

incker commented 2 years ago

Everything works correct in 0.20.0 Time is local (not UTC) printing timezone using formatter is also correct

incker commented 2 years ago

Update: time is not local if not set export RUSTFLAGS="--cfg unsound_local_offset"

emabee commented 2 years ago

That's too bad - and not in line with what time promises, I think. Just to be sure: are you initializing flexi_logger in your main? Is it a plain main, or is it an async main?

emabee commented 2 years ago

For some additional information, see time-rs/time#380. According to the discussion there, time 0.3.5 uses /proc/self/task on Linux to determine if multiple threads are running.

I could add an optional feature to use chrono for determining the offset. This brings back the original situation, where, if I got it right, the application must refrain from modifying the environment in concurrent threads.

jhpratt commented 2 years ago

The time crate will never return UTC as the local offset if it can't determine it; it'll return the error value.

emabee commented 2 years ago

That's the right thing to do, definitely. I just hope that it will be possible soon to determine the local offset in a safe manner on all important platforms.

jhpratt commented 2 years ago

Likewise! The primary thing that isn't implemented is a way to determine the number of threads on Mac. Windows works unconditionally and Linux works when single-threaded. Other platforms are, of course, less common. I'm open to implementations, though.

snaar commented 2 years ago

This was mentioned earlier in this issue, but what about introducing option to always use UTC with no offset? I kinda always wanted that feature regardless.

tdudz commented 2 years ago

hi @emabee

it looks like latest release of time 0.3.6 supports local offset on all platforms now

Screen Shot 2022-01-22 at 1 47 00 AM
emabee commented 2 years ago

Thanks for triggering; with version 0.22.3 I updated the dependency to time to version 0.3.7.