Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1091793002:
Report the connect status of the oldest connection in the socket pool. (Closed)
DescriptionReport the connect status of the oldest connection in the socket pool.
This CL implements a combination of the suggestions 1) and 2)
and comment #1 in crbug.com/462808. This reduces the number of
calls to GetLoadState. However the status might be less accurate
as we are far from guaranteed that the oldest connection is the
one which has come the furthest. This should be acceptable though,
as the state is mainly used report activity or at which stage
connections are hung at.
BUG=462808
Committed: https://crrev.com/835c1d6d6b8374ed2e71e04825b7b3c58fd0aa32
Cr-Commit-Position: refs/heads/master@{#326244}
Patch Set 1 #Patch Set 2 : Improve comment #
Total comments: 8
Patch Set 3 : Use a list instead of a set. #
Total comments: 9
Patch Set 4 : Address mmenkes' comments #Patch Set 5 : Fixed typo #Patch Set 6 : Final nit #
Total comments: 4
Patch Set 7 : Address more comments #Patch Set 8 : Fixed typo #
Messages
Total messages: 16 (3 generated)
haavardm@opera.com changed reviewers: + eroman@chromium.org, mmenke@chromium.org
mmenke please review, eroman, FYI.
https://codereview.chromium.org/1091793002/diff/20001/net/socket/client_socke... File net/socket/client_socket_pool_base.h (right): https://codereview.chromium.org/1091793002/diff/20001/net/socket/client_socke... net/socket/client_socket_pool_base.h:120: } If two ConnectJob are created at the same time, doesn't this mean the set will consider them equivalent? So when one of two requests with the same start time connects, we could end up removing the other one from the set of ConnectJobs? https://codereview.chromium.org/1091793002/diff/20001/net/socket/client_socke... net/socket/client_socket_pool_base.h:120: } Also, should probably have two tests for this (In both tests, two sockets. In one, the first hangs, second connects. In the other, reverse them. Then in the test make sure we get the status of the first socket. Have to make sure the sort method distinguished between the two. Maybe it would be simplest to just use a list? Does mean we have to search through it when a socket connects, but there are generally at most 6 of them, anyways.
https://codereview.chromium.org/1091793002/diff/20001/net/socket/client_socke... File net/socket/client_socket_pool_base.h (right): https://codereview.chromium.org/1091793002/diff/20001/net/socket/client_socke... net/socket/client_socket_pool_base.h:120: } On 2015/04/17 15:32:40, mmenke wrote: > If two ConnectJob are created at the same time, doesn't this mean the set will > consider them equivalent? So when one of two requests with the same start time > connects, we could end up removing the other one from the set of ConnectJobs? Yes, I discovered yesterday that that's exactly what seems to happen. The weird thing is that it should be fixed by adding if (lhs->connect_timing().connect_start == rhs->connect_timing().connect_start) return lhs < rhs before comparing the time. However, connect jobs are still removed. This addition also means that connections created at the same time will be in random order. https://codereview.chromium.org/1091793002/diff/20001/net/socket/client_socke... net/socket/client_socket_pool_base.h:120: } On 2015/04/17 15:32:39, mmenke wrote: > Also, should probably have two tests for this (In both tests, two sockets. In > one, the first hangs, second connects. In the other, reverse them. Then in the > test make sure we get the status of the first socket. Have to make sure the > sort method distinguished between the two. Maybe it would be simplest to just > use a list? Does mean we have to search through it when a socket connects, but > there are generally at most 6 of them, anyways. Agree. Using something else than a set here is probably a good idea and a list seems appropriate.
https://codereview.chromium.org/1091793002/diff/20001/net/socket/client_socke... File net/socket/client_socket_pool_base.h (right): https://codereview.chromium.org/1091793002/diff/20001/net/socket/client_socke... net/socket/client_socket_pool_base.h:120: } On 2015/04/18 07:24:16, haavardm wrote: > On 2015/04/17 15:32:40, mmenke wrote: > > If two ConnectJob are created at the same time, doesn't this mean the set will > > consider them equivalent? So when one of two requests with the same start > time > > connects, we could end up removing the other one from the set of ConnectJobs? > > Yes, I discovered yesterday that that's exactly what seems to happen. The weird > thing is that it should be fixed by adding > if (lhs->connect_timing().connect_start == rhs->connect_timing().connect_start) > return lhs < rhs > before comparing the time. However, connect jobs are still removed. This > addition also means that connections created at the same time will be in random > order. > That's probably because connect_start times can change (ConnectJobs in higher layer socket pools copy it from the lower level ones, for instance)
Did the list approach. PTAL https://codereview.chromium.org/1091793002/diff/20001/net/socket/client_socke... File net/socket/client_socket_pool_base.h (right): https://codereview.chromium.org/1091793002/diff/20001/net/socket/client_socke... net/socket/client_socket_pool_base.h:120: } On 2015/04/17 15:32:39, mmenke wrote: > Also, should probably have two tests for this (In both tests, two sockets. In > one, the first hangs, second connects. In the other, reverse them. Then in the > test make sure we get the status of the first socket. Not sure I get this test. As I understand it, sockets that connect is removed from the list. So in both those tests only the hanging connection will be left, not matter how it is sorted? Have to make sure the > sort method distinguished between the two. Maybe it would be simplest to just > use a list? Does mean we have to search through it when a socket connects, but > there are generally at most 6 of them, anyways. I implemented the list approach. https://codereview.chromium.org/1091793002/diff/20001/net/socket/client_socke... net/socket/client_socket_pool_base.h:120: } On 2015/04/18 13:49:57, mmenke wrote: > On 2015/04/18 07:24:16, haavardm wrote: > > On 2015/04/17 15:32:40, mmenke wrote: > > > If two ConnectJob are created at the same time, doesn't this mean the set > will > > > consider them equivalent? So when one of two requests with the same start > > time > > > connects, we could end up removing the other one from the set of > ConnectJobs? > > > > Yes, I discovered yesterday that that's exactly what seems to happen. The > weird > > thing is that it should be fixed by adding > > if (lhs->connect_timing().connect_start == > rhs->connect_timing().connect_start) > > return lhs < rhs > > before comparing the time. However, connect jobs are still removed. This > > addition also means that connections created at the same time will be in > random > > order. > > > > That's probably because connect_start times can change (ConnectJobs in higher > layer socket pools copy it from the lower level ones, for instance) I removed the compare function since list is now used instead of a set.
Just minor suggestions https://codereview.chromium.org/1091793002/diff/20001/net/socket/client_socke... File net/socket/client_socket_pool_base.h (right): https://codereview.chromium.org/1091793002/diff/20001/net/socket/client_socke... net/socket/client_socket_pool_base.h:120: } On 2015/04/20 13:03:48, haavardm wrote: > On 2015/04/17 15:32:39, mmenke wrote: > > Also, should probably have two tests for this (In both tests, two sockets. In > > one, the first hangs, second connects. In the other, reverse them. Then in > the > > test make sure we get the status of the first socket. > > Not sure I get this test. As I understand it, sockets that connect is removed > from the list. So in both those tests only the hanging connection will be left, > not matter how it is sorted? I was thinking SSL sockets, where more is needed after the TCP connection. That would coincidentally also exercise the connect time overwriting code, though unless we inserted some sort of delay, I suspect times would almost always be the same, anyways. Anyhow, see (different, less aggressive) suggestions in other file. > Have to make sure the > > sort method distinguished between the two. Maybe it would be simplest to just > > use a list? Does mean we have to search through it when a socket connects, > but > > there are generally at most 6 of them, anyways. > > I implemented the list approach. https://codereview.chromium.org/1091793002/diff/40001/net/socket/client_socke... File net/socket/client_socket_pool_base.cc (right): https://codereview.chromium.org/1091793002/diff/40001/net/socket/client_socke... net/socket/client_socket_pool_base.cc:1197: jobs_.remove(job); Suggest: // |job| should appear once and only once in the list. DCHECK_NE(jobs_.find(job), jobs_.end()); jobs_.remove(job); DCHECK_EQ(jobs_.find(job), jobs_.end()); https://codereview.chromium.org/1091793002/diff/40001/net/socket/client_socke... File net/socket/client_socket_pool_base_unittest.cc (right): https://codereview.chromium.org/1091793002/diff/40001/net/socket/client_socke... net/socket/client_socket_pool_base_unittest.cc:1982: EXPECT_EQ(LOAD_STATE_CONNECTING, handle2.GetLoadState()); This seems a little weird now. Suggest moving the "client_socket_factory_.SetJobLoadState(0, LOAD_STATE_RESOLVING_HOST);" just below the handle.Init / EXPECT_EQ lines, and getting rid of this check. https://codereview.chromium.org/1091793002/diff/40001/net/socket/client_socke... net/socket/client_socket_pool_base_unittest.cc:1987: EXPECT_EQ(LOAD_STATE_RESOLVING_HOST, handle2.GetLoadState()); I'm paranoid: Could you have another test just like this, but set the state of the second socket (And have the second socket connect at the end)? https://codereview.chromium.org/1091793002/diff/40001/net/socket/client_socke... net/socket/client_socket_pool_base_unittest.cc:1990: // second handle switches to the state of the remaining ConnectJob. This comment is no longer accurate (It's the one that's not farthest along...Maybe just "First job connects and...."
https://codereview.chromium.org/1091793002/diff/40001/net/socket/client_socke... File net/socket/client_socket_pool_base_unittest.cc (right): https://codereview.chromium.org/1091793002/diff/40001/net/socket/client_socke... net/socket/client_socket_pool_base_unittest.cc:1982: EXPECT_EQ(LOAD_STATE_CONNECTING, handle2.GetLoadState()); On 2015/04/20 14:49:16, mmenke wrote: > This seems a little weird now. Suggest moving the > "client_socket_factory_.SetJobLoadState(0, LOAD_STATE_RESOLVING_HOST);" just > below the handle.Init / EXPECT_EQ lines, and getting rid of this check. Actually, going backwards in general seems a little weird, though the test was going it before, too. Maybe just move the first request forward instead (And in a new test, move the second one forward).
PTAL https://codereview.chromium.org/1091793002/diff/40001/net/socket/client_socke... File net/socket/client_socket_pool_base.cc (right): https://codereview.chromium.org/1091793002/diff/40001/net/socket/client_socke... net/socket/client_socket_pool_base.cc:1197: jobs_.remove(job); On 2015/04/20 14:49:16, mmenke wrote: > Suggest: > > // |job| should appear once and only once in the list. > DCHECK_NE(jobs_.find(job), jobs_.end()); > jobs_.remove(job); > DCHECK_EQ(jobs_.find(job), jobs_.end()); Done. Lists doesn't have the find operation though, so I used std::find from <algorithm> instead. Note that I had to give the DCHECK macros the actual object instead of the iterator to get the same template version of std::find in the compare operation. https://codereview.chromium.org/1091793002/diff/40001/net/socket/client_socke... File net/socket/client_socket_pool_base_unittest.cc (right): https://codereview.chromium.org/1091793002/diff/40001/net/socket/client_socke... net/socket/client_socket_pool_base_unittest.cc:1982: EXPECT_EQ(LOAD_STATE_CONNECTING, handle2.GetLoadState()); On 2015/04/20 20:32:16, mmenke wrote: > On 2015/04/20 14:49:16, mmenke wrote: > > This seems a little weird now. Suggest moving the > > "client_socket_factory_.SetJobLoadState(0, LOAD_STATE_RESOLVING_HOST);" just > > below the handle.Init / EXPECT_EQ lines, and getting rid of this check. > > Actually, going backwards in general seems a little weird, though the test was > going it before, too. Maybe just move the first request forward instead (And in > a new test, move the second one forward). Done. https://codereview.chromium.org/1091793002/diff/40001/net/socket/client_socke... net/socket/client_socket_pool_base_unittest.cc:1987: EXPECT_EQ(LOAD_STATE_RESOLVING_HOST, handle2.GetLoadState()); On 2015/04/20 14:49:16, mmenke wrote: > I'm paranoid: Could you have another test just like this, but set the state of > the second socket (And have the second socket connect at the end)? Done. https://codereview.chromium.org/1091793002/diff/40001/net/socket/client_socke... net/socket/client_socket_pool_base_unittest.cc:1990: // second handle switches to the state of the remaining ConnectJob. On 2015/04/20 14:49:16, mmenke wrote: > This comment is no longer accurate (It's the one that's not farthest > along...Maybe just "First job connects and...." Done.
LGTM, modulo comments. https://codereview.chromium.org/1091793002/diff/100001/net/socket/client_sock... File net/socket/client_socket_pool_base.cc (right): https://codereview.chromium.org/1091793002/diff/100001/net/socket/client_sock... net/socket/client_socket_pool_base.cc:1202: DCHECK_EQ(*std::find(jobs_.begin(), jobs_.end(), job), *jobs_.end()); Oops: This line doesn't actually accomplish anything: list::remove removes all hits, not just the first one. https://codereview.chromium.org/1091793002/diff/100001/net/socket/client_sock... File net/socket/client_socket_pool_base_unittest.cc (right): https://codereview.chromium.org/1091793002/diff/100001/net/socket/client_sock... net/socket/client_socket_pool_base_unittest.cc:1999: client_socket_factory_.SetJobLoadState(0, LOAD_STATE_RESOLVING_HOST); Suggest just removing this line and making sure they're both connecting, updating the EXPECT_EQs are needed. (And removing the "client_socket_factory_.SetJobLoadState(1, LOAD_STATE_CONNECTING);" line and the following two EXPECT_EQS).
PTAL https://codereview.chromium.org/1091793002/diff/100001/net/socket/client_sock... File net/socket/client_socket_pool_base.cc (right): https://codereview.chromium.org/1091793002/diff/100001/net/socket/client_sock... net/socket/client_socket_pool_base.cc:1202: DCHECK_EQ(*std::find(jobs_.begin(), jobs_.end(), job), *jobs_.end()); On 2015/04/21 14:27:33, mmenke wrote: > Oops: This line doesn't actually accomplish anything: list::remove removes all > hits, not just the first one. Well, it tests that list::remove works, but that's hardly our job :) I'll remove it. https://codereview.chromium.org/1091793002/diff/100001/net/socket/client_sock... File net/socket/client_socket_pool_base_unittest.cc (right): https://codereview.chromium.org/1091793002/diff/100001/net/socket/client_sock... net/socket/client_socket_pool_base_unittest.cc:1999: client_socket_factory_.SetJobLoadState(0, LOAD_STATE_RESOLVING_HOST); On 2015/04/21 14:27:33, mmenke wrote: > Suggest just removing this line and making sure they're both connecting, > updating the EXPECT_EQs are needed. (And removing the > "client_socket_factory_.SetJobLoadState(1, LOAD_STATE_CONNECTING);" line and the > following two EXPECT_EQS). Fair enough. Done.
The CQ bit was checked by haavardm@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1091793002/#ps140001 (title: "Fixed typo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1091793002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/835c1d6d6b8374ed2e71e04825b7b3c58fd0aa32 Cr-Commit-Position: refs/heads/master@{#326244} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
