nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
105.14k stars 28.48k forks source link

TZ environment variable doesn't parse POSIX strings correctly #46246

Open mattjohnsonpint opened 1 year ago

mattjohnsonpint commented 1 year ago

Version

v16.14.0

Platform

Darwin XHFHLG2CJP.local 22.1.0 Darwin Kernel Version 22.1.0: Sun Oct 9 20:15:09 PDT 2022; root:xnu-8792.41.9~2/RELEASE_ARM64_T6000 arm64

Subsystem

No response

What steps will reproduce the bug?

When the TZ environment variable is set and interpreted by Node, it should support any of the formats allowed by the TZ environment variable (see man tzset).

It does appear to support IANA time zones (ex America/Los_Angeles) and POSIX time zones (ex PST8PDT), but it is not supporting POSIX time zones that include minutes or seconds in the offset, which is allowed by the TZ spec.

Example that works:

TZ=XXX+12 node -e 'console.log((new Date).getTimezoneOffset())'
# returns 720

Example that should work and does not:

TZ=XXX+12:00 node -e 'console.log((new Date).getTimezoneOffset())'
# should return 720, but returns whatever the local system's current time zone offset is

How often does it reproduce? Is there a required condition?

always reproducible

What is the expected behavior?

The TZ format should work in its entirety, as in the spec https://man7.org/linux/man-pages/man3/tzset.3.html

What do you see instead?

Only IANA time zones, or POSIX time zones with simple whole-hour offsets are supported.

Additional information

https://stackoverflow.com/questions/75150048/tz-utc0300-vs-tz-utc3-difference-for-nodejs

bnoordhuis commented 1 year ago

This is an upstream bug. TZ parsing is done by ICU, not node.

I remember an upstream bug report exists but I can't find it. The specific ICU function is i18n::SetDefaultTimeZone().

mattjohnsonpint commented 1 year ago

FWIW, I don't think this is super important to most people. It really only applies in scenarios where POSIX time zones are still prevalent, such as IoT devices.

mattjohnsonpint commented 1 year ago

After reading more through ICU sources, I think this is not necessarily an ICU issue. Node's i18n::SetDefaultTimeZone calls ICU's ucal_setDefaultTimeZone which in turn calls icu::TimeZone::createTimeZone, which documents supporting only IANA time zone identifiers, or custom IDs with the example given as "GMT-8:00". The code comments also say that the custom zone should be of the form GMT[+-]hh:mm. So honestly, I'm not quite sure how even XXX+12 (my first example) is working at all.

Digging even deeper, it seems that ICU doesn't have a parser for POSIX time zone strings at all. Instead, its TimeZone::detectHostTimeZone function is using standard C runtime functions like tzset that I referenced earlier (via uprv_tzset in ICU).

Perhaps Node should simply not call ucal_setDefaultTimeZone at all when the TZ environment variable is set? That way, the system default would be used - picking up on the TZ variable (I think).

bnoordhuis commented 1 year ago

Perhaps Node should simply not call ucal_setDefaultTimeZone at all when the TZ environment variable is set?

I'm 95% sure you get inconsistent behavior on UNIX and Windows without it but maybe @srl295 can advise.