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

flexi_logger 0.19.6 has utc time #99

Closed
incker opened this issue Nov 9, 2021 · 24 comments
Closed

flexi_logger 0.19.6 has utc time #99

incker opened this issue Nov 9, 2021 · 24 comments

Comments

@incker
Copy link

incker commented Nov 9, 2021

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
Copy link
Owner

emabee commented Nov 9, 2021

0.19.6 switched form chrono to the current version of time, due to an unhandled CVE (see #97).

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

@incker
Copy link
Author

incker commented Nov 9, 2021

0.19.5 -> 0.19.6 migration makes some breaking changes

image

logger
    .format(custom_format)
    .start()
    .unwrap();

@incker
Copy link
Author

incker commented Nov 9, 2021

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

@emabee
Copy link
Owner

emabee commented Nov 9, 2021

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
Copy link
Author

incker commented Nov 9, 2021

What environment are you running in? Plain linux?

linux mint

@emabee
Copy link
Owner

emabee commented Nov 9, 2021

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
Copy link
Author

incker commented Nov 10, 2021

Have asked a question here also:

time-rs/time#389

I'm asking because DeferredNow::now() calls OffsetDateTime::now_local().unwrap_or_else(|_| 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
Copy link
Owner

emabee commented Nov 10, 2021

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
Copy link
Author

incker commented Nov 11, 2021

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

Looks like chrono and time have same issue
Chrono:
chronotope/chrono#499
Time:
time-rs/time#293

So reading changelog https:/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
Copy link
Owner

emabee commented Nov 11, 2021

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
Copy link
Author

incker commented Nov 11, 2021

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
Copy link
Author

incker commented Nov 11, 2021

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

@incker incker changed the title flexy_logger 0.19.6 has utc time flexi_logger 0.19.6 has utc time Nov 12, 2021
@emabee
Copy link
Owner

emabee commented Nov 13, 2021

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
Copy link
Owner

emabee commented Nov 15, 2021

Please let me know if this works for you.

@incker
Copy link
Author

incker commented Nov 15, 2021

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

@incker incker closed this as completed Nov 15, 2021
@incker
Copy link
Author

incker commented Nov 15, 2021

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

@emabee
Copy link
Owner

emabee commented Nov 15, 2021

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
Copy link
Owner

emabee commented Nov 16, 2021

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
Copy link

jhpratt commented Nov 26, 2021

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

@emabee
Copy link
Owner

emabee commented Nov 26, 2021

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
Copy link

jhpratt commented Nov 26, 2021

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
Copy link

snaar commented Nov 28, 2021

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
Copy link

tdudz commented Jan 22, 2022

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
Copy link
Owner

emabee commented Feb 2, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants