HTTP/2 #5659

ogoffart · 2017-03-28T12:50:25Z

We need Qt 5.9 for HTTP2 because, even if Qt 5.8 already has support
for it, there is some critical bug in the HTTP2 implementation which
make it unusable [ https://codereview.qt-project.org/186050 and
https://codereview.qt-project.org/186066 ]

When using HTTP2, we can use many more parallel network request, this
is especially good for small file handling

mention-bot · 2017-03-28T12:50:27Z

@ogoffart, thanks for your PR! By analyzing the history of the files in this pull request, we identified @jturcotte, @ckamm and @krnowak to be potential reviewers.

ogoffart · 2017-03-28T12:51:13Z

I have not tried this on any real http2 server yet. We anyway will have to wait for Qt 5.9
I'm just creating this pull request for reference.

ogoffart · 2017-03-31T09:25:59Z

Another problem i foresee is that this might cause the sycning "long-running" job to make the other short lived control job queued and timeout. I'm thinking about the quota check for the UI, or the connection validator, or the job to expend the selective sync view.

Currently, without HTTP2, we only limit the transfer of long running (big PUT or GET) jobs to 3. But Qt keeps 6 TCP connection open. So the short selective sync jobs happen in parallel and are still finishing well on time.

However, with HTTP2, there is only one connection and the big job might take the whole bandwidth of this connection, meaning that the smaller jobs would be queued and will not finish in time to have a responsible UI.

The solution would be to use higher priority for such jobs. But the Qt API does not support setting a priority on the request, as far as i know.

guruz · 2017-04-04T12:39:56Z

As mentioned on IRC, we need to set maintenance jobs to HighPriority (QNetworkRequest property).

guruz · 2017-04-04T12:40:55Z

We talked to the developer of HTTP2 in Qt, he did not check if certain PUT requests can stall each other, e.g. one request gets all the send window and other requests don't progress.
This would be bad since our timeout handling would trigger for the non-sending PUTs

mrow4a · 2017-04-07T11:56:00Z

src/libsync/owncloudpropagator.cpp

+ if (max)
+ return max;
+ if (_account->isHttp2Supported())
+ return 20;


Only in download or upload also?

That number is for smaller jobs (mkcol, small uploads, small downloads, move, delete)

the maxium amount of longer jobs is still 3

But anyways, it might need some tests probably.

20 sounds scary :-) let's see if the servers can handle it.. @DeepDiver1975 @PVince81 @butonic

CLAassistant · 2017-05-29T08:53:51Z

All committers have signed the CLA.

ogoffart · 2017-05-29T08:57:45Z

Using Qt 5.9 beta, I did some basic tests against an HTTP/2 enabled server, and everything seemed to work.
I did not do any performence testing tough.

mrow4a · 2017-05-29T10:55:48Z

I will happily test it again looking on performance if we would decide to integrate http2 in the client officialy @labkode @moscicki @DeepDiver1975. I can probably look into number of parallel files both for EOS/Standard OC/S3?

ogoffart · 2017-06-06T12:54:36Z

@mrow4a Yes, we integrate HTTP2 in the client officially, and this pull request does it.

Now that Qt 5.9 is released, we could already integrate this patch.

guruz · 2017-06-13T11:15:33Z

Was #5659 (comment) adressed?

ogoffart · 2017-06-13T12:00:13Z

Was #5659 (comment) adressed?

We set a low priority on the PUT and GET jobs:

req.setPriority(QNetworkRequest::LowPriority);

ogoffart · 2017-07-13T08:20:25Z

[Conflicts resolved]

Any objections merging this to master now?

guruz · 2017-07-13T08:27:36Z

src/libsync/accessmanager.cpp

@@ -79,6 +79,15 @@ QNetworkReply *AccessManager::createRequest(QNetworkAccessManager::Operation op,
 if (verb == "PROPFIND") {
 newRequest.setHeader(QNetworkRequest::ContentTypeHeader, QLatin1String("text/xml; charset=utf-8"));
 }
+
+#if QT_VERSION >= QT_VERSION_CHECK(5, 9, 0)


wasn't it 5.9.1 with all necessary bugfixes?

There are necessary bugfixes in 5.9.1.

SamuAlfageme · 2017-07-13T08:31:06Z

@ogoffart are the upload/download propagator low priorities introduced here also applicable for non-HTTP/2 requests?

No questions/objections apart from that - just tested it on macOS with Qt 5.9.1 and everything seem to be smooth.

We might want to start setting a plan to have a stable Qt 5.9 toolchain on the build system.

ogoffart · 2017-07-13T09:57:28Z

@SamuAlfageme Yes, this also apply to Non-HTTP/2 requests, that should not change much actually for we never have more than 6 requests at the same time normally (and if we have, this is actually a good change)

We need Qt 5.9 for HTTP2 because, even if Qt 5.8 already has support for it, there is some critical bug in the HTTP2 implementation which make it unusable [ https://codereview.qt-project.org/186050 and https://codereview.qt-project.org/186066 ] When using HTTP2, we can use many more parallel network request, this is especially good for small file handling Lower the priority of the GET and PUT propagation jobs, so the quota or selective sync ui PROPFIND will not be blocked by them

Qt would otherwise still try to do HTTP/2 connection even over "http://". And that does not work with server that does not support it

ogoffart added the DO NOT MERGE YET label Mar 28, 2017

guruz mentioned this pull request Mar 28, 2017

Qt patches to apply #5618

Closed

ogoffart mentioned this pull request Mar 31, 2017

HTTP2 support #3146

Closed

mrow4a reviewed Apr 7, 2017

View reviewed changes

ogoffart force-pushed the http2 branch from 1effe3b to 15ecc6f Compare April 13, 2017 13:38

ogoffart force-pushed the http2 branch 2 times, most recently from 272c3ee to db22fc5 Compare May 29, 2017 08:53

ogoffart removed the DO NOT MERGE YET label May 29, 2017

ogoffart changed the title ~~WIP: HTTP/2~~ HTTP/2 May 29, 2017

ogoffart force-pushed the http2 branch from db22fc5 to c4f0577 Compare May 29, 2017 08:57

ogoffart force-pushed the http2 branch from c4f0577 to 0b0ff43 Compare June 6, 2017 12:56

mightyBroccoli mentioned this pull request Jun 13, 2017

Will there be http2 compatibility with the 2.3.0 client? nextcloud/client_theming#76

Closed

ogoffart added this to the 2.4.0 milestone Jun 13, 2017

SamuAlfageme self-requested a review June 13, 2017 12:17

ogoffart force-pushed the http2 branch from f6861a7 to 1ea9f26 Compare June 13, 2017 15:11

ogoffart added the ReadyToTest QA, please validate the fix/enhancement label Jun 13, 2017

ogoffart force-pushed the http2 branch from 1ea9f26 to 7164bef Compare July 13, 2017 08:05

guruz reviewed Jul 13, 2017

View reviewed changes

guruz approved these changes Jul 13, 2017

View reviewed changes

ogoffart added 2 commits July 17, 2017 07:55

AccessManager: Disable HTTP/2 without TLS

ce955fc

Qt would otherwise still try to do HTTP/2 connection even over "http://". And that does not work with server that does not support it

ogoffart force-pushed the http2 branch from 7164bef to ce955fc Compare July 17, 2017 05:55

ogoffart merged commit b1363d1 into master Jul 17, 2017

ogoffart deleted the http2 branch July 17, 2017 06:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTTP/2 #5659

HTTP/2 #5659

ogoffart commented Mar 28, 2017

mention-bot commented Mar 28, 2017

ogoffart commented Mar 28, 2017

ogoffart commented Mar 31, 2017

guruz commented Apr 4, 2017

guruz commented Apr 4, 2017

mrow4a Apr 7, 2017 •

edited

Loading

ogoffart Apr 8, 2017

mrow4a Apr 8, 2017

guruz Jul 13, 2017

CLAassistant commented May 29, 2017 •

edited

Loading

ogoffart commented May 29, 2017

mrow4a commented May 29, 2017 •

edited

Loading

ogoffart commented Jun 6, 2017

guruz commented Jun 13, 2017

ogoffart commented Jun 13, 2017

ogoffart commented Jul 13, 2017

guruz Jul 13, 2017

ogoffart Jul 13, 2017

SamuAlfageme commented Jul 13, 2017

ogoffart commented Jul 13, 2017

HTTP/2 #5659

HTTP/2 #5659

Conversation

ogoffart commented Mar 28, 2017

mention-bot commented Mar 28, 2017

ogoffart commented Mar 28, 2017

ogoffart commented Mar 31, 2017

guruz commented Apr 4, 2017

guruz commented Apr 4, 2017

mrow4a Apr 7, 2017 • edited Loading

Choose a reason for hiding this comment

ogoffart Apr 8, 2017

Choose a reason for hiding this comment

mrow4a Apr 8, 2017

Choose a reason for hiding this comment

guruz Jul 13, 2017

Choose a reason for hiding this comment

CLAassistant commented May 29, 2017 • edited Loading

ogoffart commented May 29, 2017

mrow4a commented May 29, 2017 • edited Loading

ogoffart commented Jun 6, 2017

guruz commented Jun 13, 2017

ogoffart commented Jun 13, 2017

ogoffart commented Jul 13, 2017

guruz Jul 13, 2017

Choose a reason for hiding this comment

ogoffart Jul 13, 2017

Choose a reason for hiding this comment

SamuAlfageme commented Jul 13, 2017

ogoffart commented Jul 13, 2017

mrow4a Apr 7, 2017 •

edited

Loading

CLAassistant commented May 29, 2017 •

edited

Loading

mrow4a commented May 29, 2017 •

edited

Loading