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

HTTP pipelining insufficiently documented #452

Closed
hasufell opened this issue Jan 11, 2021 · 9 comments
Closed

HTTP pipelining insufficiently documented #452

hasufell opened this issue Jan 11, 2021 · 9 comments

Comments

@hasufell
Copy link
Contributor

Manager docs say:

If possible, you should share a single Manager between multiple threads and requests.

What does it mean? What happens if I share a Manager and use forConcurrently to spawn multiple requests? Does it use http pipelining? What does it do? If not, how to do http pipelining?

@snoyberg
Copy link
Owner

snoyberg commented Jan 11, 2021 via email

@hasufell
Copy link
Contributor Author

There’s no mention of pipelining because there is no pipelining

Is it considered an unnecessary feature? Could you expand more on how to do large batches of requests with http-client?

@snoyberg
Copy link
Owner

Let me turn this around. What exactly do you mean by HTTP pipelining? Because the documentation on Manager spells out what it does (keep-alive), and the pipelining question seems like a non-sequitor.

@hasufell
Copy link
Contributor Author

hasufell commented Jan 11, 2021

Let me turn this around. What exactly do you mean by HTTP pipelining?

See https://en.wikipedia.org/wiki/HTTP_pipelining

Because the documentation on Manager spells out what it does (keep-alive), and the pipelining question seems like a non-sequitor.

The questions are simple and clear:

  1. how to do http-pipelining with http-client?
  2. what exactly does Manager do? (there was uncertainty whether that maybe somehow implicitly uses http pipelining)
  3. if the answers to 1. and 2. are "no", then why does http-client not provide means to do pipelining? And if so, maybe that should be added to the documentation.

@snoyberg
Copy link
Owner

This has been a very confusing issue, let me explain why:

  • You asked about HTTP pipelining, despite the fact that the docs never implied that they did pipelining
  • I then confirmed that, in fact, it doesn't do HTTP pipelining
  • You then asked a brand new series of questions about "large batches of requests" and if it's "unnecessary". That's a completely separate kind of question, and implies a bunch of assumptions I don't feel like randomly arguing out with no basis.
  • You then link to an article on HTTP pipelining, implying you've read it. But within the article, it points out "As of 2018, HTTP pipelining is not enabled by default in modern browsers, due to several issues including buggy proxy servers and HOL blocking"
  • You restate three questions, two of which I've already definitively answered above, and then ask a third question, which I'll address next

I'm not going to go on a spurious fact-finding mission here. If you have reason to believe HTTP pipelining should be added: state it. I'm not going to answer what-if questions with hidden implications. I've already stated: the library doesn't do pipelining.

@hasufell
Copy link
Contributor Author

hasufell commented Jan 11, 2021

You asked about HTTP pipelining, despite the fact that the docs never implied that they did pipelining

Yes, and this could be documented.

You then asked a brand new series of questions about "large batches of requests" and if it's "unnecessary". That's a completely separate kind of question, and implies a bunch of assumptions I don't feel like randomly arguing out with no basis.

The whole point of HTTP pipelining is speeding up latency of large batches of requests, no? I'm not sure why you think I'm having some complicated hidden use case here that I'm not willing to share. In fact, if you looked closely at this github issue, there's a link to the project issue exactly explaining the problem (with all the open-source code attached).

You then link to an article on HTTP pipelining, implying you've read it. But within the article, it points out "As of 2018, HTTP pipelining is not enabled by default in modern browsers, due to several issues including buggy proxy servers and HOL blocking"

I'm not developing a browser (most ppl don't), so I'm not sure why this should be relevant. Browsers have completely different concerns. It seems you're assuming this means HTTP pipelining is unnecessary. Well, maybe, maybe not. Does http-client support HTTP2 with multiplexing? Then that may be true.

See https://developer.mozilla.org/en-US/docs/Web/HTTP/Connection_management_in_HTTP_1.x#http_pipelining

For these reasons, pipelining has been superseded by a better algorithm, multiplexing, that is used by HTTP/2.

But afais, it doesn't: #178

If you have reason to believe HTTP pipelining should be added: state it. I'm not going to answer what-if questions with hidden implications. I've already stated: the library doesn't do pipelining.

I was expecting a civil discussion without passive-aggressive comments, that will address the following points:

  1. does HTTP pipelining still make sense today?
  2. if not, how does one optimize large batches of requests for latency without HTTP2?

So I'm not even sure that we need it. I'm looking at options, but the only option seems to be HTTP2.

@snoyberg
Copy link
Owner

Pro-tip: don't use terms like "passive aggressive" if you can't use them correctly, and especially if you're trying to get someone's help.

I've already stated: the library doesn't do HTTP pipelining, and you haven't given me a reason to think it needs to. You haven't justified that "optimizing large batches of requests" is something that needs to happen, and I'm not going to spend my time reading another issue on a different issue tracker to try to justify that.

@rvl
Copy link

rvl commented Jan 12, 2021

My apologies @snoyberg - we were a bit loose with terms and got "pipelining" and Connection: Keep-Alive mixed up. The latter is probably what we want, and I can see that the http-client connection manager correctly keeps connections open for a short time, unless the server closes the connection.

@snoyberg
Copy link
Owner

No worries @rvl, thanks for confirming

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

No branches or pull requests

3 participants