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

Use HTTP1.1 to download nodejs package #6348

Merged
merged 1 commit into from
Nov 6, 2023

Conversation

crazytonyli
Copy link
Contributor

Issue

Sometimes on CI, we got the following error (here is an example) from installing node:

curl: (92) HTTP/2 stream 0 was not closed cleanly: INTERNAL_ERROR (err 2)

Even though the error is originated from nvm, but it doesn't appear to be an issue in nvm. The same issue appears in n too, which is a similar tool as nvm. And, it's likely an issue in the node.js package CDN (https://nodejs.org/dist).

Changes

I have added a new curlrc option to the nvm plugin, so that we can pass extra curl arguments to the curl commands invoked by nvm. This PR set the new option to --http1.1, which should make nvm downloads nodejs packages using HTTP1.1 instead of HTTP2 protocol, which hopefully would resolve the aforementioned errors on CI.

Test Instructions

This PR can be merged as long as CI jobs pass.

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered if this change warrants user-facing release notes more info and have added them to RELEASE-NOTES.txt if necessary.

@crazytonyli crazytonyli requested review from mokagio and geriux November 2, 2023 01:58
@crazytonyli crazytonyli enabled auto-merge November 2, 2023 01:58
@twstokes twstokes self-requested a review November 2, 2023 02:15
Copy link
Contributor

@twstokes twstokes left a comment

Choose a reason for hiding this comment

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

Makes sense! Thanks @crazytonyli. 🚀

Copy link
Contributor

@mokagio mokagio left a comment

Choose a reason for hiding this comment

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

Let's see how this behaves in prod 👍 Thanks @crazytonyli

@mokagio
Copy link
Contributor

mokagio commented Nov 2, 2023

For some reason, Circle CI didn't run for 63d0270. However, Circle CI run fine for other recent pushes 🤔

image

I think it must have been a hiccup on their end. Not sure how to restart the build, maybe a rebase and force push or a merge of trunk in here?

@crazytonyli crazytonyli force-pushed the use-http-1.1-to-install-nodejs branch from 63d0270 to ad30952 Compare November 2, 2023 20:03
@crazytonyli
Copy link
Contributor Author

@mokagio I have force-pushed but the Circle CI job still didn't run. Is it okay to ignore the CircleCI check on this PR, since only buildkite pipeline is changed?

@mokagio mokagio disabled auto-merge November 6, 2023 02:51
@mokagio
Copy link
Contributor

mokagio commented Nov 6, 2023

@mokagio I have force-pushed but the Circle CI job still didn't run. Is it okay to ignore the CircleCI check on this PR, since only buildkite pipeline is changed?

How odd. Yes. I'll merge this now.

@mokagio mokagio merged commit 1db3b4b into trunk Nov 6, 2023
@mokagio mokagio deleted the use-http-1.1-to-install-nodejs branch November 6, 2023 02:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants