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 download timeout on node v20 install #2388

Merged
merged 1 commit into from
Aug 3, 2023

Conversation

xemle
Copy link
Contributor

@xemle xemle commented Jul 31, 2023

This is a fix for #2332 for node v20, see #2332 (comment)

In node v20 the process hangs until the request times out caused by not consumed response data. Users might get the impression that the installation fails. This behavior is not observable for node v18, v16, v14, v12, v10.

From nodes docs:

if a 'response' event handler is added, then the data from the response object must be consumed, either by calling response.read() whenever there is a 'readable' event, or by adding a 'data' handler, or by calling the .resume() method

This PR fixes the installation delay by adding res.resume() calls.

How to reproduce:

Output for

docker run -ti --rm node:18-alpine /bin/sh
mkdir /app
cd /app
npm init -y
time npm i @getgauge/cli

is

# time npm i @getgauge/cli

added 2 packages, and audited 3 packages in 5s

found 0 vulnerabilities
npm notice 
npm notice New minor version of npm available! 9.6.7 -> 9.8.1
npm notice Changelog: https://github.com/npm/cli/releases/tag/v9.8.1
npm notice Run npm install -g npm@9.8.1 to update!
npm notice 
real	0m 5.20s
user	0m 0.81s
sys	0m 0.26s

for node v20, the times are

real	2m 3.35s
user	0m 2.82s
sys	0m 1.14s

@github-actions
Copy link
Contributor

github-actions bot commented Jul 31, 2023

Benchmark Results

java_gradle_serial.csv

Commit CPU Memory Time ExitCode
c93f7af 166% 619168 0:33.39 0
e7c5dc7 6% 119700 0:31.42 0
cbb087a 6% 124600 0:29.64 0
c713df8 7% 127752 0:34.03 0

java_simple_serial.csv

Commit CPU Memory Time ExitCode
c93f7af 163% 341344 0:15.11 0
e7c5dc7 26% 65580 0:18.79 0
cbb087a 28% 65620 0:14.39 0
c713df8 30% 65704 0:13.15 0

java_maven_multithreaded.csv

Commit CPU Memory Time ExitCode
c93f7af 153% 362648 0:37.51 0
e7c5dc7 42% 203644 0:23.39 0
cbb087a 45% 194884 0:22.46 0
c713df8 45% 192780 0:32.52 0

java_maven_serial.csv

Commit CPU Memory Time ExitCode
c93f7af 147% 296200 0:30.25 0
e7c5dc7 50% 209008 0:24.03 0
cbb087a 41% 201340 0:28.75 0
c713df8 51% 197484 0:22.89 0

java_gradle_multithreaded.csv

Commit CPU Memory Time ExitCode
c93f7af 179% 558492 0:33.83 0
e7c5dc7 6% 120660 0:34.02 0
cbb087a 6% 94368 0:36.67 0
c713df8 6% 98104 0:30.64 0

java_simple_multithreaded.csv

Commit CPU Memory Time ExitCode
c93f7af 174% 385536 0:14.13 0
e7c5dc7 26% 66300 0:12.22 0
cbb087a 26% 63600 0:12.15 0
c713df8 24% 65664 0:13.01 0

java_simple_parallel.csv

Commit CPU Memory Time ExitCode
c93f7af 182% 324672 0:20.33 0
e7c5dc7 20% 67916 0:23.75 0
cbb087a 20% 65780 0:23.02 0
c713df8 23% 65888 0:20.86 0

java_gradle_parallel.csv

Commit CPU Memory Time ExitCode
c93f7af 166% 639544 0:30.80 0
e7c5dc7 5% 121060 0:36.79 0
cbb087a 5% 119308 0:48.68 0
c713df8 5% 122696 0:52.41 0

java_maven_parallel.csv

Commit CPU Memory Time ExitCode
c93f7af 158% 341744 0:31.93 0
e7c5dc7 37% 206568 0:42.16 0
cbb087a 37% 199396 0:30.74 0
c713df8 36% 199652 0:41.32 0

Notes

  • The results above are generated by running against seed projects in https://github.com/getgauge/gauge-benchmark
  • These results are not persisted, but on merging to master the benchmark will be rerun.
  • These benchmark are run in Github Actions' agents, which are virtualized. Results are not to be taken as actual benchmarks.Rather, these are indicative numbers and make sense for comparison.

See Workflow log for more details.

zabil
zabil previously approved these changes Aug 1, 2023
@gaugebot
Copy link

gaugebot bot commented Aug 2, 2023

@xemle Thank you for contributing to gauge. Your pull request has been labeled as a release candidate 🎉🎉.

Merging this PR will trigger a release.

Please bump up the version as part of this PR.

Instructions to bump the version can found at CONTRIBUTING.md

If the CONTRIBUTING.md file does not exist or does not include instructions about bumping up the version, please looks previous commits in git history to see what changes need to be done.

Signed-off-by: Sebastian Felis <sebastian@silef.de>
@xemle xemle force-pushed the fix/node-v20-download-timeout branch from 15669fa to 8c5feca Compare August 2, 2023 18:25
@xemle
Copy link
Contributor Author

xemle commented Aug 2, 2023

@zabil thank you very much for marking the PR as release candidate

@zabil zabil merged commit 30a0c23 into getgauge:master Aug 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants