-
Notifications
You must be signed in to change notification settings - Fork 541
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
Implement keep-alive max request hint support #1030
Comments
@szmarczak or @dnlup? |
I can take this one. |
This is actually pretty tricky I think, u may send a lot of pipelined requests, without even receiving the first response header with a hint. For example if u open a connection and send 10 piplined requests |
@artur-ma That's actually a good point! The workaround could be to block until we receive the first response. There still may be a situation where we send 10 pipelined requests and in the third one we get max=4. Lines 1009 to 1012 in 20ce79f
Do we really need to use the timeout from https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Keep-Alive
Here, the timeout is handled by the host. If the host decides the connection times out, it simply disconnects us. I think there's no need to handle the logic for host client-side. Maybe I'm missing something... |
Yes you are. The client should not send a request if the server can kill the connection before receiving the message, i.e. there is a race condition here. This is particularly bad with non-idempotent requests. |
Regarding pipelining... I thinking having a request limit close to the pipelining depth is very unlikely in practice. I would be fine with having that as a known and documented limitation. i.e. pipelining is usually < 10 while request limits are usually > 100 |
Having in mind #1031 (review) I think we can close this one? |
If server is limiting number of requests, then piplined requests that send after "connection: close" will be not handled Shouldn't it resend the requests to another connection? E.g I send to a connection 100 request |
We already do this. |
Refs: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Keep-Alive
Refs: nodejs/node#40082
The text was updated successfully, but these errors were encountered: