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

disable keep alive timeout, headers timeout and log any server timeouts #85

Merged
merged 3 commits into from Jun 25, 2020

Conversation

blushingpenguin
Copy link
Contributor

This cures my issue with randomly aborted connections (#60). I tried to write a test for this, but it's actually hard to reproduce -- the easiest way to elicit this was to debug the node process while making multiple concurrent connections to it.

I think what's happening is that you get a .NET connection pooled, connected http connection which node then closes when data is sent to it as in:

nodejs/node#26165

also as per this PR:

nodejs/node#33307

the headers timeout cannot be disabled (but it can be set to a very long value, which is what I've done). I also disabled the keepalive timeout for sockets as that's another possible reason that node is dropping the pooled connections.

@JeremyTCD JeremyTCD added the bug Something isn't working label Jun 24, 2020
@codecov
Copy link

codecov bot commented Jun 24, 2020

Codecov Report

Merging #85 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #85   +/-   ##
=======================================
  Coverage   97.03%   97.03%           
=======================================
  Files          23       23           
  Lines         809      809           
=======================================
  Hits          785      785           
  Misses         24       24           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5956ab2...259780e. Read the comment docs.

@JeremyTCD
Copy link
Member

Looks promising, will review tomorrow.

@JeremyTCD
Copy link
Member

JeremyTCD commented Jun 25, 2020

Released 5.4.2 🚀.

Changes look good, did some reading up, disabling these timeouts certainly will not hurt. Might even reduce latency by keeping connections alive. Logging unexpected timeouts is an excellent addition.

No worries about tests, haven't been able to reproduce #60 either. Though will leave #60 open since we can't prove conclusively that the premature response issue is resolved.

Thanks again for contributing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants