-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
feat(ext/cache): support lscache #27628
base: main
Are you sure you want to change the base?
Conversation
The TLS start sequence has been broken since denoland#26661 because of the way how we wrap TCP handle to create TLS handle. denoland#26661 introduced happy-eyeballs algorithm and some connection could be dropped because of happy-eyeball attempt timeout. The current implementation doesn't consider that case and it could start TLS handshake with timed out TCP connection. That caused denoland#27652 . This PR fixes it by changing the initialization steps. Now `wrapHandle` of TLSSocket set up `afterConnectTls` callback in TCP handle, and `afterConnect` of TCP handle calls it at `connect` event timing if it exists. This avoids starting TLS session with timed out connection. closes denoland#27652
This slightly degrades the performance of CJS export analysis on subsequent runs because I changed it to no longer cache in the DENO_DIR with this PR (denort now properly has no idea about the DENO_DIR). We'll have to change it to embed this data in the binary and that will also allow us to get rid of swc in denort (will do that in a follow-up PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think enablement should happen through a DENO_CACHE_LSC_ENDPOINT
env var and no flag. The env var should be structured as <endpoint>,<token>
. This would allow removing LSC_ENDPOINT
and LSC_TOKEN
.
|
||
impl CacheShard { | ||
pub fn new(endpoint: String, token: String) -> Self { | ||
let client = reqwest::Client::builder() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not use reqwest
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I upstreamed it directly as it was, maybe @losfair can look into rewriting it to use hyper
directly?
No description provided.