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

Resend network requests on ContentReSendError #7174

Closed
ckamm opened this issue May 3, 2019 · 9 comments
Closed

Resend network requests on ContentReSendError #7174

ckamm opened this issue May 3, 2019 · 9 comments
Assignees
@ckamm
Labels
Enhancement p3-medium Normal priority ReadyToTest QA, please validate the fix/enhancement
Milestone
2.6.0

Comments

@ckamm
Copy link
Contributor

ckamm commented May 3, 2019 •
edited
Loading

Now that https://codereview.qt-project.org/#/c/254111/ is merged and will apear in Qt 5.12.4 we can prepare the client by catching the error and doing a manual resend. This will allow us to re-enable HTTP2 support once that Qt is available.
@ckamm ckamm added this to the 2.6.0 milestone May 3, 2019
@ckamm ckamm self-assigned this May 3, 2019
@ckamm ckamm added the p3-medium Normal priority label Jun 4, 2019
@ogoffart ogoffart modified the milestones: 2.6.0, 2.6.1 Jun 4, 2019
@ogoffart
Copy link
Contributor

ogoffart commented Jun 4, 2019

(moved to 2.6.1 since Qt 5.12.4 is not yet released)

@guruz guruz mentioned this issue Jun 5, 2019
Closed
ckamm added a commit that referenced this issue Jun 6, 2019
@ckamm
Http2: Resend requests on ContentReSend error #7174
4d3a812 
Since Qt does not yet transparently resend HTTP2 requests in some cases
we do it manually.

The test showed a problem where the initial non-200 reply would close
the target temporary file and the follow-up request couldn't store any
data. Removing that close() call is safe because there also is a
_saveBodyToFile flag that guards writes to the target file.
@ckamm ckamm mentioned this issue Jun 6, 2019
Merged
@ckamm ckamm modified the milestones: 2.6.1, 2.5.5 Jun 6, 2019
ckamm added a commit that referenced this issue Jun 6, 2019
@ckamm
Http2: Resend requests on ContentReSend error #7174
ea0bfda 
Since Qt does not yet transparently resend HTTP2 requests in some cases
we do it manually.

The test showed a problem where the initial non-200 reply would close
the target temporary file and the follow-up request couldn't store any
data. Removing that close() call is safe because there also is a
_saveBodyToFile flag that guards writes to the target file.
ckamm added a commit that referenced this issue Jun 6, 2019
@ckamm
Http2: Resend requests on ContentReSend error #7174
6e39a5c 
Since Qt does not yet transparently resend HTTP2 requests in some cases
we do it manually.

The test showed a problem where the initial non-200 reply would close
the target temporary file and the follow-up request couldn't store any
data. Removing that close() call is safe because there also is a
_saveBodyToFile flag that guards writes to the target file.
ckamm added a commit that referenced this issue Jun 7, 2019
@ckamm
HTTP2: Enable if using Qt >=5.12.4 #7092
600ef2e 
This Qt version fixes QTBUG-73947 and along with the resend workaround
for #7174 HTTP2 should work more reliably.
@ckamm ckamm mentioned this issue Jun 7, 2019
Merged
@ckamm ckamm modified the milestones: 2.5.5, 2.6.0 Jun 7, 2019
ckamm added a commit that referenced this issue Jun 7, 2019
@ckamm
Http2: Resend requests on ContentReSend error #7174
442c290 
Since Qt does not yet transparently resend HTTP2 requests in some cases
we do it manually.

The test showed a problem where the initial non-200 reply would close
the target temporary file and the follow-up request couldn't store any
data. Removing that close() call is safe because there also is a
_saveBodyToFile flag that guards writes to the target file.
ckamm added a commit that referenced this issue Jun 7, 2019
@ckamm
Http2: Resend requests on ContentReSend error #7174
677e44d 
Since Qt does not yet transparently resend HTTP2 requests in some cases
we do it manually.

The test showed a problem where the initial non-200 reply would close
the target temporary file and the follow-up request couldn't store any
data. Removing that close() call is safe because there also is a
_saveBodyToFile flag that guards writes to the target file.
ckamm added a commit that referenced this issue Jun 7, 2019
@ckamm
HTTP2: Enable if using Qt >=5.12.4 #7092
fd523c3 
This Qt version fixes QTBUG-73947 and along with the resend workaround
for #7174 HTTP2 should work more reliably.
@ckamm ckamm added ReadyToTest QA, please validate the fix/enhancement and removed PR available labels Jun 7, 2019
@ckamm
Copy link
Contributor Author

ckamm commented Jun 7, 2019

This is extremely hard to test, to the point where I would skip it. One needs to run with 5.12.4 and have a server that's enabled for http2 and sends GOAWAY frames in the way that triggers the Qt bug that used to exist.

ckamm added a commit that referenced this issue Jun 12, 2019
@ckamm
Http2: Resend requests on ContentReSend error #7174
98ba64e 
Since Qt does not yet transparently resend HTTP2 requests in some cases
we do it manually.

The test showed a problem where the initial non-200 reply would close
the target temporary file and the follow-up request couldn't store any
data. Removing that close() call is safe because there also is a
_saveBodyToFile flag that guards writes to the target file.

(cherry picked from commit 677e44d)

Cherry-picked to allow 2.5 users with new enough Qt to test the HTTP2
workaround.
@HanaGemela
Copy link
Contributor

@ckamm How come all the platforms are using different version of Qt?
macOS uses 5.12.1
Window uses Qt 5.12.4
Ubuntu uses Qt 5.12.2

Does this need testing on lower version than 5.12.4?

@ckamm
Copy link
Contributor Author

ckamm commented Jul 23, 2019 •
edited
Loading

@HanaGemela I don't know the exact reason. (@dschmidt ?)

This enhancement can only be tested with Qt >=5.12.4, can't test it with an earlier version. The client will also not use HTTP2 with Qt versions before that by default. Can be overridden with OWNCLOUD_HTTP2_ENABLED=1. But that's not recommended because we know there are issues.

@HanaGemela
Copy link
Contributor

@ckamm does that mean that the client with Qt 5.12.4 has HTTP2 enabled by default?
What if I'll connect it to the server that does not support it?
Do you know how to send GOAWAY frames in the way that triggers the Qt bug?

@ckamm
Copy link
Contributor Author

ckamm commented Jul 24, 2019

@HanaGemela Yes, a 2.6.0 client with Qt >= 5.12.4 will check whether a server supports http2 and use it if possible. If the server doesn't support it will work as before.

GOAWAY: I think this nginx config triggered the issue, but only sometimes:

http2_max_concurrent_streams 2;
http2_max_requests 2;
http2_idle_timeout 3m;

@HanaGemela
Copy link
Contributor

Closing as exact steps to recreate are not known.

@guruz
Copy link
Contributor

guruz commented Aug 5, 2019

@ckamm How come all the platforms are using different version of Qt?

The Qt version is defined by the build systems not by the client source code :-)
We try to get this all to 5.12.4 though

@guruz
Copy link
Contributor

guruz commented Sep 11, 2019

@ckamm Please evaluate https://codereview.qt-project.org/c/qt/qtbase/+/272754 when you have the time..

@papjul papjul mentioned this issue Oct 1, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ckamm ckamm

Labels
Enhancement p3-medium Normal priority ReadyToTest QA, please validate the fix/enhancement
Projects
None yet
Milestone
2.6.0
Development

No branches or pull requests
4 participants
@ckamm @ogoffart @guruz @HanaGemela