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: add connect timeout #619

Merged
merged 8 commits into from
Mar 27, 2021
Merged

fix: add connect timeout #619

merged 8 commits into from
Mar 27, 2021

Conversation

ronag
Copy link
Member

@ronag ronag commented Mar 24, 2021

Haven't been able to build a test case for this but in production I have case where neither 'connect' not 'error' is invoked on a socket after socket.connect().

@ronag ronag added this to the 4.0 milestone Mar 24, 2021
@ronag ronag requested a review from mcollina March 24, 2021 23:04
@ronag ronag changed the title @ronag fix: add connect timeout fix: add connect timeout Mar 24, 2021
@codecov-io
Copy link

codecov-io commented Mar 25, 2021

Codecov Report

Merging #619 (1381b5e) into main (56bd998) will increase coverage by 0.60%.
The diff coverage is 61.11%.

❗ Current head 1381b5e differs from pull request most recent head 5b3d2f4. Consider uploading reports for the commit 5b3d2f4 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #619      +/-   ##
==========================================
+ Coverage   98.03%   98.63%   +0.60%     
==========================================
  Files          17       17              
  Lines        1574     1463     -111     
==========================================
- Hits         1543     1443     -100     
+ Misses         31       20      -11     
Impacted Files Coverage Δ
lib/core/errors.js 92.75% <0.00%> (-7.25%) ⬇️
lib/core/symbols.js 100.00% <ø> (ø)
lib/core/client.js 98.82% <84.61%> (+0.04%) ⬆️
lib/llhttp/parser.js
lib/node/http-parser.js 100.00% <0.00%> (ø)
lib/core/util.js 96.05% <0.00%> (+7.89%) ⬆️

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 56bd998...5b3d2f4. Read the comment docs.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would really prefer if this had a test.

Does it worsen our benchmarks?

lib/core/errors.js Show resolved Hide resolved
@mcollina
Copy link
Member

Should this have a default as it can happen in reality?

Co-authored-by: Matteo Collina <hello@matteocollina.com>
@ronag
Copy link
Member Author

ronag commented Mar 25, 2021

Should this have a default as it can happen in reality?

It has a default. 10s.

@ronag
Copy link
Member Author

ronag commented Mar 25, 2021

Should this have a default as it can happen in reality?

Any idea on how to reproduce this scenario? When I try I always get an error. Maybe if I monkeypatch the global socket prototype :/?

@ronag
Copy link
Member Author

ronag commented Mar 25, 2021

Does it worsen our benchmarks?

No.

@dnlup
Copy link
Contributor

dnlup commented Mar 25, 2021

Should this have a default as it can happen in reality?

Any idea on how to reproduce this scenario? When I try I always get an error. Maybe if I monkeypatch the global socket prototype :/?

Can we tap into some net.Server hook and delay the connect event?

Nvm, I don't see anything public we can use server-side to delay the connect event client side, I don't know what I was thinking.

@mcollina
Copy link
Member

Any idea on how to reproduce this scenario? When I try I always get an error. Maybe if I monkeypatch the global socket prototype :/?

Go for it in its own test file.

@ronag
Copy link
Member Author

ronag commented Mar 27, 2021

Added test

@ronag ronag merged commit d056c20 into main Mar 27, 2021
@Uzlopak Uzlopak deleted the connect-timeut branch February 21, 2024 12:39
crysmags pushed a commit to crysmags/undici that referenced this pull request Feb 27, 2024
* fix: add connect timeout

* fixuP

* fixup

* fixup

* fixuP

* Update lib/core/errors.js

Co-authored-by: Matteo Collina <hello@matteocollina.com>

* fixup: add test

Co-authored-by: Matteo Collina <hello@matteocollina.com>
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.

None yet

4 participants