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

fix(ext/net): update moka cache to avoid potential panic in Deno.resolveDns on some laptops with Ryzen CPU #27572

Merged
merged 1 commit into from
Jan 8, 2025

Conversation

tatsuya6502
Copy link
Contributor

@tatsuya6502 tatsuya6502 commented Jan 7, 2025

Fixes #27534

This PR updates the moka crate in Cargo.lock from v0.12.7 to v0.12.10 (by running cargo update -p moka). This should prevent potential panic in Deno.resolveDns on some laptops with Ryzen CPU.

No test case, as the changes does not affect the behavior of Deno itself.

Some background information

Deno v2.1.4 started using the moka crate via the hickory-resolver crate. When Deno.resolveDns is called, hickory-resolver uses moka to cache DNS lookup results in memory. However, moka v0.12.8 or older may panic on some laptops with Ryzen CPUs when using some specific features of moka. Deno v2.1.4 uses moka v0.12.7.

The latest version of the moka crate (v0.12.10) has a fix for this problem.

The root cause of the problem is that some laptops with Ryzen CPUs seem to have a problem with the BIOS that causes the TSC (Time Stamp Counter) to become unsynchronized across CPU cores. moka depends on the quanta crate, which uses TSC on x86_64 CPUs to get the current time. If TSC is not synchronized across cores, moka may panic because of the inconsistency of the time. (The time may jump back a few seconds.)

moka v0.12.10 addressed the problem by making quanta crate optional. If the quanta crate is not used, moka will use std::time::Instant instead of TSC. If the quanta crate is used, moka (v0.12.10) only uses quanta where time accuracy is not critical, and (v0.12.9 or later) uses saturated arithmetic to avoid panic. This PR uses the default of the moka crate, which is not to use the quanta crate.

Note that I am the creator of moka and I do not have such a laptop to reproduce the problem. I received a report from a hickory-resolver user that moka (v0.12.8) occasionally panics on his laptop with Ryzen CPU. He confirmed that upgrading moka to v0.12.9 fixed the problem.

Change log of moka crate

(shows only relevant changes)

https://github.com/moka-rs/moka/blob/main/CHANGELOG.md#version-01210

Version 0.12.10

Changed

  • Disable the quanta feature by default. (#482)
  • Replaced most uses of quanta::Instant with std::time::Instant to increase the accuracy of time measurements (#481):
    • When quanta feature is enabled, quanta::Instant is used for some performance critical parts in the cache, and std::time::Instant is used for the rest of the parts.
    • When quanta feature is disabled (default), std::time::Instant is used for all time measurements.

Version 0.12.9

Fixed

  • Prevent an occasional panic in an internal to_std_instant method when per-entry expiration policy is used. (#472)

@CLAassistant
Copy link

CLAassistant commented Jan 7, 2025

CLA assistant check
All committers have signed the CLA.

@tatsuya6502
Copy link
Contributor Author

tatsuya6502 commented Jan 7, 2025

I am not sure why this CI job failed on node_unit_tests::http2_test:
https://github.com/denoland/deno/actions/runs/12642658752/job/35227363098?pr=27572

failed_node_unit_tests_http2_test

I cannot reproduce it locally. (macos aarch64)

passed_node_unit_tests_http2_test

EDIT
Thank you for rerunning the failed and canceled jobs. Now they all passed.

@tatsuya6502 tatsuya6502 changed the title fix(ext/net): Update moka cache to avoid potential panic in Deno.dnsLookup on some laptops with Ryzen CPU fix(ext/net, hickory-resolver): Update moka cache to avoid potential panic in Deno.dnsLookup on some laptops with Ryzen CPU Jan 8, 2025
@tatsuya6502 tatsuya6502 changed the title fix(ext/net, hickory-resolver): Update moka cache to avoid potential panic in Deno.dnsLookup on some laptops with Ryzen CPU fix(ext/net, hickory-resolver): Update moka cache to avoid potential panic in Deno.resolveDns on some laptops with Ryzen CPU Jan 8, 2025
Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@dsherret dsherret changed the title fix(ext/net, hickory-resolver): Update moka cache to avoid potential panic in Deno.resolveDns on some laptops with Ryzen CPU fix(ext/net): update moka cache to avoid potential panic in Deno.resolveDns on some laptops with Ryzen CPU Jan 8, 2025
@dsherret dsherret merged commit 814da49 into denoland:main Jan 8, 2025
17 checks passed
@tatsuya6502 tatsuya6502 deleted the update-moka-cache branch January 8, 2025 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants