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

V8 no longer resets the timezone cache upon invoking v8::Date::DateTimeConfigurationChangeNotification #19974

Closed
jeroenvollenbrock opened this issue Apr 12, 2018 · 12 comments · Fixed by #20026
Labels
v8 engine Issues and PRs related to the V8 dependency.

Comments

@jeroenvollenbrock
Copy link

jeroenvollenbrock commented Apr 12, 2018

  • Version: 8.9.4 (First impact at 7.0.0)
  • Platform: Linux 4.14.15-h1 deps: update openssl to 1.0.1j #1 SMP Wed Jan 31 15:49:06 UTC 2018 armv7l GNU/Linux
  • Subsystem: V8 Engine

Apparently one of the V8 updates after Node.js 4.8.4 6.12.2 caused the v8::Date::DateTimeConfigurationChangeNotification that is triggered by the reset-date-cache module of @evanlucas to not actually invalidate the timezone everywhere. I'm not completely sure if this is i18n related or if it fails to reach the v8::base::TimezoneCache::Clear stage. Issue has been first reported in the issue tracker of the module, but the cause seems to be in V8 itself instead after more debugging. I'm suspecting the upgrade to V8 5.0 or 6.0 5.4 to be the cause , as there are multiple chromium issues describing similar behaviour after these V8 Engine versions got landed, and a few of them required changes on the chromium side.
This issue can be easily reproduced by running the test code of the reset-date-cache module, might be a good candidate for CitGM as well?

@bnoordhuis
Copy link
Member

not actually invalidate the timezone everywhere

Can you be more precise? Does it work when you pass --noicu_timezone_data on the command line?

@bnoordhuis bnoordhuis added the v8 engine Issues and PRs related to the V8 dependency. label Apr 12, 2018
@jeroenvollenbrock
Copy link
Author

jeroenvollenbrock commented Apr 12, 2018

I haven't been able to make the testcase work at all, but in cluster workers and VM's the timezone does change. I think this is caused by the different isolate context that is used there, since it probably just didn't have a populated cache yet. --noicu_timezone_data does not seem to make the testcase pass either.

The issue seems OS dependent, but not architecture dependent, as the test runs successfully on OSX, but fails on Linux (tested on arm, x64 and x86)

@jeroenvollenbrock
Copy link
Author

I've pushed a task to our testfarm and Node.js 7.0.0 on Linux seems to be the first impacted version, which would place the most likely cause at the V8 5.4 upgrade

@bnoordhuis
Copy link
Member

@evanlucas Can you comment?

FWIW, the date cache reset logic in V8 doesn't really seem to have changed in the last two years or so.

@evanlucas
Copy link
Contributor

evanlucas commented Apr 13, 2018

yea this is strange. On debian jessie for me, it seems like the TZ environment variable is only read on boot or first access (not sure which one) and never read again, even after calling the following:

Isolate *isolate = Isolate::GetCurrent();
Date::DateTimeConfigurationChangeNotification(isolate);

It does seem to work on macOS though.

@bnoordhuis
Copy link
Member

Right, I understand now that the idea is to let you do this:

process.env.TZ = 'Europe/Amsterdam'
require('reset-date-cache').reset()
process.env.TZ = 'Europe/London'

That doesn't work due to glibc's timezone cache but #20026 addresses that.

I'm not 100% convinced it's a good idea but since people keep bumping into TZ's 'immutability', and since it obsoletes reset-date-cache, I guess it's progress.

@jeroenvollenbrock
Copy link
Author

I’m actually using it in combination with https://github.com/athombv/node-systemd-timedated-client/ to reset the cache when the system timezone changes, which doesnt work either. My TZ env var used to be unset, but i’ve tried both with and without setting it to the changed timezone before resetting the cache.

@bnoordhuis
Copy link
Member

The missing piece is the tzset() call (_tzset() on Windows) to reset libc's timezone cache.

@evanlucas FYI, might want to add that to reset-date-cache.

@jeroenvollenbrock
Copy link
Author

jeroenvollenbrock commented Apr 14, 2018

Don’t want to be nitpicking here, but if this is the case, shouldnt it be something the V8 notification should invoke instead of the caller? 🙈

@bnoordhuis
Copy link
Member

You could open a V8 issue but I expect the answer is no. tzset() has process-wide side effects. V8 is a library and changes in a library should not affect other parts of the program.

@evanlucas
Copy link
Contributor

Thanks for the fix @bnoordhuis! Just updated reset-date-cache

@BridgeAR
Copy link
Member

BridgeAR commented Mar 9, 2019

Reopen due to reverting the original fix.

@BridgeAR BridgeAR reopened this Mar 9, 2019
bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue May 23, 2019
Since the presence of the libc and V8 timezone caches seem to be
a perennial source of confusion to users ("why doesn't it work?!"),
let's try to support that pattern by intercepting assignments to
the TZ environment variable and reset the caches as a side effect.

Fixes: nodejs#19974
PR-URL: nodejs#20026
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Steven R Loomis <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants