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

lib: adding keep-alive header to download requests #1863

Closed
wants to merge 1 commit into from
Closed

lib: adding keep-alive header to download requests #1863

wants to merge 1 commit into from

Conversation

miladfarca
Copy link

@miladfarca miladfarca commented Aug 23, 2019

Checklist
Description of change

We have been seeing random failures on AIX in the CI when building node-addon-api:
https://ci.nodejs.org/job/node-test-node-addon-api-new/nodes=aix61-ppc64/481/console

Taking a closer look shows the build fails when gyp is trying to download and extract node source tarball. This is done using a stream, data gets decompressed as its being downloaded:

...
gyp verb extracted file from tarball node-v13.0.0-nightly20190822775048d54c/include/node/openssl/opensslconf_no-asm.h
gyp verb extracted file from tarball node-v13.0.0-nightly20190822775048d54c/include/node/zconf.h
Error: read ECONNRESET
    at TLSWrap.onStreamRead (internal/stream_base_commons.js:183:27) {
  errno: -73,
  code: 'ECONNRESET',
  syscall: 'read'
}
gyp WARN install got an error, rolling back install
...

I came up with a simpler test case to investigate the cause:

const https = require('https');
var tar = require('tar');

const request = https.get("https://nodejs.org/download/nightly/v13.0.0-nightly20190822775048d54c/node-v13.0.0-nightly20190822775048d54c-headers.tar.gz", function(response) {
    response.pipe(tar.extract({
        strip: 1
    }));
});

Running the above on AIX using node test.js causes the same error to appear. It seemed like
the connection was being interrupted while stream was being decompressed.

I noticed requests are being handled by cloudflare. I tried moving the headers.tar.gz file to my personal server with https enabled and then ran the script. It worked with no errors.

I figured cloudflare is most likely dropping the TLS connection once it doesn't hear a tick which happens on AIX as the live tar extraction is interfering with it and is taking longer than other platforms.

I looked at the response headers from cloudflare and the default connection header is set to close. Adding a keep-alive header in my request managed to fix the problem and force a live connection until the stream is done. I also tested this with node-addon-api by modifying gyp and the headers were downloaded successfully on AIX.

I have found a similar ticket from 2017 whit the same issue which might have been caused by cloudflare dropping connections:
#1084

@miladfarca
Copy link
Author

/cc @nodejs/platform-ppc

@rvagg
Copy link
Member

rvagg commented Aug 25, 2019

I don't really get this, it's just performing one request so keep-alive isn't technically necessary, do you think that turning it on is changing the cloudflare timeouts?

@miladfarca
Copy link
Author

miladfarca commented Aug 26, 2019

I'm suspecting there is a timeout which this header is disabling and server continues with sending the stream.

@rvagg
Copy link
Member

rvagg commented Aug 28, 2019

Well, it's odd, but I guess it can't hurt, so maybe we should give it a go. Any objections @nodejs/node-gyp?

@rvagg
Copy link
Member

rvagg commented Sep 25, 2019

landed in 0384683, let's cross fingers and see how this goes

@rvagg rvagg closed this Sep 25, 2019
rvagg pushed a commit that referenced this pull request Sep 26, 2019
PR-URL: #1863
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@rvagg rvagg mentioned this pull request Sep 26, 2019
rvagg pushed a commit that referenced this pull request Sep 26, 2019
PR-URL: #1863
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@rvagg rvagg mentioned this pull request Sep 26, 2019
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