-
Notifications
You must be signed in to change notification settings - Fork 236
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: Silence ECONNRESET errors after connection close #392
Conversation
Thanks! Are you able to run all tests successfully? |
I get some test failures, but it's the same tests with and without my change:
|
@tomas, since the test failures I'm seeing do not appear to be regressions, can we merge this change? Less.js is waiting on this fix to update their dependency. Without this fix, less v4 is broken on macOS. |
Yes sure. Thanks for the heads up. |
This seems to fix the issue, but I'm not sure if it solves #392. This might also be relevant: I don't remember exactly why I commented that part of the code, but it seems related. |
I'll test on mac and see if the issue is still fixed with that change. |
It looks like that change would reintroduce the ECONNRESET bug. If you disable my fix, the bug I'm fixing seems to be triggered by one of the existing tests on macOS, FWIW:
I don't see any of the socket reuse tests failing, for some reasons, neither on macOS nor on Linux. But I see many test failures with or without my changes, and they are inconsistent and different across operating systems. Does limiting my workaround to macOS solve the test failure for you? (Since I can't reproduce the failure, I can't say if this is helpful or not, but I'm assuming we have some OS-specific difference at play.) if (process.platform == 'darwin') request.abort(); |
I filed issues for the tests that fail on my mac and Linux workstations with node v16, and I uploaded PRs to fix them. I'll try node v12 on Linux via nvm and see if I can reproduce that reuse issue. |
Yup, there it is. Reverting this fixes it. nvm exec 12 node node_modules/.bin/mocha test |
I have PRs up to fix all the various test failures I've seen, including the reuse issue you found. With those, I have tests passing on Linux and macOS on node v4, v6, v8, v10, v12, v14, v16, and v17. |
The issue was traced upstream to needle, and resolved in: - tomas/needle#392 - tomas/needle#394 - tomas/needle#396 - tomas/needle#398 Closes less#3693
The issue was traced upstream to needle, and resolved in: - tomas/needle#392 - tomas/needle#394 - tomas/needle#396 - tomas/needle#398 Closes #3693
The issue was traced upstream to needle, and resolved in: - tomas/needle#392 - tomas/needle#394 - tomas/needle#396 - tomas/needle#398 Closes less#3693
See also less/less.js#3693
Fixes #391