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: set agentkeepalive freeSocketTimeout back to 15 seconds #100

Merged
merged 1 commit into from
Feb 15, 2022

Conversation

nlf
Copy link
Contributor

@nlf nlf commented Feb 15, 2022

this is a bit of a bandaid for the timeout issue that got introduced here recently. it makes our tests pass consistently again without timeouts, but it does also indicate some potential issues in minipass-fetch. or at least issues between minipass-fetch and agentkeepalive..

either way, this would be great to land to get our CI happy again and to stop causing other peoples tests to potentially fail (node-gyp saw this issue on their end as well) and we can move on to figuring out how to fix the root cause later.

@nlf nlf marked this pull request as ready for review February 15, 2022 16:51
@nlf nlf requested a review from a team as a code owner February 15, 2022 16:51
@wraithgar
Copy link
Member

attn @rvagg in re nodejs/node-gyp#2607. This looks too be the culprit. When this lands I'll make a pr into node-gyp

@nlf nlf merged commit 3371abf into main Feb 15, 2022
@nlf nlf deleted the nlf/socket-timeout branch February 15, 2022 16:55
@darcyclarke
Copy link
Contributor

Note: for those following along at home, the initial change in behavior was caused by agentkeepalive updating their default timeout values, to a significantly lower threshold, in a minor version (ie. make-fetch-happen picked up the latest version of that dep which had breaking changes) - ref. https://github.com/node-modules/agentkeepalive/blob/master/History.md#420--2021-12-31 (pr: node-modules/agentkeepalive#102 & commit: node-modules/agentkeepalive@f418c67).

As @nlf notes, we'll want to determine the root-cause/need for such a high threshold but that may take some time.

@rvagg
Copy link

rvagg commented Feb 16, 2022

any chance this might have windows-specific behaviours as well? we're still seeing high-frequency flakies on windows even with this change: https://github.com/nodejs/node-gyp/actions (can't be sure make-fetch-happen is to blame, but the regularity coincides with the v10 upgrade).

@nlf
Copy link
Contributor Author

nlf commented Feb 16, 2022

any chance this might have windows-specific behaviours as well? we're still seeing high-frequency flakies on windows even with this change: https://github.com/nodejs/node-gyp/actions (can't be sure make-fetch-happen is to blame, but the regularity coincides with the v10 upgrade).

yeah i saw that in the pull request i submitted. i'll keep digging. our own CI stabilized with this one so i was really hopeful it would take care of the problem for you too. i'll make sure we send you another pull request when we get it figured out

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.

4 participants