Large parallel POST requests in H2 stop uploading and eventually time-out |
||||||||||||
Issue descriptionChrome Version: 63 OS: OSX 10.12.6 What steps will reproduce the problem? (1) Reproduction happens behind a login, so cannot post a link here :/ (2) Net logs - I'll send logs on demand once I get customer approval for it. Unfortunately I cannot post them here as well :| This issue happens in a wordpress web app, which uploads large chunks of data. In the logs we can see a couple of large POSTs that happen in parallel, and one of them gets stalled. I believe that the issue is the result of failed resumption in SpdySession::ResumeSendStalledStreams. All the streams are being taken out of the queue, even if some of them failed to resume POSTing (due to the session being stalled because another POST stream have stalled it). I have a local fix for this issue, but I'm not sure it's the *right* fix. I'll upload it for discussion shortly.
,
Sep 5 2017
Thanks to Tim Vereecke[1], we now have a non-customer reproduction scenario! I'm uploading the netlog for that scenario. I can provide the reproduction instructions on demand. In those logs, you can see an HTTP2_SESSION (on 31573), where stream id 43 gets stalled, never resumes sending and eventually 503s. The proposed fix works here as well. [1] https://twitter.com/TimVereecke/status/905003165673775104
,
Sep 5 2017
,
Sep 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/baf6f3555a8c44310269bcd9c3eeec027698d406 commit baf6f3555a8c44310269bcd9c3eeec027698d406 Author: Yoav Weiss <yoav@yoav.ws> Date: Wed Sep 06 05:04:15 2017 Fix hanged large parallel POSTS in H2 This CL makes sure that streams that get stalled on send are being put back in the queue if they cannot send when attempting to unstall them. Bug: 761919 Change-Id: If8f948b83b2d37e80b1cbe24aa8a3be6c8068441 Reviewed-on: https://chromium-review.googlesource.com/649849 Reviewed-by: Bence Béky <bnc@chromium.org> Commit-Queue: Yoav Weiss <yoav@yoav.ws> Cr-Commit-Position: refs/heads/master@{#499883} [modify] https://crrev.com/baf6f3555a8c44310269bcd9c3eeec027698d406/net/spdy/chromium/spdy_session.cc [modify] https://crrev.com/baf6f3555a8c44310269bcd9c3eeec027698d406/net/spdy/chromium/spdy_stream.cc [modify] https://crrev.com/baf6f3555a8c44310269bcd9c3eeec027698d406/net/spdy/chromium/spdy_stream.h
,
Sep 6 2017
As this is a critical issue for some of our customers, I'd like to try and merge it to Chrome 62. (assuming it's too late for a Chrome 61 merge)
,
Sep 6 2017
I have a unittest in progress at https://crrev.com/c/651188, that should also be merged in case merging https://crrev.com/c/649849 is approved.
,
Sep 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/61e123b1719510bd7c966dce1b3eb304d426008a commit 61e123b1719510bd7c966dce1b3eb304d426008a Author: Bence Béky <bnc@chromium.org> Date: Wed Sep 06 16:08:39 2017 Add regression test for https://crrev.com/c/649849. Both the author of https://crrev.com/c/649849 and myself verified locally that this test fails without that CL but passes with it. BUG= 761919 Change-Id: I769f6b3ca015b468427f33705dfc5194a184f0d5 Reviewed-on: https://chromium-review.googlesource.com/651188 Commit-Queue: Bence Béky <bnc@chromium.org> Reviewed-by: Helen Li <xunjieli@chromium.org> Cr-Commit-Position: refs/heads/master@{#499978} [modify] https://crrev.com/61e123b1719510bd7c966dce1b3eb304d426008a/net/spdy/chromium/spdy_session_unittest.cc
,
Sep 6 2017
Closing as fixed, as the test also landed
,
Sep 7 2017
,
Sep 7 2017
Your change meets the bar and is auto-approved for M62. Please go ahead and merge the CL to branch 3202 manually. Please contact milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Sep 11 2017
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible! If all merges have been completed, please remove any remaining Merge-Approved labels from this issue. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Sep 11 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f1b7017a5805df6c1ca01eb4980b11e1a4e314e9 commit f1b7017a5805df6c1ca01eb4980b11e1a4e314e9 Author: Yoav Weiss <yoav@yoav.ws> Date: Mon Sep 11 16:33:29 2017 Fix hanged large parallel POSTS in H2 This CL makes sure that streams that get stalled on send are being put back in the queue if they cannot send when attempting to unstall them. Bug: 761919 Change-Id: If8f948b83b2d37e80b1cbe24aa8a3be6c8068441 Reviewed-on: https://chromium-review.googlesource.com/649849 Reviewed-by: Bence Béky <bnc@chromium.org> Commit-Queue: Yoav Weiss <yoav@yoav.ws> Cr-Original-Commit-Position: refs/heads/master@{#499883}(cherry picked from commit baf6f3555a8c44310269bcd9c3eeec027698d406) Reviewed-on: https://chromium-review.googlesource.com/658037 Reviewed-by: Yoav Weiss <yoav@yoav.ws> Cr-Commit-Position: refs/branch-heads/3202@{#123} Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098} [modify] https://crrev.com/f1b7017a5805df6c1ca01eb4980b11e1a4e314e9/net/spdy/chromium/spdy_session.cc [modify] https://crrev.com/f1b7017a5805df6c1ca01eb4980b11e1a4e314e9/net/spdy/chromium/spdy_stream.cc [modify] https://crrev.com/f1b7017a5805df6c1ca01eb4980b11e1a4e314e9/net/spdy/chromium/spdy_stream.h
,
Sep 11 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c941e5a14dccd6f06678619ca3e40a8401e4908e commit c941e5a14dccd6f06678619ca3e40a8401e4908e Author: Bence Béky <bnc@chromium.org> Date: Mon Sep 11 16:33:46 2017 Add regression test for https://crrev.com/c/649849. Both the author of https://crrev.com/c/649849 and myself verified locally that this test fails without that CL but passes with it. BUG= 761919 Change-Id: I769f6b3ca015b468427f33705dfc5194a184f0d5 Reviewed-on: https://chromium-review.googlesource.com/651188 Commit-Queue: Bence Béky <bnc@chromium.org> Reviewed-by: Helen Li <xunjieli@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#499978}(cherry picked from commit 61e123b1719510bd7c966dce1b3eb304d426008a) Reviewed-on: https://chromium-review.googlesource.com/659537 Reviewed-by: Yoav Weiss <yoav@yoav.ws> Cr-Commit-Position: refs/branch-heads/3202@{#124} Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098} [modify] https://crrev.com/c941e5a14dccd6f06678619ca3e40a8401e4908e/net/spdy/chromium/spdy_session_unittest.cc |
||||||||||||
►
Sign in to add a comment |
||||||||||||
Comment 1 by y...@yoav.ws
, Sep 5 2017