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.

Sign up for GitHub

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

Jump to bottom

request timeout does not apply while in Pool queue #341

Closed
ronag opened this issue Aug 18, 2020 · 3 comments · Fixed by #343
Closed

request timeout does not apply while in Pool queue #341

ronag opened this issue Aug 18, 2020 · 3 comments · Fixed by #343
Labels
bug Something isn't working

Comments

@ronag
Copy link
Member

ronag commented Aug 18, 2020 •
edited
Loading

When a request is dispatched on Pool but there is no available client it is put into a queue, the requestTimeout in the current implementation starts from the the request is dispatched on the Client, not when it's in the Pool queue. Bit of an abstraction issue possibly?

Can't think of an elegant way to do this without duplicating code.

Is this something we should fix? @mcollina
@ronag ronag added the bug Something isn't working label Aug 18, 2020
@ronag
Copy link
Member Author

ronag commented Aug 18, 2020 •
edited
Loading

slightly unrelated, TBH I'm not too big of a fan of requestTimeout and I've had developers that didn't read the docs in detail quite surprised by it. They just spam the pipeline queue with requests and just expect it to eventually catch up. As it is now they need to either have another queue or disable the requestTimeout. Maybe we should change default to 0 so that the user has to be explicitly aware of it?

@ronag
Copy link
Member Author

ronag commented Aug 18, 2020 •
edited
Loading

One solution might be to move this logic up one abstraction, i.e. remove it from Client and put it in Pool instead.

@ronag
Copy link
Member Author

ronag commented Aug 19, 2020

I have an idea (semver-major) on how to resolve this

ronag added a commit that referenced this issue Aug 19, 2020
@ronag
fix: requestTimeout monitors from sent
319b35e 
This changes so that requestTimeout measures how long a requests
has been in the pipeline. This is a more useful measure than how
long the request has been enqueued on the client, which is something
that is predictable.

Fixes: #341
@ronag ronag mentioned this issue Aug 19, 2020
Merged
ronag added a commit that referenced this issue Aug 19, 2020
@ronag
fix: requestTimeout monitors from sent
46df119 
This changes so that requestTimeout measures how long a requests
has been in the pipeline. This is a more useful measure than how
long the request has been enqueued on the client, which is something
that is predictable.

Fixes: #341
ronag added a commit that referenced this issue Aug 19, 2020
@ronag
fix: requestTimeout monitors from sent
49fde01 
This changes so that requestTimeout measures how long a requests
has been in the pipeline. This is a more useful measure than how
long the request has been enqueued on the client, which is something
that is predictable.

Fixes: #341
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: requestTimeout monitors from sent nodejs/undici
1 participant
@ronag