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 fetch parameters not being applied correctly #1870

Merged
merged 5 commits into from
Jan 20, 2023
Merged

Fix fetch parameters not being applied correctly #1870

merged 5 commits into from
Jan 20, 2023

Conversation

xconverge
Copy link
Contributor

Fixes #1864

@xconverge
Copy link
Contributor Author

Not sure how this directly undoes #1386 and the test still passes, will look into it and see if I am missing something obvious?

lib/agent.js Outdated
@@ -81,6 +81,10 @@ class Agent extends DispatcherBase {
}
}

options () {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this seems a little hacky, any thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

This would need a bit more support from all the available agents.
Otherwise, I'm good with it.

@mcollina mcollina requested a review from KhafraDev January 20, 2023 08:27
@mcollina
Copy link
Member

@ronag @KhafraDev why is fetch() completely overriding the agent options? I think it'd be best to stick to what is configured in the agent.

(I think I missed #1386, otherwise I would have objected then. It was a busy period of my life.).

@codecov-commenter
Copy link

codecov-commenter commented Jan 20, 2023

Codecov Report

Base: 90.35% // Head: 90.33% // Decreases project coverage by -0.02% ⚠️

Coverage data is based on head (2c0caef) compared to base (2d2512c).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1870      +/-   ##
==========================================
- Coverage   90.35%   90.33%   -0.02%     
==========================================
  Files          70       70              
  Lines        6042     6043       +1     
==========================================
  Hits         5459     5459              
- Misses        583      584       +1     
Impacted Files Coverage Δ
lib/fetch/index.js 84.83% <ø> (ø)
lib/agent.js 100.00% <100.00%> (ø)
lib/fetch/file.js 89.65% <0.00%> (-1.15%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

I think the fix here is simply that fetch doesn't pass it's own timeout values. Why do we override them in fetch?

@KhafraDev
Copy link
Member

KhafraDev commented Jan 20, 2023

They are overridden in fetch to match chrome's behavior. I agree that the fix is to remove them.

edit: #1373

@xconverge
Copy link
Contributor Author

Do I need to adjust any defaults anywhere to preserve behavior or simply delete the 2 lines in lib/fetch/index.js? And remove my 300 sec test that was aiming to enforce the previous feature request?

@KhafraDev
Copy link
Member

KhafraDev commented Jan 20, 2023

removing the 2 lines is fine, the other changes should also be removed

@xconverge
Copy link
Contributor Author

Ok this is ready for a re-review, it was my original changeset as well until I realized it was undo-ing #1373 and #1386

If you want me to add the 300 second tests back in from my previous commit and change the defaults elsewhere in the agent, please let me know, but for now at least the ability to override in fetch exists/works and a test that makes sure of that is in (unless the default gets changed to a much larger number in the future)

@xconverge xconverge requested review from ronag and removed request for KhafraDev January 20, 2023 16:56
@xconverge
Copy link
Contributor Author

Oops sorry I didn't know clicking re-request review on a change request removes the other reviewers

Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

LGTM once test pass

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.

lgtm

lib/agent.js Outdated
@@ -81,6 +81,10 @@ class Agent extends DispatcherBase {
}
}

options () {
Copy link
Member

Choose a reason for hiding this comment

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

This would need a bit more support from all the available agents.
Otherwise, I'm good with it.

@mcollina mcollina merged commit d6ba9e5 into nodejs:main Jan 20, 2023
@xconverge xconverge deleted the testLongRequestTimeout branch January 20, 2023 17:45
@xconverge
Copy link
Contributor Author

@mcollina @ronag what happens if you set bodyTimeout and headersTimeout to 20 minutes, but do NOT set keepAliveTimeout (default is 10 minutes), and a request takes say 15 minutes.

Looking at this code, it kind of looks to me like the request will just die and eventually timeout at the 20 minutes and return a bodyTimeout or headersTimeout. This is what I am experiencing (I know this is wack to have such long requests... but I am starting by just porting this request from axios which works)

    if (this.shouldKeepAlive && client[kPipelining]) {
      const keepAliveTimeout = this.keepAlive ? util.parseKeepAliveTimeout(this.keepAlive) : null

      if (keepAliveTimeout != null) {
        const timeout = Math.min(
          keepAliveTimeout - client[kKeepAliveTimeoutThreshold],
          client[kKeepAliveMaxTimeout]
        )
        if (timeout <= 0) {
          socket[kReset] = true
        } else {
          client[kKeepAliveTimeoutValue] = timeout
        }
      } else {
        client[kKeepAliveTimeoutValue] = client[kKeepAliveDefaultTimeout]
      }
    } else {
      // Stop more requests from being dispatched.
      socket[kReset] = true
    }

I will increase setting keepAliveTimeout but am wondering if there is a more elegant way to handle this so that the errors I get make sense

@mcollina
Copy link
Member

I'm failing to understand how that relates to this PR. If not, please open a new issue.

@xconverge
Copy link
Contributor Author

I'm failing to understand how that relates to this PR. If not, please open a new issue.

Ok fair enough, I have little to no information to put in a new issue yet so was hoping to just get some guidance. I was hopeful that this PR fixed my long request issue and it does not, so to me there is something about undici that I continue to not understand for a long request. I am glad I was able to write a tangible test for this particular issue, so I will see if I can figure out how to write a test case for what I am still experiencing.

@xconverge
Copy link
Contributor Author

This is why I thought it was related, because it allowed me to expose the next issue basically #1887

anonrig pushed a commit to anonrig/undici that referenced this pull request Apr 4, 2023
* Fix default fetch parameters

* Preserve existing behavior with 300 second timeout if not defined

* Add test for 300 second timeout as default

* Cleanup old unused tests

* Simplify how fetch utilizes timeouts from agent
metcoder95 pushed a commit to metcoder95/undici that referenced this pull request Jul 21, 2023
* Fix default fetch parameters

* Preserve existing behavior with 300 second timeout if not defined

* Add test for 300 second timeout as default

* Cleanup old unused tests

* Simplify how fetch utilizes timeouts from agent
crysmags pushed a commit to crysmags/undici that referenced this pull request Feb 27, 2024
* Fix default fetch parameters

* Preserve existing behavior with 300 second timeout if not defined

* Add test for 300 second timeout as default

* Cleanup old unused tests

* Simplify how fetch utilizes timeouts from agent
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.

UND_ERR_HEADERS_TIMEOUT received even when setting a larger headersTimeout in dispatcher for fetch()
5 participants