Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1494813002:
Fix HttpStreamParser::CanReuseConnection(). (Closed)
DescriptionFix HttpStreamParser::CanReuseConnection().
It was incorrectly returning false when the connection was not idle.
This regressed in https://codereview.chromium.org/1320683003, when the
logic was moved down into HttpStreamParser from HttpNetworkTransaction.
The most noticeable thing this broke is multi-round auth when there
was a large response body, but it also made it so Chrome wasn't
reusing sockets when requests with partially received responses were
cancelled.
I'm landing this without tests - unfortunately, writing tests for this
revealed that a huge number of net tests rely on mock sockets that keep
pointers around to SocketDataProviders that are deleted before the mock
socket is. Tests are blocked on either resolving that, or making new
infrastructure that works around the issue.
BUG=544255
Committed: https://crrev.com/911ee022046cbcfaa8c7ea8d41760bea5b67070c
Cr-Commit-Position: refs/heads/master@{#363116}
Patch Set 1 #Patch Set 2 : Update logic #Patch Set 3 : Fix tests #Patch Set 4 : Bring back removed test #
Total comments: 4
Patch Set 5 : add override #
Total comments: 2
Patch Set 6 : Fix typo #Patch Set 7 : More fixes #
Total comments: 1
Patch Set 8 : One more try? #Patch Set 9 : Remove tests #Patch Set 10 : One test change was needed... #
Messages
Total messages: 46 (22 generated)
Patchset #3 (id:40001) has been deleted
mmenke@chromium.org changed reviewers: + asanka@chromium.org
Believe all tests should now pass. https://codereview.chromium.org/1494813002/diff/80001/net/http/http_network_t... File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/1494813002/diff/80001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:5904: MockRead("hello world"), MockRead(ASYNC, ERR_CONNECTION_CLOSED) // EOF Note that this test depended on MockSSLWhateverSocket::IsConnected() returning false while IsConnectedAndIdle() returned true, and IsConnected() never being called until SSLClientSocket tried to reuse the connection. That was a rather bizarre requirement. I've fixed the mock client socket, and changed this test so it still works, but this does change the lines it ends up testing a bit. https://codereview.chromium.org/1494813002/diff/80001/net/socket/socket_test_... File net/socket/socket_test_util.h (right): https://codereview.chromium.org/1494813002/diff/80001/net/socket/socket_test_... net/socket/socket_test_util.h:915: bool IsConnectedAndIdle() const; See comment in the unittest file for why I added this.
LGTM https://codereview.chromium.org/1494813002/diff/80001/net/socket/socket_test_... File net/socket/socket_test_util.h (right): https://codereview.chromium.org/1494813002/diff/80001/net/socket/socket_test_... net/socket/socket_test_util.h:376: // When true, IsConnectedAndIdle() will return false if the next even in the *event https://codereview.chromium.org/1494813002/diff/100001/net/socket/socket_test... File net/socket/socket_test_util.h (right): https://codereview.chromium.org/1494813002/diff/100001/net/socket/socket_test... net/socket/socket_test_util.h:379: void set_busy_before_sync_reads(bool busy_before_sync_reads) { Perhaps not for this CL, but shouldn't this always be true? The IsConnectedAndIdle() test is basically looking for whether the next read will succeed synchronously.
Thanks! https://codereview.chromium.org/1494813002/diff/80001/net/socket/socket_test_... File net/socket/socket_test_util.h (right): https://codereview.chromium.org/1494813002/diff/80001/net/socket/socket_test_... net/socket/socket_test_util.h:376: // When true, IsConnectedAndIdle() will return false if the next even in the On 2015/12/03 17:41:56, asanka wrote: > *event Done. https://codereview.chromium.org/1494813002/diff/100001/net/socket/socket_test... File net/socket/socket_test_util.h (right): https://codereview.chromium.org/1494813002/diff/100001/net/socket/socket_test... net/socket/socket_test_util.h:379: void set_busy_before_sync_reads(bool busy_before_sync_reads) { On 2015/12/03 17:41:57, asanka wrote: > Perhaps not for this CL, but shouldn't this always be true? The > IsConnectedAndIdle() test is basically looking for whether the next read will > succeed synchronously. > I was worried about breaking all existing synchronous tests that check socket reuse cases, but I suppose those would generally have a write before the read. I'll experiment with it in a followup CL, and see how things look.
Doh! We still have a problem... http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_... That's a real failure. A ResponseBodyDrainer is still hanging on to a mock socket after we exit the response body. :(
On 2015/12/03 18:29:49, mmenke wrote: > Doh! We still have a problem... > > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_... > > That's a real failure. A ResponseBodyDrainer is still hanging on to a mock > socket after we exit the response body. :( I think I'll just fix that test fixture, and hope nothing else has that problem.
WDYT? I haven't extended the extra logic to the SSL mock sockets (Not currently needed, but does seem like a good idea), or DeterministicSocketData (Also not currently needed, and we should remove that class). https://codereview.chromium.org/1494813002/diff/140001/net/socket/socket_test... File net/socket/socket_test_util.h (right): https://codereview.chromium.org/1494813002/diff/140001/net/socket/socket_test... net/socket/socket_test_util.h:245: virtual void OnDataProviderDestroyed() = 0; Could use weak ptrs instead, but I think this is a bit more flexible. Could make this cancel pending callbacks or something in the future.
[+rch]: Just FYI, concerning test socket changes.
On 2015/12/03 19:49:12, mmenke wrote: > [+rch]: Just FYI, concerning test socket changes. And general text borkage - a bunch of the changes here are due to issues in old tests.
The CQ bit was checked by mmenke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1494813002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1494813002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
So this is running into issues with mock sockets not created through factories, since they don't set the socket on data providers. Also, sometimes providers are used by multiple sockets...Which rather breaks the whole set_socket thing. I'm trying a fix for that, but if this doesn't work, I'll need to switch to weak ptrs...And wrestle out how this should be done some other time.
On 2015/12/03 21:24:16, mmenke wrote: > So this is running into issues with mock sockets not created through factories, > since they don't set the socket on data providers. Also, sometimes providers > are used by multiple sockets...Which rather breaks the whole set_socket thing. > I'm trying a fix for that, but if this doesn't work, I'll need to switch to weak > ptrs...And wrestle out how this should be done some other time. Yeah. I saw the breakages. Let's see what happens this time around.
On 2015/12/03 21:24:16, mmenke wrote: > So this is running into issues with mock sockets not created through factories, > since they don't set the socket on data providers. Also, sometimes providers > are used by multiple sockets...Which rather breaks the whole set_socket thing. > I'm trying a fix for that, but if this doesn't work, I'll need to switch to weak > ptrs...And wrestle out how this should be done some other time. Talked to the TPM. We'll land it without the tests, and see how things go on Canary tomorrow, and then talk about a merge.
On 2015/12/03 22:21:39, mmenke wrote: > On 2015/12/03 21:24:16, mmenke wrote: > > So this is running into issues with mock sockets not created through > factories, > > since they don't set the socket on data providers. Also, sometimes providers > > are used by multiple sockets...Which rather breaks the whole set_socket thing. > > > I'm trying a fix for that, but if this doesn't work, I'll need to switch to > weak > > ptrs...And wrestle out how this should be done some other time. > > Talked to the TPM. We'll land it without the tests, and see how things go on > Canary tomorrow, and then talk about a merge. And for the record, Asanka and I talked offline, and he said he'd sign off on landing without a tests.
Description was changed from ========== Fix HttpStreamParser::CanReuseConnection(). It was incorrectly returning false when the connection was not idle. This regressed in https://codereview.chromium.org/1320683003, when the logic was moved down into HttpStreamParser from HttpNetworkTransaction. The most noticeable thing this broke is multi-round auth when there was a large response body, but it also made it so Chrome wasn't reusing sockets when requests with partially received responses were cancelled. BUG=544255 ========== to ========== Fix HttpStreamParser::CanReuseConnection(). It was incorrectly returning false when the connection was not idle. This regressed in https://codereview.chromium.org/1320683003, when the logic was moved down into HttpStreamParser from HttpNetworkTransaction. The most noticeable thing this broke is multi-round auth when there was a large response body, but it also made it so Chrome wasn't reusing sockets when requests with partially received responses were cancelled. I'm landing this without tests - unfortunately, writing tests for this revealed that a huge number of net tests rely on mock sockets that keep pointers around to SocketDataProviders that are deleted before the mock socket is. Tests are blocked on either resolving that, or making new infrastructure that works around the issue. BUG=544255 ==========
The CQ bit was checked by mmenke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org Link to the patchset: https://codereview.chromium.org/1494813002/#ps180001 (title: "Remove tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1494813002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1494813002/180001
The CQ bit was unchecked by mmenke@chromium.org
The CQ bit was checked by mmenke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1494813002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1494813002/180001
The CQ bit was unchecked by mmenke@chromium.org
The CQ bit was checked by mmenke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1494813002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1494813002/180001
The CQ bit was checked by mmenke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org Link to the patchset: https://codereview.chromium.org/1494813002/#ps200001 (title: "One test change was needed...")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1494813002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1494813002/200001
The CQ bit was checked by mmenke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org Link to the patchset: https://codereview.chromium.org/1494813002/#ps220001 (title: "Grr...")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1494813002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1494813002/220001
Patchset #11 (id:220001) has been deleted
The CQ bit was unchecked by mmenke@chromium.org
The CQ bit was checked by mmenke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1494813002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1494813002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) android_clang_dbg_recipe on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) android_compile_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_android on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_linux on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_android_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by mmenke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1494813002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1494813002/200001
Message was sent while issue was closed.
Description was changed from ========== Fix HttpStreamParser::CanReuseConnection(). It was incorrectly returning false when the connection was not idle. This regressed in https://codereview.chromium.org/1320683003, when the logic was moved down into HttpStreamParser from HttpNetworkTransaction. The most noticeable thing this broke is multi-round auth when there was a large response body, but it also made it so Chrome wasn't reusing sockets when requests with partially received responses were cancelled. I'm landing this without tests - unfortunately, writing tests for this revealed that a huge number of net tests rely on mock sockets that keep pointers around to SocketDataProviders that are deleted before the mock socket is. Tests are blocked on either resolving that, or making new infrastructure that works around the issue. BUG=544255 ========== to ========== Fix HttpStreamParser::CanReuseConnection(). It was incorrectly returning false when the connection was not idle. This regressed in https://codereview.chromium.org/1320683003, when the logic was moved down into HttpStreamParser from HttpNetworkTransaction. The most noticeable thing this broke is multi-round auth when there was a large response body, but it also made it so Chrome wasn't reusing sockets when requests with partially received responses were cancelled. I'm landing this without tests - unfortunately, writing tests for this revealed that a huge number of net tests rely on mock sockets that keep pointers around to SocketDataProviders that are deleted before the mock socket is. Tests are blocked on either resolving that, or making new infrastructure that works around the issue. BUG=544255 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Fix HttpStreamParser::CanReuseConnection(). It was incorrectly returning false when the connection was not idle. This regressed in https://codereview.chromium.org/1320683003, when the logic was moved down into HttpStreamParser from HttpNetworkTransaction. The most noticeable thing this broke is multi-round auth when there was a large response body, but it also made it so Chrome wasn't reusing sockets when requests with partially received responses were cancelled. I'm landing this without tests - unfortunately, writing tests for this revealed that a huge number of net tests rely on mock sockets that keep pointers around to SocketDataProviders that are deleted before the mock socket is. Tests are blocked on either resolving that, or making new infrastructure that works around the issue. BUG=544255 ========== to ========== Fix HttpStreamParser::CanReuseConnection(). It was incorrectly returning false when the connection was not idle. This regressed in https://codereview.chromium.org/1320683003, when the logic was moved down into HttpStreamParser from HttpNetworkTransaction. The most noticeable thing this broke is multi-round auth when there was a large response body, but it also made it so Chrome wasn't reusing sockets when requests with partially received responses were cancelled. I'm landing this without tests - unfortunately, writing tests for this revealed that a huge number of net tests rely on mock sockets that keep pointers around to SocketDataProviders that are deleted before the mock socket is. Tests are blocked on either resolving that, or making new infrastructure that works around the issue. BUG=544255 Committed: https://crrev.com/911ee022046cbcfaa8c7ea8d41760bea5b67070c Cr-Commit-Position: refs/heads/master@{#363116} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/911ee022046cbcfaa8c7ea8d41760bea5b67070c Cr-Commit-Position: refs/heads/master@{#363116} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
