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

Investigate HTTP Pipelining #4454

Closed
dragotin opened this issue Feb 10, 2016 · 16 comments
Closed

Investigate HTTP Pipelining #4454

dragotin opened this issue Feb 10, 2016 · 16 comments

Comments

@dragotin
Copy link
Contributor

dragotin commented Feb 10, 2016

Please investigate

  • Is HTTP pipelining a strategy to improve the upload and download performance especially with small files
  • Do we enable HTTP pipelining properly in our Qt code? The documentation about QNetworkRequest::HttpPipeliningAllowedAttribute says that pipelining is disabled by default, but we do not set this to true.
  • Does the ownCloud Server support pipelining, or does it require special configuration. If so, which?
  • How can we add a test that verifies that pipelining works?
--- Want to back this issue? **[Post a bounty on it!](https://www.bountysource.com/issues/30721985-investigate-http-pipelining?utm_campaign=plugin&utm_content=tracker%2F216457&utm_medium=issues&utm_source=github)** We accept bounties via [Bountysource](https://www.bountysource.com/?utm_campaign=plugin&utm_content=tracker%2F216457&utm_medium=issues&utm_source=github).
@dragotin dragotin added this to the 2.2-next milestone Feb 10, 2016
@LukasReschke LukasReschke self-assigned this Feb 10, 2016
@guruz
Copy link
Contributor

guruz commented Feb 11, 2016

I implemented pipelining in QNAM back in the days. I don't know how well it works these days given our experiences with recent Qt bugs (and given that it is disabled by default it has less testing).

Pipelining is an attribute of the web server, not of the oC/PHP. Web browsers disable it because of possible problems with "middle boxes" but hopefully our customers all use good software/hardware there. Maybe we should only enable it for HTTPS.

As long as oC has unexpected performance characteristics, we cannot enable pipelining in general because we don't know if there is an actual timeout or not because we don't know how long requests take.

We might however want to enable it for downloading (GET) of small files.
QNAM pipelines 3 requests at once into a socket, so if we assume that they all finish in less time than our GETFileJob timeout we are good.
.oO(maybe QNetworkReply should have a `requestSent`` signal)

@guruz
Copy link
Contributor

guruz commented Feb 11, 2016

QNAM only supports pipelining for GET but I'm sure we could upstream a patch for DELETE and MOVE.

https://code.woboq.org/qt5/qtbase/src/network/access/qhttpnetworkconnection.cpp.html#726

IMHO If we should try to enable this but need to be super careful for things not to break.
.oO(too bad we don't have checksums in general.. @rullzer @ckamm )

@dragotin
Copy link
Contributor Author

@guruz how about PUT?

@guruz
Copy link
Contributor

guruz commented Feb 12, 2016

@dragotin Would need a QNAM patch. The QNAM implementation was done back in the days in a very conservative way with focus on usage in a mobile phone web browser.

@guruz
Copy link
Contributor

guruz commented Feb 23, 2016

Another caveat: QNAM does not do Pipelining when Authorization headers are there. But we currently send this header again, see ef9483c

So maybe we'd need a switch based on the server version which has the cookie-only auth fixed.

Also QNAM might still blindly send the Authorization header because of the QAuthenticator. One idea to fix this would be to call clearAccessCache after going online in ConnectionValidator

@guruz guruz removed this from the 2.2.0-current milestone Feb 25, 2016
@guruz
Copy link
Contributor

guruz commented Feb 25, 2016

Moving this out of 2.2
Will investigate instead of to dynamically adjust the parallelism for smaller requests (together with issue #3382 )

@guruz
Copy link
Contributor

guruz commented Mar 1, 2016

-> #4529

@dragotin
Copy link
Contributor Author

@guruz can this be closed because of the merged #4529 ?

@Marginal
Copy link

Marginal commented May 22, 2016

Please don't close this.

  • There's still a significant potential efficiency gain from using pipelining with lots of small files over a highish latency connection, on top of the benefits provided by Propagator: Pump in more requests if we think current ones are quick #4529.
  • My hosting provider (OwnCube) effectively forbids using the ownCloud client because of the large number of TCP connections that the client can currently make - apparently it looks like a flood DDOS attack to their firewall. Edit: I see that this has come up before.

@danimo
Copy link
Contributor

danimo commented May 22, 2016

@Marginal HTTP/1.1 pipelining has a number of issues and will probably never happen. However, we are investigating HTTP 2.0 support which has implicit pipelining.

@danimo
Copy link
Contributor

danimo commented May 22, 2016

Btw: ownCube had once stated that they would no longer block the official client. Other providers and customers can deal with this without problems.

@Marginal
Copy link

@danimo
Turns out that the slowness and number of TCP/IP connections issues that I was seeing were due to OwnCube running apache with KeepAlive disabled, and nothing to do with the lack of pipelining. Sorry for the noise.

@danimo
Copy link
Contributor

danimo commented May 23, 2016

@Marginal But they now did enable it, did they not?

@Marginal
Copy link

@danimo Yes for my virtual host. Not in general I think.

@guruz
Copy link
Contributor

guruz commented Oct 7, 2016

We probably won't do this and wait for #3146 instead

@guruz
Copy link
Contributor

guruz commented Jun 14, 2017

https://lwn.net/Articles/725293/

"HTTP/1 Pipelining support has been removed in Firefox 54. Maintaining it as we make the move into a new world full of HTTP/2 and other substantial, standardized improvements to networking performance is not worthwhile given pipelining's compatibility and performance issues."

@guruz guruz closed this as completed Jun 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants