Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2856073003:
Fix QuicNetworkTransactionTest.RetryMisdirectedRequest (Closed)
DescriptionFix QuicNetworkTransactionTest.RetryMisdirectedRequest
This test was depending on the SSL deprecated cipher fallback (which is
a quirk to probe server cipher preferences and not something QUIC may
depend on) to clear away the ERR_CONNECTION_CLOSED when it shouldn't
have been surfacing one to begin with.
Fortunately, the actual logic isn't relying on this. The test just needs
fixing. Rather than scheduling three connections, schedule two. The main
job hangs until after all the QUIC machinery is done, then we unblock
the main job and let that complete.
BUG=546991, 684730
Review-Url: https://codereview.chromium.org/2856073003
Cr-Commit-Position: refs/heads/master@{#469799}
Committed: https://chromium.googlesource.com/chromium/src/+/a4449725c3c1d07ecd8008d5d03227c4588b85dc
Patch Set 1 #
Total comments: 4
Patch Set 2 : rch comment #Messages
Total messages: 17 (9 generated)
The CQ bit was checked by davidben@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
davidben@chromium.org changed reviewers: + bnc@chromium.org, rch@chromium.org, zhongyi@chromium.org
I don't think the paused MockConnect ended up being necessary at all, but I opted for keeping a tight control on the timing to make sure future changes to the main/alternate job logic doesn't mess things up.
On 2017/05/03 22:50:26, davidben wrote: > I don't think the paused MockConnect ended up being necessary at all, but I > opted for keeping a tight control on the timing to make sure future changes to > the main/alternate job logic doesn't mess things up. LGTM. Thank you for fixing the mess I made.
lgtm, I think, but I'm confused about one of the comments. https://codereview.chromium.org/2856073003/diff/1/net/quic/chromium/quic_netw... File net/quic/chromium/quic_network_transaction_unittest.cc (right): https://codereview.chromium.org/2856073003/diff/1/net/quic/chromium/quic_netw... net/quic/chromium/quic_network_transaction_unittest.cc:1069: // deterministic. The first main job gets aborted with no socket. Hm. This comment seems to say: 1) The main job attempts to call Connect() on a socket, but that hangs. 2) The main job is aborted without ever getting a socket. I don't think I understand how to reconcile that.
https://codereview.chromium.org/2856073003/diff/1/net/quic/chromium/quic_netw... File net/quic/chromium/quic_network_transaction_unittest.cc (right): https://codereview.chromium.org/2856073003/diff/1/net/quic/chromium/quic_netw... net/quic/chromium/quic_network_transaction_unittest.cc:1069: // deterministic. The first main job gets aborted with no socket. On 2017/05/03 23:20:39, Ryan Hamilton wrote: > Hm. This comment seems to say: > > 1) The main job attempts to call Connect() on a socket, but that hangs. > 2) The main job is aborted without ever getting a socket. > > I don't think I understand how to reconcile that. (1) isn't quite true. The socket pools call Connect() but never vend out sockets until they succeed. Another way to write this test would be to have three sockets and guarantee that the second socket is bound to the main job before the main job is destroyed (and thus the comment about "there is no open TCP socket" would be true), but I didn't see a way to do it. Honestly, most of this mess isn't necessary because you all delay the alternate job by 300ms, meanwhile the QUIC job completes in a few PostTasks (not quite synchronously though). I believe the test works just fine if you just remove failing_data and leave http_data alone. But it seemed poor to me to depend on how the two jobs race. I dunno, what do you all think?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2856073003/diff/1/net/quic/chromium/quic_netw... File net/quic/chromium/quic_network_transaction_unittest.cc (right): https://codereview.chromium.org/2856073003/diff/1/net/quic/chromium/quic_netw... net/quic/chromium/quic_network_transaction_unittest.cc:1069: // deterministic. The first main job gets aborted with no socket. On 2017/05/03 23:33:13, davidben wrote: > On 2017/05/03 23:20:39, Ryan Hamilton wrote: > > Hm. This comment seems to say: > > > > 1) The main job attempts to call Connect() on a socket, but that hangs. > > 2) The main job is aborted without ever getting a socket. > > > > I don't think I understand how to reconcile that. > > (1) isn't quite true. The socket pools call Connect() but never vend out sockets > until they succeed. Another way to write this test would be to have three > sockets and guarantee that the second socket is bound to the main job before the > main job is destroyed (and thus the comment about "there is no open TCP socket" > would be true), but I didn't see a way to do it. Ah. Ok, I think the test is fine. (no surprise there) but I think the last sentence in the comment could perhaps be clarified to mention that "it gets aborted without the socket pool ever dispensing the socket" or some such. > Honestly, most of this mess isn't necessary because you all delay the alternate > job by 300ms, meanwhile the QUIC job completes in a few PostTasks (not quite > synchronously though). I believe the test works just fine if you just remove > failing_data and leave http_data alone. But it seemed poor to me to depend on > how the two jobs race. I dunno, what do you all think? That's possible. The delay-tcp-race logic was not on by default until just recently. I'd be inclined to leave it be, but perhaps leave a comment/TODO with your suggestion?
https://codereview.chromium.org/2856073003/diff/1/net/quic/chromium/quic_netw... File net/quic/chromium/quic_network_transaction_unittest.cc (right): https://codereview.chromium.org/2856073003/diff/1/net/quic/chromium/quic_netw... net/quic/chromium/quic_network_transaction_unittest.cc:1069: // deterministic. The first main job gets aborted with no socket. On 2017/05/03 23:38:55, Ryan Hamilton wrote: > On 2017/05/03 23:33:13, davidben wrote: > > On 2017/05/03 23:20:39, Ryan Hamilton wrote: > > > Hm. This comment seems to say: > > > > > > 1) The main job attempts to call Connect() on a socket, but that hangs. > > > 2) The main job is aborted without ever getting a socket. > > > > > > I don't think I understand how to reconcile that. > > > > (1) isn't quite true. The socket pools call Connect() but never vend out > sockets > > until they succeed. Another way to write this test would be to have three > > sockets and guarantee that the second socket is bound to the main job before > the > > main job is destroyed (and thus the comment about "there is no open TCP > socket" > > would be true), but I didn't see a way to do it. > > Ah. Ok, I think the test is fine. (no surprise there) but I think the last > sentence in the comment could perhaps be clarified to mention that "it gets > aborted without the socket pool ever dispensing the socket" or some such. Done. > > Honestly, most of this mess isn't necessary because you all delay the > alternate > > job by 300ms, meanwhile the QUIC job completes in a few PostTasks (not quite > > synchronously though). I believe the test works just fine if you just remove > > failing_data and leave http_data alone. But it seemed poor to me to depend on > > how the two jobs race. I dunno, what do you all think? > > That's possible. The delay-tcp-race logic was not on by default until just > recently. I'd be inclined to leave it be, but perhaps leave a comment/TODO with > your suggestion? Sorry, that might have been unclear. This is the more complicated but (hopefully?) more robust version of the test. It is robust against a thing perhaps it's not worth being robust against (the mock TCP job winning the race against the mock QUIC job, which it currently can't because of the 300ms delay). The simpler version, if you're happy with just assuming the QUIC job always wins the race, would be to remove the explicit pause at Connect().
The CQ bit was checked by davidben@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bnc@chromium.org, rch@chromium.org Link to the patchset: https://codereview.chromium.org/2856073003/#ps20001 (title: "rch comment")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1494021465768940,
"parent_rev": "980c8ee750096770976476654d13b5ecf412bee4", "commit_rev":
"a4449725c3c1d07ecd8008d5d03227c4588b85dc"}
Message was sent while issue was closed.
Description was changed from ========== Fix QuicNetworkTransactionTest.RetryMisdirectedRequest This test was depending on the SSL deprecated cipher fallback (which is a quirk to probe server cipher preferences and not something QUIC may depend on) to clear away the ERR_CONNECTION_CLOSED when it shouldn't have been surfacing one to begin with. Fortunately, the actual logic isn't relying on this. The test just needs fixing. Rather than scheduling three connections, schedule two. The main job hangs until after all the QUIC machinery is done, then we unblock the main job and let that complete. BUG=546991,684730 ========== to ========== Fix QuicNetworkTransactionTest.RetryMisdirectedRequest This test was depending on the SSL deprecated cipher fallback (which is a quirk to probe server cipher preferences and not something QUIC may depend on) to clear away the ERR_CONNECTION_CLOSED when it shouldn't have been surfacing one to begin with. Fortunately, the actual logic isn't relying on this. The test just needs fixing. Rather than scheduling three connections, schedule two. The main job hangs until after all the QUIC machinery is done, then we unblock the main job and let that complete. BUG=546991,684730 Review-Url: https://codereview.chromium.org/2856073003 Cr-Commit-Position: refs/heads/master@{#469799} Committed: https://chromium.googlesource.com/chromium/src/+/a4449725c3c1d07ecd8008d5d032... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/a4449725c3c1d07ecd8008d5d032... |
