Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1898133002:
Add reprioritization to socket pools. (Closed)
|
Created:
4 years, 8 months ago by Randy Smith (Not in Mondays) Modified:
3 years, 10 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, Adam Rice Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd reprioritization to socket pools.
BUG=600839
BUG=166689
Review-Url: https://codereview.chromium.org/1898133002
Cr-Commit-Position: refs/heads/master@{#451185}
Committed: https://chromium.googlesource.com/chromium/src/+/29dbad12b55c8d2d520b26987735354c25b9590c
Patch Set 1 #Patch Set 2 : Merged to p388233 #Patch Set 3 : Plumbed SetPriority down from HttpStreamFactoryImpl::Job. #
Total comments: 9
Patch Set 4 : Sync'd to p390110 #Patch Set 5 : Sync'd to p392237 #Patch Set 6 : Fixed incorrect assupmtion of ClientSocketHandle::pool_ initialization. #Patch Set 7 : Incorporated Matt's comments. #
Total comments: 8
Patch Set 8 : Sync'd to p440510 #Patch Set 9 : Fixed reprioritization test. #Patch Set 10 : Add Matt's suggested tests. #Patch Set 11 : Test for not propagating priority in HttpStreamFactoryImpl::Job if the stream's been returned alrea… #
Total comments: 17
Patch Set 12 : Fix signed/unsigned comparison for windows build. #Patch Set 13 : Respond to Matt's comments. #Patch Set 14 : Fix glitch in ClientSocketHandle API contract. #
Total comments: 17
Patch Set 15 : Incorporated latest round of comments. #
Total comments: 17
Patch Set 16 : Incorporated latest comments. #Patch Set 17 : Attempt to fix windows compile failure. #
Total comments: 2
Patch Set 18 : Sync'd to p449871 #Patch Set 19 : Incorporated comments. #Messages
Total messages: 91 (48 generated)
rdsmith@chromium.org changed reviewers: + mmenke@chromium.org
Matt, willing to take a quick look at this? I'm part way through the plumbing of reprioritization through the socket pools, and while I'm pretty sure I'm moving in the right direction, I'm doing enough that it seems worth a quick checkin. What I've done so far: * Added a SetPriority entry point to all pools. * Plumbed that through ClientSocketPoolBase, ClientSocketPoolBaseHelper, ClientSocketPoolBaseHelper::Request, and into the priority queue held by CSPBH. * Added ClientSocketHandle::SetPriority and called it from the HttpStreamFactoryImpl::Job::SetPriority. What I still need to do: * Add ConnectJob::SetPriority (and all subclass implementations). * Plumb that through the ClientSocketHandle::SetPriority on the underlying transport sockets. * Plumb the websockets and transport implementations through to the DNS resolver(s). My concerns: * I removed a lot of "const"s on Request classes to do the plumbing within the socket pools. I decided that this was ok (partially because I don't think Request is conceptually const anymore if I'm reprioritizing it, partially because it's a class whose use is pretty limited within the socket pools, so loosing the const guarantee doesn't seem that risky) but it's obvious something worth evaluating for gotchas. * Long-term, I'm concerned about the implementation of FindAndRemovePendingRequest(), since a) I use it for reprioritization, and b) it does a linear search for the handle argument. If I successfully move throttling down into the network stack, there'll be a lot more queued requests, and potentially a lot more searching on reprioritization. So I think this'll need to be fixed, but I'm inclined to worry about it in a separate CL. * I could easily submit this as a single large CL, or I could break it apart. I think the minimum CL would be this one (the plumbing to but not through the socket pools adds real functionality, for all that it doesn't add all the functionality you'd want). I'm inclined to submit them as small CLs--i.e. add tests to this one and submit, and add plumbing to transport socket pools in a separate CL, and to resolvers in a CL after that. But I'd like to know your thoughts on appropriate granularity. * I'm really not looking forward to figuring out how to test all this, though I presume there's no better way than groking each classes unit tests and making sure to add tests that cover the new functionality. Having said that, if you have any pointers I'm interested. I'm also interested in what thoughts you might have about integration tests (tests of groups of these classes together) as this CL and the coming CLs will be mostly plumbing, so making sure the classes are correctly talking to each other will be important. That ended up being longer than I expect :-}. When you get a chance, I'm curious what you think. As remarked on the other CL, no worries; I have several irons in the fire.
Since you've indicated there's no rush on these, I'll probably put them both off until next week. Had 14 codereviews waiting for me after I took two days off. You could look for other reviewers, but socket pool changes in the past have been sufficiently problematic that I think I should take this one on myself, at least. On 2016/04/20 00:46:42, Randy Smith - Not in Fridays wrote: > Matt, willing to take a quick look at this? I'm part way through the plumbing > of reprioritization through the socket pools, and while I'm pretty sure I'm > moving in the right direction, I'm doing enough that it seems worth a quick > checkin. What I've done so far: > * Added a SetPriority entry point to all pools. > * Plumbed that through ClientSocketPoolBase, ClientSocketPoolBaseHelper, > ClientSocketPoolBaseHelper::Request, and into the priority queue held by CSPBH. > * Added ClientSocketHandle::SetPriority and called it from the > HttpStreamFactoryImpl::Job::SetPriority. > > What I still need to do: > * Add ConnectJob::SetPriority (and all subclass implementations). > * Plumb that through the ClientSocketHandle::SetPriority on the underlying > transport sockets. > * Plumb the websockets and transport implementations through to the DNS > resolver(s). > > My concerns: > * I removed a lot of "const"s on Request classes to do the plumbing within the > socket pools. I decided that this was ok (partially because I don't think > Request is conceptually const anymore if I'm reprioritizing it, partially > because it's a class whose use is pretty limited within the socket pools, so > loosing the const guarantee doesn't seem that risky) but it's obvious something > worth evaluating for gotchas. > > * Long-term, I'm concerned about the implementation of > FindAndRemovePendingRequest(), since a) I use it for reprioritization, and b) it > does a linear search for the handle argument. If I successfully move throttling > down into the network stack, there'll be a lot more queued requests, and > potentially a lot more searching on reprioritization. So I think this'll need > to be fixed, but I'm inclined to worry about it in a separate CL. > > * I could easily submit this as a single large CL, or I could break it apart. I > think the minimum CL would be this one (the plumbing to but not through the > socket pools adds real functionality, for all that it doesn't add all the > functionality you'd want). I'm inclined to submit them as small CLs--i.e. add > tests to this one and submit, and add plumbing to transport socket pools in a > separate CL, and to resolvers in a CL after that. But I'd like to know your > thoughts on appropriate granularity. > > * I'm really not looking forward to figuring out how to test all this, though I > presume there's no better way than groking each classes unit tests and making > sure to add tests that cover the new functionality. Having said that, if you > have any pointers I'm interested. I'm also interested in what thoughts you > might have about integration tests (tests of groups of these classes together) > as this CL and the coming CLs will be mostly plumbing, so making sure the > classes are correctly talking to each other will be important. > > That ended up being longer than I expect :-}. When you get a chance, I'm > curious what you think. As remarked on the other CL, no worries; I have several > irons in the fire.
On 2016/04/21 18:34:07, mmenke wrote: > Since you've indicated there's no rush on these, I'll probably put them both off > until next week. Had 14 codereviews waiting for me after I took two days off. > You could look for other reviewers, but socket pool changes in the past have > been sufficiently problematic that I think I should take this one on myself, at > least. No problem on the delay, and I'm happy to go with your opinions about appropriate reviewers; feel free to foist me off if-and-only-if you think that's safe. > > On 2016/04/20 00:46:42, Randy Smith - Not in Fridays wrote: > > Matt, willing to take a quick look at this? I'm part way through the plumbing > > of reprioritization through the socket pools, and while I'm pretty sure I'm > > moving in the right direction, I'm doing enough that it seems worth a quick > > checkin. What I've done so far: > > * Added a SetPriority entry point to all pools. > > * Plumbed that through ClientSocketPoolBase, ClientSocketPoolBaseHelper, > > ClientSocketPoolBaseHelper::Request, and into the priority queue held by > CSPBH. > > * Added ClientSocketHandle::SetPriority and called it from the > > HttpStreamFactoryImpl::Job::SetPriority. > > > > What I still need to do: > > * Add ConnectJob::SetPriority (and all subclass implementations). > > * Plumb that through the ClientSocketHandle::SetPriority on the underlying > > transport sockets. > > * Plumb the websockets and transport implementations through to the DNS > > resolver(s). > > > > My concerns: > > * I removed a lot of "const"s on Request classes to do the plumbing within the > > socket pools. I decided that this was ok (partially because I don't think > > Request is conceptually const anymore if I'm reprioritizing it, partially > > because it's a class whose use is pretty limited within the socket pools, so > > loosing the const guarantee doesn't seem that risky) but it's obvious > something > > worth evaluating for gotchas. > > > > * Long-term, I'm concerned about the implementation of > > FindAndRemovePendingRequest(), since a) I use it for reprioritization, and b) > it > > does a linear search for the handle argument. If I successfully move > throttling > > down into the network stack, there'll be a lot more queued requests, and > > potentially a lot more searching on reprioritization. So I think this'll need > > to be fixed, but I'm inclined to worry about it in a separate CL. > > > > * I could easily submit this as a single large CL, or I could break it apart. > I > > think the minimum CL would be this one (the plumbing to but not through the > > socket pools adds real functionality, for all that it doesn't add all the > > functionality you'd want). I'm inclined to submit them as small CLs--i.e. add > > tests to this one and submit, and add plumbing to transport socket pools in a > > separate CL, and to resolvers in a CL after that. But I'd like to know your > > thoughts on appropriate granularity. > > > > * I'm really not looking forward to figuring out how to test all this, though > I > > presume there's no better way than groking each classes unit tests and making > > sure to add tests that cover the new functionality. Having said that, if you > > have any pointers I'm interested. I'm also interested in what thoughts you > > might have about integration tests (tests of groups of these classes together) > > as this CL and the coming CLs will be mostly plumbing, so making sure the > > classes are correctly talking to each other will be important. > > > > That ended up being longer than I expect :-}. When you get a chance, I'm > > curious what you think. As remarked on the other CL, no worries; I have > several > > irons in the fire.
Some preliminary comments. Looks like your new tests are failing. :( https://codereview.chromium.org/1898133002/diff/40001/net/socket/client_socke... File net/socket/client_socket_pool_base.cc (right): https://codereview.chromium.org/1898133002/diff/40001/net/socket/client_socke... net/socket/client_socket_pool_base.cc:533: PendingCallbackMap::iterator callback_it = pending_callback_map_.find(handle); This seems a roundabout way of doing things, and while perf does matter, doing two searches here seems unnecessary (And suppose it could increase binary size, which matters more). Can we just use group_map_.find(group_name), and if it's not found, DCHECK that it's in pending_callback_map_? https://codereview.chromium.org/1898133002/diff/40001/net/socket/client_socke... net/socket/client_socket_pool_base.cc:544: request->set_priority(priority); Can we either DCHECK the priorities are not the same, or do nothing if they are the same? Just helps use keep FIFO ordering if we do nothing when they're the same. https://codereview.chromium.org/1898133002/diff/40001/net/socket/client_socke... net/socket/client_socket_pool_base.cc:544: request->set_priority(priority); DCHECK(!request->ignore_limits())? Decreasing the priority of an ignore limits request isn't really allowed.
https://codereview.chromium.org/1898133002/diff/40001/net/socket/client_socke... File net/socket/client_socket_pool_base.cc (right): https://codereview.chromium.org/1898133002/diff/40001/net/socket/client_socke... net/socket/client_socket_pool_base.cc:533: PendingCallbackMap::iterator callback_it = pending_callback_map_.find(handle); On 2016/04/22 17:04:14, mmenke wrote: > This seems a roundabout way of doing things, and while perf does matter, doing > two searches here seems unnecessary (And suppose it could increase binary size, > which matters more). > > Can we just use group_map_.find(group_name), and if it's not found, DCHECK that > it's in pending_callback_map_? does matter -> does not matter
Other than the facts this is completely wired up, and doesn't have any tests, this looks fine to me, modulo my earlier comments. Happy to offer test suggestions, if you want them.
Description was changed from ========== Add reprioritization to socket pools. BUG=600839 ========== to ========== Add reprioritization to socket pools. BUG=600839 BUG=166689 ==========
On 2016/04/29 18:23:00, mmenke wrote: > Other than the facts this is completely wired up, and doesn't have any tests, > this looks fine to me, modulo my earlier comments. > > Happy to offer test suggestions, if you want them. I'd be interested in any test suggestions you have; I've just started to grok the test structure for the pools.
Patchset #8 (id:140001) has been deleted
Matt: I've incorporated your comments, and am now going to work on tests. You're welcome to wait for tests for your next round of review (though as I say above, I'm interested in any test suggestions you have--groking the test code is about as much effort as groking the application code, and less fun :-}). https://codereview.chromium.org/1898133002/diff/40001/net/socket/client_socke... File net/socket/client_socket_pool_base.cc (right): https://codereview.chromium.org/1898133002/diff/40001/net/socket/client_socke... net/socket/client_socket_pool_base.cc:533: PendingCallbackMap::iterator callback_it = pending_callback_map_.find(handle); On 2016/04/22 17:04:14, mmenke wrote: > This seems a roundabout way of doing things, and while perf does matter, doing > two searches here seems unnecessary (And suppose it could increase binary size, > which matters more). > > Can we just use group_map_.find(group_name), and if it's not found, DCHECK that > it's in pending_callback_map_? Sure, that's fine by me. Done. I did it this way copying the pattern in CancelRequest; should I extend the change there too? https://codereview.chromium.org/1898133002/diff/40001/net/socket/client_socke... net/socket/client_socket_pool_base.cc:533: PendingCallbackMap::iterator callback_it = pending_callback_map_.find(handle); On 2016/04/22 17:05:19, mmenke wrote: > On 2016/04/22 17:04:14, mmenke wrote: > > This seems a roundabout way of doing things, and while perf does matter, doing > > two searches here seems unnecessary (And suppose it could increase binary > size, > > which matters more). > > > > Can we just use group_map_.find(group_name), and if it's not found, DCHECK > that > > it's in pending_callback_map_? > > does matter -> does not matter Acknowledged. https://codereview.chromium.org/1898133002/diff/40001/net/socket/client_socke... net/socket/client_socket_pool_base.cc:544: request->set_priority(priority); On 2016/04/22 17:04:14, mmenke wrote: > DCHECK(!request->ignore_limits())? Decreasing the priority of an ignore limits > request isn't really allowed. Done. https://codereview.chromium.org/1898133002/diff/40001/net/socket/client_socke... net/socket/client_socket_pool_base.cc:544: request->set_priority(priority); On 2016/04/22 17:04:14, mmenke wrote: > Can we either DCHECK the priorities are not the same, or do nothing if they are > the same? Just helps use keep FIFO ordering if we do nothing when they're the > same. Hmmm. I'm not certain how to do nothing if they are the same (which would be my preference); I don't see the Group class as having lookup operators that don't remove the request. Am I missing something? Should I add an interface like that to Group? It's probably just my rationalizing avoiding something that would be tricky to do :-}, but I'm also not sure that I don't want the current semantics. If we ever document the behavior of requests within a priority group, we'll be saying FIFO, with the call to SetPriority for movements between priorities being an insert (i.e. putting things at the end of the queue). It seems like not putting the request in question at the end of the queue if you do a SetPriority to the same priority would violate that pattern.
Not a full review. For the test fixture, I suggest just a max of one request per pool, have the connection succeed asynchronously. Make/modify requests synchronously, and then make sure the expected on gets the socket. I think it's enough to check which request gets a new socket, no need to cover which gets an idle socket when it's returned to the pool. Suggested tests (May be going a bit overboard): * Make a request. Change its priority. Make sure it gets the connection when it completes. * Make a medium and then a low priority request. Make the low priority request have high priority, make sure it gets the first connection. * Make a medium and then a low priority request. Make the low priority request have lowest priority, make sure the first request gets the first connection. * Make a high and then a medium priority request. Make the high priority request have low priority, make sure the medium priority request gets the first connection. * Make a high and then a medium priority request. Make the high priority request have medium priority, make sure the medium priority request gets the first connection. * Make a low priority request and an ignore_limits request, increase priority of the low priority request to max, make sure the ignore_limits request is the one to get the first connection. https://codereview.chromium.org/1898133002/diff/40001/net/socket/client_socke... File net/socket/client_socket_pool_base.cc (right): https://codereview.chromium.org/1898133002/diff/40001/net/socket/client_socke... net/socket/client_socket_pool_base.cc:544: request->set_priority(priority); On 2016/05/08 01:36:19, Randy Smith - Not in Fridays wrote: > On 2016/04/22 17:04:14, mmenke wrote: > > Can we either DCHECK the priorities are not the same, or do nothing if they > are > > the same? Just helps use keep FIFO ordering if we do nothing when they're the > > same. > > Hmmm. I'm not certain how to do nothing if they are the same (which would be my > preference); I don't see the Group class as having lookup operators that don't > remove the request. Am I missing something? Should I add an interface like > that to Group? Looks like you're right. You could just DCHECK they're not the same, and catch them at the URLRequest::SetPriority (Looks like we already catch that case there, actually). > It's probably just my rationalizing avoiding something that would be tricky to > do :-}, but I'm also not sure that I don't want the current semantics. If we > ever document the behavior of requests within a priority group, we'll be saying > FIFO, with the call to SetPriority for movements between priorities being an > insert (i.e. putting things at the end of the queue). It seems like not putting > the request in question at the end of the queue if you do a SetPriority to the > same priority would violate that pattern. But then again, we're already doing that, per above comment. I'm fine with changing behavior in the future, but then we should make sure it affects everything that cares about priority. https://codereview.chromium.org/1898133002/diff/120001/net/socket/client_sock... File net/socket/client_socket_handle.cc (right): https://codereview.chromium.org/1898133002/diff/120001/net/socket/client_sock... net/socket/client_socket_handle.cc:35: if (pool_) When is this called without a pool? https://codereview.chromium.org/1898133002/diff/120001/net/socket/client_sock... File net/socket/client_socket_pool_base.cc (right): https://codereview.chromium.org/1898133002/diff/120001/net/socket/client_sock... net/socket/client_socket_pool_base.cc:544: DCHECK(!request->ignore_limits()); Think this is worth a comment.
Removing myself as a reviewer from CLs that are very stale, and clearly not in my court. Feel free to add me back when/if needed.
mmenke@chromium.org changed reviewers: - mmenke@chromium.org
The CQ bit was checked by rdsmith@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by rdsmith@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Matt: I resurrected this old CL over the break and brought it up to what I hope is striking distance of landing; willing to take another look? A couple of notes: * I'm uncertain of the value of the ignore limits test, because ignore limits means that I don't have a way (that I know about) to keep StartRequest from synchronously progressing to connection establishment. There's probably something I'm missing, so I'll raise a flag to direct your attention that way. * The above is a systemic level race (if a SetPriority comes in between HttpStreamFactoryImpl::Job::DoCreateStream() OnStreamReadyCallback() (separated by a PostTask) it'll be dropped on the floor. I figure that should be handled in a separate CL, but let me know if you disagree. * As per conversation with you and Charles, setting priority to the same value is now a no-op (rather than requeing the requests to the end of the list). Thanks much in advance ... https://codereview.chromium.org/1898133002/diff/120001/net/socket/client_sock... File net/socket/client_socket_handle.cc (right): https://codereview.chromium.org/1898133002/diff/120001/net/socket/client_sock... net/socket/client_socket_handle.cc:35: if (pool_) On 2016/05/09 18:41:03, mmenke (Out Dec 17 to Jan 2) wrote: > When is this called without a pool? Well, as I read the code, HttpStreamFactoryImpl::Job allocates a ClientSocketHandle for itself upon creation. That handle doesn't get passed into the pools for initialization until DoInitConnection. So for all the states before that, the ClientSocketHandle doesn't have a pool. I'd think that URLRequest::SetPriority() could be called anytime during that state machine, and that that would propagate down to this level (and if that's not currently plumbed, it should be, but I think it is :-}). Am I missing something? https://codereview.chromium.org/1898133002/diff/120001/net/socket/client_sock... File net/socket/client_socket_pool_base.cc (right): https://codereview.chromium.org/1898133002/diff/120001/net/socket/client_sock... net/socket/client_socket_pool_base.cc:544: DCHECK(!request->ignore_limits()); On 2016/05/09 18:41:03, mmenke (Out Dec 17 to Jan 2) wrote: > Think this is worth a comment. Done.
rdsmith@chromium.org changed reviewers: + mmenke@chromium.org
Arggh, didn't include the Reviewers change. Matt: PTAL? Actual contentful message in c#23 :-J.
The CQ bit was checked by rdsmith@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...
Two quick related comments, in an old patch set. Going to continue looking at this CL, just thought I'd send them out so they don't get overlooked. Also, a response to the ignore limits thing (Without looking at the relevant code): On 2017/01/03 21:09:08, Randy Smith - Not in Mondays wrote: > Matt: I resurrected this old CL over the break and brought it up to what I hope > is striking distance of landing; willing to take another look? A couple of > notes: > > * I'm uncertain of the value of the ignore limits test, because ignore limits > means that I don't have a way (that I know about) to keep StartRequest from > synchronously progressing to connection establishment. There's probably > something I'm missing, so I'll raise a flag to direct your attention that way. IGNORE_LIMITS is used for sync XHRs, and we don't want to delay requests in those cases, so priority is basically ignored in those cases (And so changing it doesn't make much sense. May even have a DCHECK somewhere that we don't have a priority less than HIGHEST for such requests that would also trigger on priority changes). https://codereview.chromium.org/1898133002/diff/120001/net/socket/client_sock... File net/socket/client_socket_handle.cc (right): https://codereview.chromium.org/1898133002/diff/120001/net/socket/client_sock... net/socket/client_socket_handle.cc:35: if (pool_) On 2017/01/03 21:09:08, Randy Smith - Not in Mondays wrote: > On 2016/05/09 18:41:03, mmenke (Out Dec 17 to Jan 2) wrote: > > When is this called without a pool? > > Well, as I read the code, HttpStreamFactoryImpl::Job allocates a > ClientSocketHandle for itself upon creation. That handle doesn't get passed > into the pools for initialization until DoInitConnection. So for all the states > before that, the ClientSocketHandle doesn't have a pool. I'd think that > URLRequest::SetPriority() could be called anytime during that state machine, and > that that would propagate down to this level (and if that's not currently > plumbed, it should be, but I think it is :-}). > > Am I missing something? You're right, but setting the priority of an "uninitialized" object seems weird (It's safe, but....but...). I wonder if we can just create the ClientSocketHandle in ClientSocketHandle::DoInitConnectionImpl(), and not call SetPriority if ClientSocketHandle is nullptr. https://codereview.chromium.org/1898133002/diff/120001/net/socket/client_sock... net/socket/client_socket_handle.cc:36: pool_->SetPriority(group_name_, this, priority); Also, this is calling SetPriority even after this ClientSocketHandle has a socket, which doesn't seem right. A bit unfortunate that the ClientSocketHandle both serves as a ClientSocketHandle and a SocketRequestHandle, but fixing that is well beyond the scope of this CL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Worth noting that it looks like priority changes aren't hooked up to H2 proxy sockets (For SSL over an H2 proxy). Not volunteering you to fix that, I had just assumed, completely incorrectly, that updating socket pools would fix everything, so we'll need to remember to file a bug for that. Haven't looked at tests yet. https://codereview.chromium.org/1898133002/diff/220001/net/http/http_network_... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/1898133002/diff/220001/net/http/http_network_... net/http/http_network_transaction.cc:428: // already been created). Think this should be clearer. I assume the issue is that everything will wok, except when this class gets the stream_, it will still have the old priority. https://codereview.chromium.org/1898133002/diff/220001/net/http/http_network_... net/http/http_network_transaction.cc:430: // TODO(rdsmith): Note that if throttling is ever implemented below this Know this is an old comment, but I can't remember the meaning...the socket pool is below this layer, and implements a form of throttling. https://codereview.chromium.org/1898133002/diff/220001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1898133002/diff/220001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:321: // connection_ is moved to be owned by the newly created stream "connection_ is moved to be owned by the newly created stream..." -> Maybe "Ownership of |connection_| is passed to the newly created stream or H2 session in DoCreateStream(), and the consumer not notified immediately, so this call may occur when connection_ is null." https://codereview.chromium.org/1898133002/diff/220001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:321: // connection_ is moved to be owned by the newly created stream Also, would it make more sense to put the TODO about fixing that case here? I think all you need to do (Excluding tests) to fix that is: if (stream_) stream_->SetPriority(priority); https://codereview.chromium.org/1898133002/diff/220001/net/socket/client_sock... File net/socket/client_socket_pool_base.cc (right): https://codereview.chromium.org/1898133002/diff/220001/net/socket/client_sock... net/socket/client_socket_pool_base.cc:573: return; Per earlier comment, the ClientSocketHandle knows when this happens. Should that class just early abort the call in that case, and DCHECK here? (The ClientSocketHandle consumer may not know if that's the case, since we may post a task to inform it of completion). https://codereview.chromium.org/1898133002/diff/220001/net/socket/client_sock... net/socket/client_socket_pool_base.cc:1434: } This entire block is just: std::unique_ptr<Request> request = FindAndRemovePendingRequest(); request->set_priority(priority); InsertPendingRequest(std::move(request)); (With some extra DCHECKs) As an added bonus, if we ever decide FindAndRemovePendingRequest is too weird and overhead-y as-is, this will benefit from the change as well (Or vice-versa). We lose the "pointer.priority() == priority" early exit, but that case should never happen, per the NOTREACHED below.
The CQ bit was checked by rdsmith@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rdsmith@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by rdsmith@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I've updated http://crbug.com/166689 with the list of things I think need to happen following this CL (and there are several), and included your note about SSL over an H2 proxy. Having said that, I took a look around the code and I'm not actually sure how that's supposed to work--does the HttpStream returned end up being a SpdyHttpStream? And if so, how is that plumbing different from the straight H2 stream plumbing for after sockets are allocated (which I think is also not done; specifically SpdyHttpStream::SetPriority is currently a no-op)? https://codereview.chromium.org/1898133002/diff/120001/net/socket/client_sock... File net/socket/client_socket_handle.cc (right): https://codereview.chromium.org/1898133002/diff/120001/net/socket/client_sock... net/socket/client_socket_handle.cc:35: if (pool_) On 2017/01/03 21:28:22, mmenke wrote: > On 2017/01/03 21:09:08, Randy Smith - Not in Mondays wrote: > > On 2016/05/09 18:41:03, mmenke (Out Dec 17 to Jan 2) wrote: > > > When is this called without a pool? > > > > Well, as I read the code, HttpStreamFactoryImpl::Job allocates a > > ClientSocketHandle for itself upon creation. That handle doesn't get passed > > into the pools for initialization until DoInitConnection. So for all the > states > > before that, the ClientSocketHandle doesn't have a pool. I'd think that > > URLRequest::SetPriority() could be called anytime during that state machine, > and > > that that would propagate down to this level (and if that's not currently > > plumbed, it should be, but I think it is :-}). > > > > Am I missing something? > > You're right, but setting the priority of an "uninitialized" object seems weird > (It's safe, but....but...). I wonder if we can just create the > ClientSocketHandle in ClientSocketHandle::DoInitConnectionImpl(), and not call > SetPriority if ClientSocketHandle is nullptr. So I looked into this option in response to your comment, and it makes me nervous in the "there's something here I don't understand" way. The ClientSocketHandle::Reset() is called at various points in HttpStreamFactoryImpl::Job, I presume so that the state diagram can wrap back up to before it was initialized (because why else would you bother). Those Reset() calls would turn into frees (client_socket_handle_->Reset() ==> client_socket_handle.reset()), but that brings up the question of why Reset() exists at all--the delete and recreate idiom strikes me as more natural. It may be for historical reasons or because ClientSocketHandle needs to be reset sometimes when it's a data member for a class (though why isn't that true here if so?), but it might also be because there's some hidden cost about deallocating and reallocating the object. I looked at just forbidding setting the priority of an uninitialized ClientSocketHandle (ClientSocketHandle::Init() takes a priority and HttpStreamFactoryImpl::Job stores the priority, so I can just no-op the HSFI::J::SetPriority call if ClientSocketHandle hasn't been initialized yet), but the problem is that an error return of a ClientSocketHandle from the pools will put it into an uninitialized state, which makes me feel like I can't forbid consumers from calling SetPriority in an uninitialized state. I could say something like "SetPriority() may not be called unless Init() has previously been called, has not completed in error, and Reset() has not been called" but that's a) a pretty complicated contract, and b) requires the consumer to track whether or not it's received the callback and the initialization has completed in error. So I think this is the least bad option? But I'm open to perspectives I haven't come up with, or just plain argument :-}. https://codereview.chromium.org/1898133002/diff/120001/net/socket/client_sock... net/socket/client_socket_handle.cc:36: pool_->SetPriority(group_name_, this, priority); On 2017/01/03 21:28:22, mmenke wrote: > Also, this is calling SetPriority even after this ClientSocketHandle has a > socket, which doesn't seem right. A bit unfortunate that the ClientSocketHandle > both serves as a ClientSocketHandle and a SocketRequestHandle, but fixing that > is well beyond the scope of this CL. It's worse than doesn't seem right--it can produce an error if the pool has no knowledge of the request (as happens after the socket's been allocated). I fixed that in the most recent patchset, but by making SetPriority() a no-op if there's a socket. I'm inclined to think that that is the right choice, as I could conceivably imagine a meaning for SetPriority() on a ClientSocketHandle with a socket (it could affect TCP params). I have no intention of implementing something like that in the short term, but it makes me think the right solution is no-op'ing in the implementation rather than forbidding the call in the API. Probably not an issue for this CL, as you say, but WDYT of that argument? https://codereview.chromium.org/1898133002/diff/220001/net/http/http_network_... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/1898133002/diff/220001/net/http/http_network_... net/http/http_network_transaction.cc:428: // already been created). On 2017/01/03 22:51:22, mmenke wrote: > Think this should be clearer. I assume the issue is that everything will wok, > except when this class gets the stream_, it will still have the old priority. Yep. I've reworded the comment; let me know what you think. https://codereview.chromium.org/1898133002/diff/220001/net/http/http_network_... net/http/http_network_transaction.cc:430: // TODO(rdsmith): Note that if throttling is ever implemented below this On 2017/01/03 22:51:22, mmenke wrote: > Know this is an old comment, but I can't remember the meaning...the socket pool > is below this layer, and implements a form of throttling. I've added " ... if *priority based* throttling ..." to the comment; let me know if you need more. The basic issue is that if the SetPriority calls can result in upcalls that nuke requests, this code will have problems. Currently this routine depends on the only thing that can delete *this being the throttle->SetPriority() call. https://codereview.chromium.org/1898133002/diff/220001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1898133002/diff/220001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:321: // connection_ is moved to be owned by the newly created stream On 2017/01/03 22:51:22, mmenke wrote: > "connection_ is moved to be owned by the newly created stream..." -> Maybe > "Ownership of |connection_| is passed to the newly created stream or H2 session > in DoCreateStream(), and the consumer not notified immediately, so this call may > occur when connection_ is null." Done. https://codereview.chromium.org/1898133002/diff/220001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:321: // connection_ is moved to be owned by the newly created stream On 2017/01/03 22:51:22, mmenke wrote: > Also, would it make more sense to put the TODO about fixing that case here? I > think all you need to do (Excluding tests) to fix that is: > > if (stream_) > stream_->SetPriority(priority); Huh; I had thought the PostTask() went to an object method above this one, but it's actually just to OnStreamReadyCallback(). Yes, I think that would work fine, and I think it's a better API contract (i.e. that the consumer should always be able to affect the priority via one of stream_request_ or stream_). Do you see any way to test it? There aren't currently any HttpStreamFactoryImpl::Job unit tests, which I think I could make work; without that (specifically without notification of connection establishment separate from stream ready) I don't know how to get into the state where the PostTask is in flight. Failing seeing how to test it (relatively easily--this CL is a distraction from my main tasks ATM) I've moved the TODO. https://codereview.chromium.org/1898133002/diff/220001/net/socket/client_sock... File net/socket/client_socket_pool_base.cc (right): https://codereview.chromium.org/1898133002/diff/220001/net/socket/client_sock... net/socket/client_socket_pool_base.cc:573: return; On 2017/01/03 22:51:22, mmenke wrote: > Per earlier comment, the ClientSocketHandle knows when this happens. Should > that class just early abort the call in that case, and DCHECK here? (The > ClientSocketHandle consumer may not know if that's the case, since we may post a > task to inform it of completion). I'm embarrassed to say that when I first read your comment, I thought I knew what you meant but there was a trickiness to it, but coming back I've lost that sense completely :-}. So I'll speak from my current perspective and D'oh! if needed. If the handle is still in pending_callback_map_, that means that the callback hasn't yet occurred, which means that the state in the ClientSocketHandle is still "socket allocation pending" and it doesn't know not to call into the pool to set the priority. The request may have been completed and been destroyed before the consumer is informed of the request result. So I think we need this check. Am I missing something? https://codereview.chromium.org/1898133002/diff/220001/net/socket/client_sock... net/socket/client_socket_pool_base.cc:1434: } On 2017/01/03 22:51:22, mmenke wrote: > This entire block is just: > > std::unique_ptr<Request> request = FindAndRemovePendingRequest(); > request->set_priority(priority); > InsertPendingRequest(std::move(request)); > > (With some extra DCHECKs) > > As an added bonus, if we ever decide FindAndRemovePendingRequest is too weird > and overhead-y as-is, this will benefit from the change as well (Or vice-versa). > > We lose the "pointer.priority() == priority" early exit, but that case should > never happen, per the NOTREACHED below. The entire reason for this code is indeed the case where the priority is being set to the already existing priority, in response to yours and Charles' comments about not wanting that case to have any effect (and my reluctance to forbid that at this API level and implicitly rely on the short-circuit in URLRequest::SetPriority). If you object to the extra code for a case that won't happen in the name of API cleanliness, I'll reluctantly fix it up (an API contract saying "This one specific value that you can't query isn't allowed" bothers me) , but I don't see what it has to do with the NOTREACHED() below? That's for whether or not the socket's in the group's list.
Sorry for slowness, still haven't looked at tests. Kinda buried under reviews this week. Doesn't seem like I really have *that* many, but do seem to be taking up a fair bit of time. https://codereview.chromium.org/1898133002/diff/280001/net/http/http_network_... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/1898133002/diff/280001/net/http/http_network_... net/http/http_network_transaction.cc:429: // be modified. Ok, now I understand what you mean because of your response to me, but I still think the socket pools implement priority-based throttling. Once the pool limit is reached, the group with the highest priority open socket request gets the next socket slot. Maybe be more explicit: "Note that if any level below this ever implements a throttling mechanism where changing a request's priority may cause *any* request to synchronously succeed or fail, that callback could synchronously delete |this|, and this code could potentially crash. In that case, this code will need to be modified." https://codereview.chromium.org/1898133002/diff/280001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1898133002/diff/280001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:326: // make sure the above race doesn't drop a reprioritization on the floor. On 2017/01/05 03:47:18, Randy Smith - Not in Mondays wrote: > On 2017/01/03 22:51:22, mmenke wrote: > > Also, would it make more sense to put the TODO about fixing that case here? I > > think all you need to do (Excluding tests) to fix that is: > > > > if (stream_) > > stream_->SetPriority(priority); > > Huh; I had thought the PostTask() went to an object method above this one, but > it's actually just to OnStreamReadyCallback(). Yes, I think that would work > fine, and I think it's a better API contract (i.e. that the consumer should > always be able to affect the priority via one of stream_request_ or stream_). > > Do you see any way to test it? There aren't currently any > HttpStreamFactoryImpl::Job unit tests, which I think I could make work; without > that (specifically without notification of connection establishment separate > from stream ready) I don't know how to get into the state where the PostTask is > in flight. > > Failing seeing how to test it (relatively easily--this CL is a distraction from > my main tasks ATM) I've moved the TODO. I see some ways to test it, but nothing that seems sane. Mock SSL socket pool, mock SSL socket, set it so we get the handshake message that tells us we have an H2 connection on demand (That message may actually be bypassed, so need to connect on demand - if so, need to implement that. IF it's just a mock read that does that, that's already supported). Then once you let the handshake complete, the session and stream will by synchronously created, so you can just set the priority at that point, and then make sure the right priority is sent over the wire. Could alternative mock out the SpdySessionPool/SpdySession and but no tests do that. Another option would be to make HttpStream's priority observable (And HttpBasicStream track the priority), but not sure we want to add too much stuff to the HttpStreamInterface. That would work as an HttpStreamFactoryImpl test. Getting in before the callback would require the same strategy as above, with the extra H2 stuff. https://codereview.chromium.org/1898133002/diff/280001/net/socket/client_sock... File net/socket/client_socket_handle.cc (right): https://codereview.chromium.org/1898133002/diff/280001/net/socket/client_sock... net/socket/client_socket_handle.cc:40: // params on this socket. TransportSocketParams don't have a priority, no socket params classes do, actually, I believe. The only socket class where priority matters would be for SpdyProxyClientSocket (And the QUIC equivalent, if there is one), since they can set their priority on the undlerying SPDY/QUIC session. https://codereview.chromium.org/1898133002/diff/280001/net/socket/client_sock... net/socket/client_socket_handle.cc:44: if (pool_) On 2017/01/05 03:47:18, Randy Smith - Not in Mondays wrote: > On 2017/01/03 21:28:22, mmenke wrote: > > On 2017/01/03 21:09:08, Randy Smith - Not in Mondays wrote: > > > On 2016/05/09 18:41:03, mmenke (Out Dec 17 to Jan 2) wrote: > > > > When is this called without a pool? > > > > > > Well, as I read the code, HttpStreamFactoryImpl::Job allocates a > > > ClientSocketHandle for itself upon creation. That handle doesn't get passed > > > into the pools for initialization until DoInitConnection. So for all the > > states > > > before that, the ClientSocketHandle doesn't have a pool. I'd think that > > > URLRequest::SetPriority() could be called anytime during that state machine, > > and > > > that that would propagate down to this level (and if that's not currently > > > plumbed, it should be, but I think it is :-}). > > > > > > Am I missing something? > > > > You're right, but setting the priority of an "uninitialized" object seems > weird > > (It's safe, but....but...). I wonder if we can just create the > > ClientSocketHandle in ClientSocketHandle::DoInitConnectionImpl(), and not call > > SetPriority if ClientSocketHandle is nullptr. > > So I looked into this option in response to your comment, and it makes me > nervous in the "there's something here I don't understand" way. The > ClientSocketHandle::Reset() is called at various points in > HttpStreamFactoryImpl::Job, I presume so that the state diagram can wrap back up > to before it was initialized (because why else would you bother). Those Reset() > calls would turn into frees (client_socket_handle_->Reset() ==> > client_socket_handle.reset()), but that brings up the question of why Reset() > exists at all--the delete and recreate idiom strikes me as more natural. It may > be for historical reasons or because ClientSocketHandle needs to be reset > sometimes when it's a data member for a class (though why isn't that true here > if so?), but it might also be because there's some hidden cost about > deallocating and reallocating the object. Just destroying the socket seems to return it to the same state as a newly created socket, so I can't see any reason why we can't just delete it. There may once have been a reason for it, which no longer exists. It could even have something to do with the old callback system, and/or the fact most net objects were once refcounted. I think I may try the cleanup, and see what happens (Mostly because it seems really simple just to change usage from the Job and see what happens - if things look good, Reset() is only used in about a dozen files, though those files do have a lot of references) > I looked at just forbidding setting the priority of an uninitialized > ClientSocketHandle (ClientSocketHandle::Init() takes a priority and > HttpStreamFactoryImpl::Job stores the priority, so I can just no-op the > HSFI::J::SetPriority call if ClientSocketHandle hasn't been initialized yet), > but the problem is that an error return of a ClientSocketHandle from the pools > will put it into an uninitialized state, which makes me feel like I can't forbid > consumers from calling SetPriority in an uninitialized state. I could say > something like "SetPriority() may not be called unless Init() has previously > been called, has not completed in error, and Reset() has not been called" but > that's a) a pretty complicated contract, and b) requires the consumer to track > whether or not it's received the callback and the initialization has completed > in error. Hrm... You're right that a ClientSocketHandle for a failed socket request without a socket is indistinguishable from one that has yet to be initialized, which seems rather unfortunate. Requiring a client not call a method on a handle that's not in a connecting state (from its viewpoint) seems like a pretty reasonable contract to me, the only problem I see with it is that we can't enforce it. > So I think this is the least bad option? But I'm open to perspectives I haven't > come up with, or just plain argument :-}. From my standpoint, something with is_initialized == false is logically equivalent to nullptr, so modifying it is an anti-pattern. Also, other than this method, I think ClientSocketHandles, once connected (And even before, actually) are basically immutable from the point of view of the consumer, other than Reset, and method calls on the socket. https://codereview.chromium.org/1898133002/diff/280001/net/socket/client_sock... net/socket/client_socket_handle.cc:45: pool_->SetPriority(group_name_, this, priority); On 2017/01/05 03:47:18, Randy Smith - Not in Mondays wrote: > On 2017/01/03 21:28:22, mmenke wrote: > > Also, this is calling SetPriority even after this ClientSocketHandle has a > > socket, which doesn't seem right. A bit unfortunate that the > ClientSocketHandle > > both serves as a ClientSocketHandle and a SocketRequestHandle, but fixing > that > > is well beyond the scope of this CL. > > It's worse than doesn't seem right--it can produce an error if the pool has no > knowledge of the request (as happens after the socket's been allocated). I > fixed that in the most recent patchset, but by making SetPriority() a no-op if > there's a socket. > > I'm inclined to think that that is the right choice, as I could conceivably > imagine a meaning for SetPriority() on a ClientSocketHandle with a socket (it > could affect TCP params). I have no intention of implementing something like > that in the short term, but it makes me think the right solution is no-op'ing in > the implementation rather than forbidding the call in the API. Probably not an > issue for this CL, as you say, but WDYT of that argument? Let me try and get a better...handle... on the life[and]times of ClientSocketHandles, and I'll get back to you on that.
Two quick responses, longer response later: On 2017/01/05 20:18:35, mmenke wrote: > Sorry for slowness, still haven't looked at tests. Kinda buried under reviews > this week. Doesn't seem like I really have *that* many, but do seem to be > taking up a fair bit of time. Don't worry about this CL; it's not high priority on my side either. I'd like to get it done, but that's more because "sunk costs" than anything else. > From my standpoint, something with is_initialized == false is logically > equivalent to nullptr, so modifying it is an anti-pattern. Also, other than > this method, I think ClientSocketHandles, once connected (And even before, > actually) are basically immutable from the point of view of the consumer, other > than Reset, and method calls on the socket. The problem with this argument is that is_initialized == false between the call to Init() and the async response, and that's precisely the period of time when we most want to propagate the reprioritization.
On 2017/01/05 20:28:36, Randy Smith - Not in Mondays wrote: > Two quick responses, longer response later: > > On 2017/01/05 20:18:35, mmenke wrote: > > Sorry for slowness, still haven't looked at tests. Kinda buried under reviews > > this week. Doesn't seem like I really have *that* many, but do seem to be > > taking up a fair bit of time. > > Don't worry about this CL; it's not high priority on my side either. I'd like > to get it done, but that's more because "sunk costs" than anything else. > > > > From my standpoint, something with is_initialized == false is logically > > equivalent to nullptr, so modifying it is an anti-pattern. Also, other than > > this method, I think ClientSocketHandles, once connected (And even before, > > actually) are basically immutable from the point of view of the consumer, > other > > than Reset, and method calls on the socket. > > The problem with this argument is that is_initialized == false between the call > to Init() and the async response, and that's precisely the period of time when > we most want to propagate the reprioritization. Right, but the call to Init basically puts it in an initializing state.
On 2017/01/05 20:49:56, mmenke wrote: > On 2017/01/05 20:28:36, Randy Smith - Not in Mondays wrote: > > Two quick responses, longer response later: > > > > On 2017/01/05 20:18:35, mmenke wrote: > > > Sorry for slowness, still haven't looked at tests. Kinda buried under > reviews > > > this week. Doesn't seem like I really have *that* many, but do seem to be > > > taking up a fair bit of time. > > > > Don't worry about this CL; it's not high priority on my side either. I'd like > > to get it done, but that's more because "sunk costs" than anything else. > > > > > > > From my standpoint, something with is_initialized == false is logically > > > equivalent to nullptr, so modifying it is an anti-pattern. Also, other than > > > this method, I think ClientSocketHandles, once connected (And even before, > > > actually) are basically immutable from the point of view of the consumer, > > other > > > than Reset, and method calls on the socket. > > > > The problem with this argument is that is_initialized == false between the > call > > to Init() and the async response, and that's precisely the period of time when > > we most want to propagate the reprioritization. > > Right, but the call to Init basically puts it in an initializing state. Which isn't probe-able, so I'm reluctant to put API restrictions that depend on it. Though yes, the consumer was the one who put it into that state and who presumably would have to obey the API restrictions, but it still feels not-API-goodness to me.
On 2017/01/05 22:08:04, Randy Smith - Not in Mondays wrote: > On 2017/01/05 20:49:56, mmenke wrote: > > On 2017/01/05 20:28:36, Randy Smith - Not in Mondays wrote: > > > Two quick responses, longer response later: > > > > > > On 2017/01/05 20:18:35, mmenke wrote: > > > > Sorry for slowness, still haven't looked at tests. Kinda buried under > > reviews > > > > this week. Doesn't seem like I really have *that* many, but do seem to be > > > > taking up a fair bit of time. > > > > > > Don't worry about this CL; it's not high priority on my side either. I'd > like > > > to get it done, but that's more because "sunk costs" than anything else. > > > > > > > > > > From my standpoint, something with is_initialized == false is logically > > > > equivalent to nullptr, so modifying it is an anti-pattern. Also, other > than > > > > this method, I think ClientSocketHandles, once connected (And even before, > > > > actually) are basically immutable from the point of view of the consumer, > > > other > > > > than Reset, and method calls on the socket. > > > > > > The problem with this argument is that is_initialized == false between the > > call > > > to Init() and the async response, and that's precisely the period of time > when > > > we most want to propagate the reprioritization. > > > > Right, but the call to Init basically puts it in an initializing state. > > Which isn't probe-able, so I'm reluctant to put API restrictions that depend on > it. Though yes, the consumer was the one who put it into that state and who > presumably would have to obey the API restrictions, but it still feels > not-API-goodness to me. My quick investigation of not keeping around uninitialized ClientSocketHandles: Some of them have metadata (SSL error information). Not sure if it's indelibly bound to the ClientSocketHandle, or could easily be returned as metadata on completion, but it prevents just trivially deleting uninitialized ClientSocketHandles on init completion.
https://codereview.chromium.org/1898133002/diff/280001/net/http/http_stream_f... File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/1898133002/diff/280001/net/http/http_stream_f... net/http/http_stream_factory_impl_unittest.cc:1518: // may be called on a stream request after the stream has been returned. Is this really needed by the API? Should be possible to call it once the stream has been created, and before it's been returned. But not sure abotu after it's been returned. https://codereview.chromium.org/1898133002/diff/280001/net/http/http_stream_f... net/http/http_stream_factory_impl_unittest.cc:1547: // Crash test Should not crash test? https://codereview.chromium.org/1898133002/diff/280001/net/socket/client_sock... File net/socket/client_socket_pool_base_unittest.cc (right): https://codereview.chromium.org/1898133002/diff/280001/net/socket/client_sock... net/socket/client_socket_pool_base_unittest.cc:1113: request(1)->handle()->SetPriority(HIGHEST); This doesn't test anything, does it? request 2 has already completed, so request 1 can't jump ahead of it without some sort of time machine, at this point. We'd really need asynchronous sockets to get much out of this.
On 2017/01/05 20:18:35, mmenke wrote: > https://codereview.chromium.org/1898133002/diff/280001/net/http/http_stream_f... > net/http/http_stream_factory_impl_job.cc:326: // make sure the above race > doesn't drop a reprioritization on the floor. > On 2017/01/05 03:47:18, Randy Smith - Not in Mondays wrote: > > On 2017/01/03 22:51:22, mmenke wrote: > > > Also, would it make more sense to put the TODO about fixing that case here? > I > > > think all you need to do (Excluding tests) to fix that is: > > > > > > if (stream_) > > > stream_->SetPriority(priority); > > > > Huh; I had thought the PostTask() went to an object method above this one, but > > it's actually just to OnStreamReadyCallback(). Yes, I think that would work > > fine, and I think it's a better API contract (i.e. that the consumer should > > always be able to affect the priority via one of stream_request_ or stream_). > > > > > Do you see any way to test it? There aren't currently any > > HttpStreamFactoryImpl::Job unit tests, which I think I could make work; > without > > that (specifically without notification of connection establishment separate > > from stream ready) I don't know how to get into the state where the PostTask > is > > in flight. > > > > Failing seeing how to test it (relatively easily--this CL is a distraction > from > > my main tasks ATM) I've moved the TODO. > > I see some ways to test it, but nothing that seems sane. > > Mock SSL socket pool, mock SSL socket, set it so we get the handshake message > that tells us we have an H2 connection on demand (That message may actually be > bypassed, so need to connect on demand - if so, need to implement that. IF it's > just a mock read that does that, that's already supported). Then once you let > the handshake complete, the session and stream will by synchronously created, so > you can just set the priority at that point, and then make sure the right > priority is sent over the wire. > > Could alternative mock out the SpdySessionPool/SpdySession and but no tests do > that. > > Another option would be to make HttpStream's priority observable (And > HttpBasicStream track the priority), but not sure we want to add too much stuff > to the HttpStreamInterface. That would work as an HttpStreamFactoryImpl test. > Getting in before the callback would require the same strategy as above, with > the extra H2 stuff. *chuckle/sigh* Given that, how would you feel about my putting in the stream priority setting without the tests? It easily fills a known hole and isn't a lot of code or very complex code to leave untested.
On 2017/01/13 00:47:50, Randy Smith - Not in Mondays wrote: > On 2017/01/05 20:18:35, mmenke wrote: > > > > https://codereview.chromium.org/1898133002/diff/280001/net/http/http_stream_f... > > net/http/http_stream_factory_impl_job.cc:326: // make sure the above race > > doesn't drop a reprioritization on the floor. > > On 2017/01/05 03:47:18, Randy Smith - Not in Mondays wrote: > > > On 2017/01/03 22:51:22, mmenke wrote: > > > > Also, would it make more sense to put the TODO about fixing that case > here? > > I > > > > think all you need to do (Excluding tests) to fix that is: > > > > > > > > if (stream_) > > > > stream_->SetPriority(priority); > > > > > > Huh; I had thought the PostTask() went to an object method above this one, > but > > > it's actually just to OnStreamReadyCallback(). Yes, I think that would work > > > fine, and I think it's a better API contract (i.e. that the consumer should > > > always be able to affect the priority via one of stream_request_ or > stream_). > > > > > > > > Do you see any way to test it? There aren't currently any > > > HttpStreamFactoryImpl::Job unit tests, which I think I could make work; > > without > > > that (specifically without notification of connection establishment separate > > > from stream ready) I don't know how to get into the state where the PostTask > > is > > > in flight. > > > > > > Failing seeing how to test it (relatively easily--this CL is a distraction > > from > > > my main tasks ATM) I've moved the TODO. > > > > I see some ways to test it, but nothing that seems sane. > > > > Mock SSL socket pool, mock SSL socket, set it so we get the handshake message > > that tells us we have an H2 connection on demand (That message may actually be > > bypassed, so need to connect on demand - if so, need to implement that. IF > it's > > just a mock read that does that, that's already supported). Then once you let > > the handshake complete, the session and stream will by synchronously created, > so > > you can just set the priority at that point, and then make sure the right > > priority is sent over the wire. > > > > Could alternative mock out the SpdySessionPool/SpdySession and but no tests do > > that. > > > > Another option would be to make HttpStream's priority observable (And > > HttpBasicStream track the priority), but not sure we want to add too much > stuff > > to the HttpStreamInterface. That would work as an HttpStreamFactoryImpl test. > > > Getting in before the callback would require the same strategy as above, with > > the extra H2 stuff. > > *chuckle/sigh* Given that, how would you feel about my putting in the stream > priority setting without the tests? It easily fills a known hole and isn't a > lot of code or very complex code to leave untested. Actually, I think it's much simpler than I was thinking - just make connecting the socket succeed synchronously, send the connection request to the HttpStreamFactory, reprioritize, and make sure the reprioritization was honored (That last part may be hard, but the first part is not). I forgot that HttpStreamFactory couldn't succeed synchronously.
On 2017/01/13 02:39:09, mmenke wrote: > On 2017/01/13 00:47:50, Randy Smith - Not in Mondays wrote: > > On 2017/01/05 20:18:35, mmenke wrote: > > > > > > > > https://codereview.chromium.org/1898133002/diff/280001/net/http/http_stream_f... > > > net/http/http_stream_factory_impl_job.cc:326: // make sure the above race > > > doesn't drop a reprioritization on the floor. > > > On 2017/01/05 03:47:18, Randy Smith - Not in Mondays wrote: > > > > On 2017/01/03 22:51:22, mmenke wrote: > > > > > Also, would it make more sense to put the TODO about fixing that case > > here? > > > I > > > > > think all you need to do (Excluding tests) to fix that is: > > > > > > > > > > if (stream_) > > > > > stream_->SetPriority(priority); > > > > > > > > Huh; I had thought the PostTask() went to an object method above this one, > > but > > > > it's actually just to OnStreamReadyCallback(). Yes, I think that would > work > > > > fine, and I think it's a better API contract (i.e. that the consumer > should > > > > always be able to affect the priority via one of stream_request_ or > > stream_). > > > > > > > > > > > Do you see any way to test it? There aren't currently any > > > > HttpStreamFactoryImpl::Job unit tests, which I think I could make work; > > > without > > > > that (specifically without notification of connection establishment > separate > > > > from stream ready) I don't know how to get into the state where the > PostTask > > > is > > > > in flight. > > > > > > > > Failing seeing how to test it (relatively easily--this CL is a distraction > > > from > > > > my main tasks ATM) I've moved the TODO. > > > > > > I see some ways to test it, but nothing that seems sane. > > > > > > Mock SSL socket pool, mock SSL socket, set it so we get the handshake > message > > > that tells us we have an H2 connection on demand (That message may actually > be > > > bypassed, so need to connect on demand - if so, need to implement that. IF > > it's > > > just a mock read that does that, that's already supported). Then once you > let > > > the handshake complete, the session and stream will by synchronously > created, > > so > > > you can just set the priority at that point, and then make sure the right > > > priority is sent over the wire. > > > > > > Could alternative mock out the SpdySessionPool/SpdySession and but no tests > do > > > that. > > > > > > Another option would be to make HttpStream's priority observable (And > > > HttpBasicStream track the priority), but not sure we want to add too much > > stuff > > > to the HttpStreamInterface. That would work as an HttpStreamFactoryImpl > test. > > > > > Getting in before the callback would require the same strategy as above, > with > > > the extra H2 stuff. > > > > *chuckle/sigh* Given that, how would you feel about my putting in the stream > > priority setting without the tests? It easily fills a known hole and isn't a > > lot of code or very complex code to leave untested. > > Actually, I think it's much simpler than I was thinking - just make connecting > the socket succeed synchronously, send the connection request to the > HttpStreamFactory, reprioritize, and make sure the reprioritization was honored > (That last part may be hard, but the first part is not). I forgot that > HttpStreamFactory couldn't succeed synchronously. Anyhow, I think this case is obscure enough that I can live without a test...The only problem here is that this is priority, so if it regresses, odds are it will be 5 years before anyone notices the breakage, even if not doing things right here ends up mattering more than I suspect it does currently.
The CQ bit was checked by rdsmith@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...
Matt: I *think* I've responded on all the threads we had up in the air, but there were several so you might want to keep an eye out for issues you raised I've missed. Also, re-ping on my request in c#44 for the expected implementation path for SSL over an H2 proxy? I don't think it's relevant to this CL, but I'd still like to understand it. https://codereview.chromium.org/1898133002/diff/220001/net/socket/client_sock... File net/socket/client_socket_pool_base.cc (right): https://codereview.chromium.org/1898133002/diff/220001/net/socket/client_sock... net/socket/client_socket_pool_base.cc:1434: } On 2017/01/05 03:47:18, Randy Smith - Not in Mondays wrote: > On 2017/01/03 22:51:22, mmenke wrote: > > This entire block is just: > > > > std::unique_ptr<Request> request = FindAndRemovePendingRequest(); > > request->set_priority(priority); > > InsertPendingRequest(std::move(request)); > > > > (With some extra DCHECKs) > > > > As an added bonus, if we ever decide FindAndRemovePendingRequest is too weird > > and overhead-y as-is, this will benefit from the change as well (Or > vice-versa). > > > > We lose the "pointer.priority() == priority" early exit, but that case should > > never happen, per the NOTREACHED below. > > The entire reason for this code is indeed the case where the priority is being > set to the already existing priority, in response to yours and Charles' comments > about not wanting that case to have any effect (and my reluctance to forbid that > at this API level and implicitly rely on the short-circuit in > URLRequest::SetPriority). If you object to the extra code for a case that won't > happen in the name of API cleanliness, I'll reluctantly fix it up (an API > contract saying "This one specific value that you can't query isn't allowed" > bothers me) , but I don't see what it has to do with the NOTREACHED() below? > That's for whether or not the socket's in the group's list. Ping? https://codereview.chromium.org/1898133002/diff/280001/net/http/http_network_... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/1898133002/diff/280001/net/http/http_network_... net/http/http_network_transaction.cc:429: // be modified. On 2017/01/05 20:18:35, mmenke wrote: > Ok, now I understand what you mean because of your response to me, but I still > think the socket pools implement priority-based throttling. Once the pool limit > is reached, the group with the highest priority open socket request gets the > next socket slot. > > Maybe be more explicit: "Note that if any level below this ever implements a > throttling mechanism where changing a request's priority may cause *any* request > to synchronously succeed or fail, that callback could synchronously delete > |this|, and this code could potentially crash. In that case, this code will > need to be modified." Done, though I felt a need to be a bit wordier once I got going. Let me know what you think. https://codereview.chromium.org/1898133002/diff/280001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1898133002/diff/280001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:326: // make sure the above race doesn't drop a reprioritization on the floor. On 2017/01/05 20:18:35, mmenke wrote: > On 2017/01/05 03:47:18, Randy Smith - Not in Mondays wrote: > > On 2017/01/03 22:51:22, mmenke wrote: > > > Also, would it make more sense to put the TODO about fixing that case here? > I > > > think all you need to do (Excluding tests) to fix that is: > > > > > > if (stream_) > > > stream_->SetPriority(priority); > > > > Huh; I had thought the PostTask() went to an object method above this one, but > > it's actually just to OnStreamReadyCallback(). Yes, I think that would work > > fine, and I think it's a better API contract (i.e. that the consumer should > > always be able to affect the priority via one of stream_request_ or stream_). > > > > > Do you see any way to test it? There aren't currently any > > HttpStreamFactoryImpl::Job unit tests, which I think I could make work; > without > > that (specifically without notification of connection establishment separate > > from stream ready) I don't know how to get into the state where the PostTask > is > > in flight. > > > > Failing seeing how to test it (relatively easily--this CL is a distraction > from > > my main tasks ATM) I've moved the TODO. > > I see some ways to test it, but nothing that seems sane. > > Mock SSL socket pool, mock SSL socket, set it so we get the handshake message > that tells us we have an H2 connection on demand (That message may actually be > bypassed, so need to connect on demand - if so, need to implement that. IF it's > just a mock read that does that, that's already supported). Then once you let > the handshake complete, the session and stream will by synchronously created, so > you can just set the priority at that point, and then make sure the right > priority is sent over the wire. > > Could alternative mock out the SpdySessionPool/SpdySession and but no tests do > that. > > Another option would be to make HttpStream's priority observable (And > HttpBasicStream track the priority), but not sure we want to add too much stuff > to the HttpStreamInterface. That would work as an HttpStreamFactoryImpl test. > Getting in before the callback would require the same strategy as above, with > the extra H2 stuff. You know, unless you object after reading the following points, I'm going to say heck with this case and completely remove the TODO. Consider: The HttpStream API has the consumer of the stream specifying the priority via InitializeStream, *not* the constructor (also true for the HttpBasicStream constructor, and the other constructors in the subclass tree I spot-checked). InitializeStream is called from HttpNetworkSession, long after HttpStreamFactoryImpl::Job completes, and before (I believe) we actually do anything with the stream. So I think the conceptual presentation of HttpStream is "It doesn't have a priority until it's initialized" and that anything that needs the HttpStream priority should be done after it's initialized. Thus I think that SetPriority shouldn't be called before initialization, and the right thing to do with HttpStreamFactoryImpl::Job::SetPriority after the stream is created is to drop it on the floor. This means that I still want a crash test (to make sure I don't indirect through the non-existent stream request) but that's easier. https://codereview.chromium.org/1898133002/diff/280001/net/http/http_stream_f... File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/1898133002/diff/280001/net/http/http_stream_f... net/http/http_stream_factory_impl_unittest.cc:1518: // may be called on a stream request after the stream has been returned. On 2017/01/06 20:48:54, mmenke wrote: > Is this really needed by the API? Should be possible to call it once the stream > has been created, and before it's been returned. But not sure abotu after it's > been returned. As phrased, not, but I wanted to do a crash test about the behavior of the Job at the very end of the allocation process, and it looked like the code was the same after it's returned. I've replaced this test with one that calls SetPriority before the consumer's actually gotten the stream--your comments on another thread showed me how to do that. So I think the test that's now in the CL is a clean API test. https://codereview.chromium.org/1898133002/diff/280001/net/http/http_stream_f... net/http/http_stream_factory_impl_unittest.cc:1547: // Crash test On 2017/01/06 20:48:54, mmenke wrote: > Should not crash test? Done (in the new test). https://codereview.chromium.org/1898133002/diff/280001/net/socket/client_sock... File net/socket/client_socket_handle.cc (right): https://codereview.chromium.org/1898133002/diff/280001/net/socket/client_sock... net/socket/client_socket_handle.cc:40: // params on this socket. On 2017/01/05 20:18:35, mmenke wrote: > TransportSocketParams don't have a priority, no socket params classes do, > actually, I believe. The only socket class where priority matters would be for > SpdyProxyClientSocket (And the QUIC equivalent, if there is one), since they can > set their priority on the undlerying SPDY/QUIC session. Sorry, didn't mean TransportSocketParams, more TCP Window Size and things like that. But I'm going to remove the TODO; integrating priority into things like TCP Window Size would be its own, large, project. https://codereview.chromium.org/1898133002/diff/280001/net/socket/client_sock... net/socket/client_socket_handle.cc:44: if (pool_) On 2017/01/05 20:18:35, mmenke wrote: > On 2017/01/05 03:47:18, Randy Smith - Not in Mondays wrote: > > On 2017/01/03 21:28:22, mmenke wrote: > > > On 2017/01/03 21:09:08, Randy Smith - Not in Mondays wrote: > > > > On 2016/05/09 18:41:03, mmenke (Out Dec 17 to Jan 2) wrote: > > > > > When is this called without a pool? > > > > > > > > Well, as I read the code, HttpStreamFactoryImpl::Job allocates a > > > > ClientSocketHandle for itself upon creation. That handle doesn't get > passed > > > > into the pools for initialization until DoInitConnection. So for all the > > > states > > > > before that, the ClientSocketHandle doesn't have a pool. I'd think that > > > > URLRequest::SetPriority() could be called anytime during that state > machine, > > > and > > > > that that would propagate down to this level (and if that's not currently > > > > plumbed, it should be, but I think it is :-}). > > > > > > > > Am I missing something? > > > > > > You're right, but setting the priority of an "uninitialized" object seems > > weird > > > (It's safe, but....but...). I wonder if we can just create the > > > ClientSocketHandle in ClientSocketHandle::DoInitConnectionImpl(), and not > call > > > SetPriority if ClientSocketHandle is nullptr. > > > > So I looked into this option in response to your comment, and it makes me > > nervous in the "there's something here I don't understand" way. The > > ClientSocketHandle::Reset() is called at various points in > > HttpStreamFactoryImpl::Job, I presume so that the state diagram can wrap back > up > > to before it was initialized (because why else would you bother). Those > Reset() > > calls would turn into frees (client_socket_handle_->Reset() ==> > > client_socket_handle.reset()), but that brings up the question of why Reset() > > exists at all--the delete and recreate idiom strikes me as more natural. It > may > > be for historical reasons or because ClientSocketHandle needs to be reset > > sometimes when it's a data member for a class (though why isn't that true here > > if so?), but it might also be because there's some hidden cost about > > deallocating and reallocating the object. > > Just destroying the socket seems to return it to the same state as a newly > created socket, so I can't see any reason why we can't just delete it. There > may once have been a reason for it, which no longer exists. It could even have > something to do with the old callback system, and/or the fact most net objects > were once refcounted. I think I may try the cleanup, and see what happens > (Mostly because it seems really simple just to change usage from the Job and see > what happens - if things look good, Reset() is only used in about a dozen files, > though those files do have a lot of references) > > > I looked at just forbidding setting the priority of an uninitialized > > ClientSocketHandle (ClientSocketHandle::Init() takes a priority and > > HttpStreamFactoryImpl::Job stores the priority, so I can just no-op the > > HSFI::J::SetPriority call if ClientSocketHandle hasn't been initialized yet), > > but the problem is that an error return of a ClientSocketHandle from the pools > > will put it into an uninitialized state, which makes me feel like I can't > forbid > > consumers from calling SetPriority in an uninitialized state. I could say > > something like "SetPriority() may not be called unless Init() has previously > > been called, has not completed in error, and Reset() has not been called" but > > that's a) a pretty complicated contract, and b) requires the consumer to track > > whether or not it's received the callback and the initialization has completed > > in error. > > Hrm... You're right that a ClientSocketHandle for a failed socket request > without a socket is indistinguishable from one that has yet to be initialized, > which seems rather unfortunate. Requiring a client not call a method on a > handle that's not in a connecting state (from its viewpoint) seems like a pretty > reasonable contract to me, the only problem I see with it is that we can't > enforce it. > > > So I think this is the least bad option? But I'm open to perspectives I > haven't > > come up with, or just plain argument :-}. > > From my standpoint, something with is_initialized == false is logically > equivalent to nullptr, so modifying it is an anti-pattern. Also, other than > this method, I think ClientSocketHandles, once connected (And even before, > actually) are basically immutable from the point of view of the consumer, other > than Reset, and method calls on the socket. Believe the result of this comment was tracked at the top level, and was "Eh, I guess the current situation is preferable to the large refactor involved in cleaning up the ClientSocketHandle API"; let me know if that's wrong. https://codereview.chromium.org/1898133002/diff/280001/net/socket/client_sock... net/socket/client_socket_handle.cc:45: pool_->SetPriority(group_name_, this, priority); On 2017/01/05 20:18:35, mmenke wrote: > On 2017/01/05 03:47:18, Randy Smith - Not in Mondays wrote: > > On 2017/01/03 21:28:22, mmenke wrote: > > > Also, this is calling SetPriority even after this ClientSocketHandle has a > > > socket, which doesn't seem right. A bit unfortunate that the > > ClientSocketHandle > > > both serves as a ClientSocketHandle and a SocketRequestHandle, but fixing > > that > > > is well beyond the scope of this CL. > > > > It's worse than doesn't seem right--it can produce an error if the pool has no > > knowledge of the request (as happens after the socket's been allocated). I > > fixed that in the most recent patchset, but by making SetPriority() a no-op if > > there's a socket. > > > > I'm inclined to think that that is the right choice, as I could conceivably > > imagine a meaning for SetPriority() on a ClientSocketHandle with a socket (it > > could affect TCP params). I have no intention of implementing something like > > that in the short term, but it makes me think the right solution is no-op'ing > in > > the implementation rather than forbidding the call in the API. Probably not > an > > issue for this CL, as you say, but WDYT of that argument? > > Let me try and get a better...handle... on the life[and]times of > ClientSocketHandles, and I'll get back to you on that. See last comment response :-}. https://codereview.chromium.org/1898133002/diff/280001/net/socket/client_sock... File net/socket/client_socket_pool_base_unittest.cc (right): https://codereview.chromium.org/1898133002/diff/280001/net/socket/client_sock... net/socket/client_socket_pool_base_unittest.cc:1113: request(1)->handle()->SetPriority(HIGHEST); On 2017/01/06 20:48:54, mmenke wrote: > This doesn't test anything, does it? request 2 has already completed, so > request 1 can't jump ahead of it without some sort of time machine, at this > point. > > We'd really need asynchronous sockets to get much out of this. Yes; I probably shouldn't have written this test and will delete it. Do you know of any way to make IGNORE_LIMITS connections complete asynchronously at this level? Without that, I don't see a way to test how IGNORE_LIMITS fits with priorities.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Response to comments. Not going to do a pass today, plan on one tomorrow, though I have a lot of meetings tomorrow. Figure you're not in any rush here, so treating this as lower priority than my other reviews, though certainly want to finish it this week. https://codereview.chromium.org/1898133002/diff/220001/net/socket/client_sock... File net/socket/client_socket_pool_base.cc (right): https://codereview.chromium.org/1898133002/diff/220001/net/socket/client_sock... net/socket/client_socket_pool_base.cc:1434: } On 2017/01/13 23:05:44, Randy Smith - Not in Mondays wrote: > On 2017/01/05 03:47:18, Randy Smith - Not in Mondays wrote: > > On 2017/01/03 22:51:22, mmenke wrote: > > > This entire block is just: > > > > > > std::unique_ptr<Request> request = FindAndRemovePendingRequest(); > > > request->set_priority(priority); > > > InsertPendingRequest(std::move(request)); > > > > > > (With some extra DCHECKs) > > > > > > As an added bonus, if we ever decide FindAndRemovePendingRequest is too > weird > > > and overhead-y as-is, this will benefit from the change as well (Or > > vice-versa). > > > > > > We lose the "pointer.priority() == priority" early exit, but that case > should > > > never happen, per the NOTREACHED below. > > > > The entire reason for this code is indeed the case where the priority is being > > set to the already existing priority, in response to yours and Charles' > comments > > about not wanting that case to have any effect (and my reluctance to forbid > that > > at this API level and implicitly rely on the short-circuit in > > URLRequest::SetPriority). If you object to the extra code for a case that > won't > > happen in the name of API cleanliness, I'll reluctantly fix it up (an API > > contract saying "This one specific value that you can't query isn't allowed" > > bothers me) , but I don't see what it has to do with the NOTREACHED() below? > > That's for whether or not the socket's in the group's list. Sorry, I was thinking the early return was a break. Anyhow, in the name of cleanliness, I'd rather we do one of the following: 1) Just disallow setting the priority to be the same. If the consumer is setting the API to be what it was before, they're wasting CPU cycles, and should be smarter. Worth noting that URLRequest already short circuits this case, so we're just talking about stuff outside net/ that directly uses socket pools. Which is uncommon (And which are currently second class citizens, and weird, as they use socket pooling without really wanting pooled sockets, anyways. So they're icky). 2) Catch it in ClientSocketHandle. This does require yet another bloaty field. It's not much, alone, but we have so much cruft all over the place, I'm still reluctant to add more unless we really needed it. 3) Just move the request to the back of the queue. https://codereview.chromium.org/1898133002/diff/280001/net/socket/client_sock... File net/socket/client_socket_handle.cc (right): https://codereview.chromium.org/1898133002/diff/280001/net/socket/client_sock... net/socket/client_socket_handle.cc:44: if (pool_) On 2017/01/13 23:05:44, Randy Smith - Not in Mondays wrote: > On 2017/01/05 20:18:35, mmenke wrote: > > On 2017/01/05 03:47:18, Randy Smith - Not in Mondays wrote: > > > On 2017/01/03 21:28:22, mmenke wrote: > > > > On 2017/01/03 21:09:08, Randy Smith - Not in Mondays wrote: > > > > > On 2016/05/09 18:41:03, mmenke (Out Dec 17 to Jan 2) wrote: > > > > > > When is this called without a pool? > > > > > > > > > > Well, as I read the code, HttpStreamFactoryImpl::Job allocates a > > > > > ClientSocketHandle for itself upon creation. That handle doesn't get > > passed > > > > > into the pools for initialization until DoInitConnection. So for all > the > > > > states > > > > > before that, the ClientSocketHandle doesn't have a pool. I'd think that > > > > > URLRequest::SetPriority() could be called anytime during that state > > machine, > > > > and > > > > > that that would propagate down to this level (and if that's not > currently > > > > > plumbed, it should be, but I think it is :-}). > > > > > > > > > > Am I missing something? > > > > > > > > You're right, but setting the priority of an "uninitialized" object seems > > > weird > > > > (It's safe, but....but...). I wonder if we can just create the > > > > ClientSocketHandle in ClientSocketHandle::DoInitConnectionImpl(), and not > > call > > > > SetPriority if ClientSocketHandle is nullptr. > > > > > > So I looked into this option in response to your comment, and it makes me > > > nervous in the "there's something here I don't understand" way. The > > > ClientSocketHandle::Reset() is called at various points in > > > HttpStreamFactoryImpl::Job, I presume so that the state diagram can wrap > back > > up > > > to before it was initialized (because why else would you bother). Those > > Reset() > > > calls would turn into frees (client_socket_handle_->Reset() ==> > > > client_socket_handle.reset()), but that brings up the question of why > Reset() > > > exists at all--the delete and recreate idiom strikes me as more natural. It > > may > > > be for historical reasons or because ClientSocketHandle needs to be reset > > > sometimes when it's a data member for a class (though why isn't that true > here > > > if so?), but it might also be because there's some hidden cost about > > > deallocating and reallocating the object. > > > > Just destroying the socket seems to return it to the same state as a newly > > created socket, so I can't see any reason why we can't just delete it. There > > may once have been a reason for it, which no longer exists. It could even > have > > something to do with the old callback system, and/or the fact most net objects > > were once refcounted. I think I may try the cleanup, and see what happens > > (Mostly because it seems really simple just to change usage from the Job and > see > > what happens - if things look good, Reset() is only used in about a dozen > files, > > though those files do have a lot of references) > > > > > I looked at just forbidding setting the priority of an uninitialized > > > ClientSocketHandle (ClientSocketHandle::Init() takes a priority and > > > HttpStreamFactoryImpl::Job stores the priority, so I can just no-op the > > > HSFI::J::SetPriority call if ClientSocketHandle hasn't been initialized > yet), > > > but the problem is that an error return of a ClientSocketHandle from the > pools > > > will put it into an uninitialized state, which makes me feel like I can't > > forbid > > > consumers from calling SetPriority in an uninitialized state. I could say > > > something like "SetPriority() may not be called unless Init() has previously > > > been called, has not completed in error, and Reset() has not been called" > but > > > that's a) a pretty complicated contract, and b) requires the consumer to > track > > > whether or not it's received the callback and the initialization has > completed > > > in error. > > > > Hrm... You're right that a ClientSocketHandle for a failed socket request > > without a socket is indistinguishable from one that has yet to be initialized, > > which seems rather unfortunate. Requiring a client not call a method on a > > handle that's not in a connecting state (from its viewpoint) seems like a > pretty > > reasonable contract to me, the only problem I see with it is that we can't > > enforce it. > > > > > So I think this is the least bad option? But I'm open to perspectives I > > haven't > > > come up with, or just plain argument :-}. > > > > From my standpoint, something with is_initialized == false is logically > > equivalent to nullptr, so modifying it is an anti-pattern. Also, other than > > this method, I think ClientSocketHandles, once connected (And even before, > > actually) are basically immutable from the point of view of the consumer, > other > > than Reset, and method calls on the socket. > > Believe the result of this comment was tracked at the top level, and was "Eh, I > guess the current situation is preferable to the large refactor involved in > cleaning up the ClientSocketHandle API"; let me know if that's wrong. That's right.
rdsmith@chromium.org changed reviewers: + csharrison@chromium.org
rdsmith@chromium.org changed required reviewers: + mmenke@chromium.org
Charles, could you weigh in on the below as well if you care? https://codereview.chromium.org/1898133002/diff/220001/net/socket/client_sock... File net/socket/client_socket_pool_base.cc (right): https://codereview.chromium.org/1898133002/diff/220001/net/socket/client_sock... net/socket/client_socket_pool_base.cc:1434: } On 2017/01/17 18:56:31, mmenke wrote: > On 2017/01/13 23:05:44, Randy Smith - Not in Mondays wrote: > > On 2017/01/05 03:47:18, Randy Smith - Not in Mondays wrote: > > > On 2017/01/03 22:51:22, mmenke wrote: > > > > This entire block is just: > > > > > > > > std::unique_ptr<Request> request = FindAndRemovePendingRequest(); > > > > request->set_priority(priority); > > > > InsertPendingRequest(std::move(request)); > > > > > > > > (With some extra DCHECKs) > > > > > > > > As an added bonus, if we ever decide FindAndRemovePendingRequest is too > > weird > > > > and overhead-y as-is, this will benefit from the change as well (Or > > > vice-versa). > > > > > > > > We lose the "pointer.priority() == priority" early exit, but that case > > should > > > > never happen, per the NOTREACHED below. > > > > > > The entire reason for this code is indeed the case where the priority is > being > > > set to the already existing priority, in response to yours and Charles' > > comments > > > about not wanting that case to have any effect (and my reluctance to forbid > > that > > > at this API level and implicitly rely on the short-circuit in > > > URLRequest::SetPriority). If you object to the extra code for a case that > > won't > > > happen in the name of API cleanliness, I'll reluctantly fix it up (an API > > > contract saying "This one specific value that you can't query isn't allowed" > > > bothers me) , but I don't see what it has to do with the NOTREACHED() below? > > > > That's for whether or not the socket's in the group's list. > > Sorry, I was thinking the early return was a break. > > Anyhow, in the name of cleanliness, I'd rather we do one of the following: > > 1) Just disallow setting the priority to be the same. If the consumer is > setting the API to be what it was before, they're wasting CPU cycles, and should > be smarter. Worth noting that URLRequest already short circuits this case, so > we're just talking about stuff outside net/ that directly uses socket pools. > Which is uncommon (And which are currently second class citizens, and weird, as > they use socket pooling without really wanting pooled sockets, anyways. So > they're icky). > > 2) Catch it in ClientSocketHandle. This does require yet another bloaty field. > It's not much, alone, but we have so much cruft all over the place, I'm still > reluctant to add more unless we really needed it. > > 3) Just move the request to the back of the queue. So I'm quite happy to do #3; I changed it to this because of your and Charles' opposition (see https://codereview.chromium.org/1898133002/diff/40001/net/socket/client_socke...; Charles weighed in offline, I believe). I'd rather avoid #2 for the same reason as you, and I'd rather avoid #1 because looked at in isolation is strikes me as an ugly API (the disallowed value is not probeable, so there's implicitly a tracking requirement on e consumer). Just confirming (with both you and Charles): Is #3 ok? It won't change the behavior for most requests because of the short-circuit at the URLRequest level, as you note (though there may come a time when I want that new behavior, but that'll be a different CL).
Charles, could you weigh in on the below as well if you care?
https://codereview.chromium.org/1898133002/diff/220001/net/socket/client_sock... File net/socket/client_socket_pool_base.cc (right): https://codereview.chromium.org/1898133002/diff/220001/net/socket/client_sock... net/socket/client_socket_pool_base.cc:1434: } On 2017/01/17 19:50:36, Randy Smith - Not in Mondays wrote: > On 2017/01/17 18:56:31, mmenke wrote: > > On 2017/01/13 23:05:44, Randy Smith - Not in Mondays wrote: > > > On 2017/01/05 03:47:18, Randy Smith - Not in Mondays wrote: > > > > On 2017/01/03 22:51:22, mmenke wrote: > > > > > This entire block is just: > > > > > > > > > > std::unique_ptr<Request> request = FindAndRemovePendingRequest(); > > > > > request->set_priority(priority); > > > > > InsertPendingRequest(std::move(request)); > > > > > > > > > > (With some extra DCHECKs) > > > > > > > > > > As an added bonus, if we ever decide FindAndRemovePendingRequest is too > > > weird > > > > > and overhead-y as-is, this will benefit from the change as well (Or > > > > vice-versa). > > > > > > > > > > We lose the "pointer.priority() == priority" early exit, but that case > > > should > > > > > never happen, per the NOTREACHED below. > > > > > > > > The entire reason for this code is indeed the case where the priority is > > being > > > > set to the already existing priority, in response to yours and Charles' > > > comments > > > > about not wanting that case to have any effect (and my reluctance to > forbid > > > that > > > > at this API level and implicitly rely on the short-circuit in > > > > URLRequest::SetPriority). If you object to the extra code for a case that > > > won't > > > > happen in the name of API cleanliness, I'll reluctantly fix it up (an API > > > > contract saying "This one specific value that you can't query isn't > allowed" > > > > bothers me) , but I don't see what it has to do with the NOTREACHED() > below? > > > > > > That's for whether or not the socket's in the group's list. > > > > Sorry, I was thinking the early return was a break. > > > > Anyhow, in the name of cleanliness, I'd rather we do one of the following: > > > > 1) Just disallow setting the priority to be the same. If the consumer is > > setting the API to be what it was before, they're wasting CPU cycles, and > should > > be smarter. Worth noting that URLRequest already short circuits this case, so > > we're just talking about stuff outside net/ that directly uses socket pools. > > Which is uncommon (And which are currently second class citizens, and weird, > as > > they use socket pooling without really wanting pooled sockets, anyways. So > > they're icky). > > > > 2) Catch it in ClientSocketHandle. This does require yet another bloaty > field. > > It's not much, alone, but we have so much cruft all over the place, I'm still > > reluctant to add more unless we really needed it. > > > > 3) Just move the request to the back of the queue. > > So I'm quite happy to do #3; I changed it to this because of your and Charles' > opposition (see > https://codereview.chromium.org/1898133002/diff/40001/net/socket/client_socke...; > Charles weighed in offline, I believe). I'd rather avoid #2 for the same reason > as you, and I'd rather avoid #1 because looked at in isolation is strikes me as > an ugly API (the disallowed value is not probeable, so there's implicitly a > tracking requirement on e consumer). Just confirming (with both you and > Charles): Is #3 ok? It won't change the behavior for most requests because of > the short-circuit at the URLRequest level, as you note (though there may come a > time when I want that new behavior, but that'll be a different CL). I am OK with #3 as long as it is documented clearly.
https://codereview.chromium.org/1898133002/diff/220001/net/socket/client_sock... File net/socket/client_socket_pool_base.cc (right): https://codereview.chromium.org/1898133002/diff/220001/net/socket/client_sock... net/socket/client_socket_pool_base.cc:1434: } On 2017/01/17 20:04:39, Charlie Harrison wrote: > On 2017/01/17 19:50:36, Randy Smith - Not in Mondays wrote: > > On 2017/01/17 18:56:31, mmenke wrote: > > > On 2017/01/13 23:05:44, Randy Smith - Not in Mondays wrote: > > > > On 2017/01/05 03:47:18, Randy Smith - Not in Mondays wrote: > > > > > On 2017/01/03 22:51:22, mmenke wrote: > > > > > > This entire block is just: > > > > > > > > > > > > std::unique_ptr<Request> request = FindAndRemovePendingRequest(); > > > > > > request->set_priority(priority); > > > > > > InsertPendingRequest(std::move(request)); > > > > > > > > > > > > (With some extra DCHECKs) > > > > > > > > > > > > As an added bonus, if we ever decide FindAndRemovePendingRequest is > too > > > > weird > > > > > > and overhead-y as-is, this will benefit from the change as well (Or > > > > > vice-versa). > > > > > > > > > > > > We lose the "pointer.priority() == priority" early exit, but that case > > > > should > > > > > > never happen, per the NOTREACHED below. > > > > > > > > > > The entire reason for this code is indeed the case where the priority is > > > being > > > > > set to the already existing priority, in response to yours and Charles' > > > > comments > > > > > about not wanting that case to have any effect (and my reluctance to > > forbid > > > > that > > > > > at this API level and implicitly rely on the short-circuit in > > > > > URLRequest::SetPriority). If you object to the extra code for a case > that > > > > won't > > > > > happen in the name of API cleanliness, I'll reluctantly fix it up (an > API > > > > > contract saying "This one specific value that you can't query isn't > > allowed" > > > > > bothers me) , but I don't see what it has to do with the NOTREACHED() > > below? > > > > > > > > That's for whether or not the socket's in the group's list. > > > > > > Sorry, I was thinking the early return was a break. > > > > > > Anyhow, in the name of cleanliness, I'd rather we do one of the following: > > > > > > 1) Just disallow setting the priority to be the same. If the consumer is > > > setting the API to be what it was before, they're wasting CPU cycles, and > > should > > > be smarter. Worth noting that URLRequest already short circuits this case, > so > > > we're just talking about stuff outside net/ that directly uses socket pools. > > > > Which is uncommon (And which are currently second class citizens, and weird, > > as > > > they use socket pooling without really wanting pooled sockets, anyways. So > > > they're icky). > > > > > > 2) Catch it in ClientSocketHandle. This does require yet another bloaty > > field. > > > It's not much, alone, but we have so much cruft all over the place, I'm > still > > > reluctant to add more unless we really needed it. > > > > > > 3) Just move the request to the back of the queue. > > > > So I'm quite happy to do #3; I changed it to this because of your and Charles' > > opposition (see > > > https://codereview.chromium.org/1898133002/diff/40001/net/socket/client_socke...; > > Charles weighed in offline, I believe). I'd rather avoid #2 for the same > reason > > as you, and I'd rather avoid #1 because looked at in isolation is strikes me > as > > an ugly API (the disallowed value is not probeable, so there's implicitly a > > tracking requirement on e consumer). Just confirming (with both you and > > Charles): Is #3 ok? It won't change the behavior for most requests because of > > the short-circuit at the URLRequest level, as you note (though there may come > a > > time when I want that new behavior, but that'll be a different CL). > > I am OK with #3 as long as it is documented clearly. Could also do a DLOG(WARNING), though if DLOG's are sufficiently discourages in favor of DVLOG, I don't think it's worth doing.
On 2017/01/17 20:11:29, mmenke wrote: > https://codereview.chromium.org/1898133002/diff/220001/net/socket/client_sock... > File net/socket/client_socket_pool_base.cc (right): > > https://codereview.chromium.org/1898133002/diff/220001/net/socket/client_sock... > net/socket/client_socket_pool_base.cc:1434: } > On 2017/01/17 20:04:39, Charlie Harrison wrote: > > On 2017/01/17 19:50:36, Randy Smith - Not in Mondays wrote: > > > On 2017/01/17 18:56:31, mmenke wrote: > > > > On 2017/01/13 23:05:44, Randy Smith - Not in Mondays wrote: > > > > > On 2017/01/05 03:47:18, Randy Smith - Not in Mondays wrote: > > > > > > On 2017/01/03 22:51:22, mmenke wrote: > > > > > > > This entire block is just: > > > > > > > > > > > > > > std::unique_ptr<Request> request = FindAndRemovePendingRequest(); > > > > > > > request->set_priority(priority); > > > > > > > InsertPendingRequest(std::move(request)); > > > > > > > > > > > > > > (With some extra DCHECKs) > > > > > > > > > > > > > > As an added bonus, if we ever decide FindAndRemovePendingRequest is > > too > > > > > weird > > > > > > > and overhead-y as-is, this will benefit from the change as well (Or > > > > > > vice-versa). > > > > > > > > > > > > > > We lose the "pointer.priority() == priority" early exit, but that > case > > > > > should > > > > > > > never happen, per the NOTREACHED below. > > > > > > > > > > > > The entire reason for this code is indeed the case where the priority > is > > > > being > > > > > > set to the already existing priority, in response to yours and > Charles' > > > > > comments > > > > > > about not wanting that case to have any effect (and my reluctance to > > > forbid > > > > > that > > > > > > at this API level and implicitly rely on the short-circuit in > > > > > > URLRequest::SetPriority). If you object to the extra code for a case > > that > > > > > won't > > > > > > happen in the name of API cleanliness, I'll reluctantly fix it up (an > > API > > > > > > contract saying "This one specific value that you can't query isn't > > > allowed" > > > > > > bothers me) , but I don't see what it has to do with the NOTREACHED() > > > below? > > > > > > > > > > That's for whether or not the socket's in the group's list. > > > > > > > > Sorry, I was thinking the early return was a break. > > > > > > > > Anyhow, in the name of cleanliness, I'd rather we do one of the following: > > > > > > > > 1) Just disallow setting the priority to be the same. If the consumer is > > > > setting the API to be what it was before, they're wasting CPU cycles, and > > > should > > > > be smarter. Worth noting that URLRequest already short circuits this > case, > > so > > > > we're just talking about stuff outside net/ that directly uses socket > pools. > > > > > > Which is uncommon (And which are currently second class citizens, and > weird, > > > as > > > > they use socket pooling without really wanting pooled sockets, anyways. > So > > > > they're icky). > > > > > > > > 2) Catch it in ClientSocketHandle. This does require yet another bloaty > > > field. > > > > It's not much, alone, but we have so much cruft all over the place, I'm > > still > > > > reluctant to add more unless we really needed it. > > > > > > > > 3) Just move the request to the back of the queue. > > > > > > So I'm quite happy to do #3; I changed it to this because of your and > Charles' > > > opposition (see > > > > > > https://codereview.chromium.org/1898133002/diff/40001/net/socket/client_socke...; > > > Charles weighed in offline, I believe). I'd rather avoid #2 for the same > > reason > > > as you, and I'd rather avoid #1 because looked at in isolation is strikes me > > as > > > an ugly API (the disallowed value is not probeable, so there's implicitly a > > > tracking requirement on e consumer). Just confirming (with both you and > > > Charles): Is #3 ok? It won't change the behavior for most requests because > of > > > the short-circuit at the URLRequest level, as you note (though there may > come > > a > > > time when I want that new behavior, but that'll be a different CL). > > > > I am OK with #3 as long as it is documented clearly. > > Could also do a DLOG(WARNING), though if DLOG's are sufficiently discourages in > favor of DVLOG, I don't think it's worth doing. It's hard to do a DLOG for the same reason it's hard to do a DCHECK: at the point in the code in question I don't know the current priority. Unless I'm misunderstanding your suggestion?
On 2017/01/17 20:14:51, Randy Smith - Not in Mondays wrote: > On 2017/01/17 20:11:29, mmenke wrote: > > > https://codereview.chromium.org/1898133002/diff/220001/net/socket/client_sock... > > File net/socket/client_socket_pool_base.cc (right): > > > > > https://codereview.chromium.org/1898133002/diff/220001/net/socket/client_sock... > > net/socket/client_socket_pool_base.cc:1434: } > > On 2017/01/17 20:04:39, Charlie Harrison wrote: > > > On 2017/01/17 19:50:36, Randy Smith - Not in Mondays wrote: > > > > On 2017/01/17 18:56:31, mmenke wrote: > > > > > On 2017/01/13 23:05:44, Randy Smith - Not in Mondays wrote: > > > > > > On 2017/01/05 03:47:18, Randy Smith - Not in Mondays wrote: > > > > > > > On 2017/01/03 22:51:22, mmenke wrote: > > > > > > > > This entire block is just: > > > > > > > > > > > > > > > > std::unique_ptr<Request> request = FindAndRemovePendingRequest(); > > > > > > > > request->set_priority(priority); > > > > > > > > InsertPendingRequest(std::move(request)); > > > > > > > > > > > > > > > > (With some extra DCHECKs) > > > > > > > > > > > > > > > > As an added bonus, if we ever decide FindAndRemovePendingRequest > is > > > too > > > > > > weird > > > > > > > > and overhead-y as-is, this will benefit from the change as well > (Or > > > > > > > vice-versa). > > > > > > > > > > > > > > > > We lose the "pointer.priority() == priority" early exit, but that > > case > > > > > > should > > > > > > > > never happen, per the NOTREACHED below. > > > > > > > > > > > > > > The entire reason for this code is indeed the case where the > priority > > is > > > > > being > > > > > > > set to the already existing priority, in response to yours and > > Charles' > > > > > > comments > > > > > > > about not wanting that case to have any effect (and my reluctance to > > > > forbid > > > > > > that > > > > > > > at this API level and implicitly rely on the short-circuit in > > > > > > > URLRequest::SetPriority). If you object to the extra code for a > case > > > that > > > > > > won't > > > > > > > happen in the name of API cleanliness, I'll reluctantly fix it up > (an > > > API > > > > > > > contract saying "This one specific value that you can't query isn't > > > > allowed" > > > > > > > bothers me) , but I don't see what it has to do with the > NOTREACHED() > > > > below? > > > > > > > > > > > > That's for whether or not the socket's in the group's list. > > > > > > > > > > Sorry, I was thinking the early return was a break. > > > > > > > > > > Anyhow, in the name of cleanliness, I'd rather we do one of the > following: > > > > > > > > > > 1) Just disallow setting the priority to be the same. If the consumer > is > > > > > setting the API to be what it was before, they're wasting CPU cycles, > and > > > > should > > > > > be smarter. Worth noting that URLRequest already short circuits this > > case, > > > so > > > > > we're just talking about stuff outside net/ that directly uses socket > > pools. > > > > > > > > Which is uncommon (And which are currently second class citizens, and > > weird, > > > > as > > > > > they use socket pooling without really wanting pooled sockets, anyways. > > So > > > > > they're icky). > > > > > > > > > > 2) Catch it in ClientSocketHandle. This does require yet another bloaty > > > > field. > > > > > It's not much, alone, but we have so much cruft all over the place, I'm > > > still > > > > > reluctant to add more unless we really needed it. > > > > > > > > > > 3) Just move the request to the back of the queue. > > > > > > > > So I'm quite happy to do #3; I changed it to this because of your and > > Charles' > > > > opposition (see > > > > > > > > > > https://codereview.chromium.org/1898133002/diff/40001/net/socket/client_socke...; > > > > Charles weighed in offline, I believe). I'd rather avoid #2 for the same > > > reason > > > > as you, and I'd rather avoid #1 because looked at in isolation is strikes > me > > > as > > > > an ugly API (the disallowed value is not probeable, so there's implicitly > a > > > > tracking requirement on e consumer). Just confirming (with both you and > > > > Charles): Is #3 ok? It won't change the behavior for most requests > because > > of > > > > the short-circuit at the URLRequest level, as you note (though there may > > come > > > a > > > > time when I want that new behavior, but that'll be a different CL). > > > > > > I am OK with #3 as long as it is documented clearly. > > > > Could also do a DLOG(WARNING), though if DLOG's are sufficiently discourages > in > > favor of DVLOG, I don't think it's worth doing. > > It's hard to do a DLOG for the same reason it's hard to do a DCHECK: at the > point in the code in question I don't know the current priority. Unless I'm > misunderstanding your suggestion? You do know the priority once you've found it in the socket pool, though. So it's (int pesudo-code): std::unique_ptr<RequestThingy> request = group_thingy->RemoveRequestThingy(); if (request->priority() == new_priority) DLOG(...) << ...; request->set_priority(new_priority); group_thingy->AddRequest(std::move(request));
On 2017/01/17 20:19:30, mmenke wrote: > On 2017/01/17 20:14:51, Randy Smith - Not in Mondays wrote: > > On 2017/01/17 20:11:29, mmenke wrote: > > > > > > https://codereview.chromium.org/1898133002/diff/220001/net/socket/client_sock... > > > File net/socket/client_socket_pool_base.cc (right): > > > > > > > > > https://codereview.chromium.org/1898133002/diff/220001/net/socket/client_sock... > > > net/socket/client_socket_pool_base.cc:1434: } > > > On 2017/01/17 20:04:39, Charlie Harrison wrote: > > > > On 2017/01/17 19:50:36, Randy Smith - Not in Mondays wrote: > > > > > On 2017/01/17 18:56:31, mmenke wrote: > > > > > > On 2017/01/13 23:05:44, Randy Smith - Not in Mondays wrote: > > > > > > > On 2017/01/05 03:47:18, Randy Smith - Not in Mondays wrote: > > > > > > > > On 2017/01/03 22:51:22, mmenke wrote: > > > > > > > > > This entire block is just: > > > > > > > > > > > > > > > > > > std::unique_ptr<Request> request = > FindAndRemovePendingRequest(); > > > > > > > > > request->set_priority(priority); > > > > > > > > > InsertPendingRequest(std::move(request)); > > > > > > > > > > > > > > > > > > (With some extra DCHECKs) > > > > > > > > > > > > > > > > > > As an added bonus, if we ever decide FindAndRemovePendingRequest > > is > > > > too > > > > > > > weird > > > > > > > > > and overhead-y as-is, this will benefit from the change as well > > (Or > > > > > > > > vice-versa). > > > > > > > > > > > > > > > > > > We lose the "pointer.priority() == priority" early exit, but > that > > > case > > > > > > > should > > > > > > > > > never happen, per the NOTREACHED below. > > > > > > > > > > > > > > > > The entire reason for this code is indeed the case where the > > priority > > > is > > > > > > being > > > > > > > > set to the already existing priority, in response to yours and > > > Charles' > > > > > > > comments > > > > > > > > about not wanting that case to have any effect (and my reluctance > to > > > > > forbid > > > > > > > that > > > > > > > > at this API level and implicitly rely on the short-circuit in > > > > > > > > URLRequest::SetPriority). If you object to the extra code for a > > case > > > > that > > > > > > > won't > > > > > > > > happen in the name of API cleanliness, I'll reluctantly fix it up > > (an > > > > API > > > > > > > > contract saying "This one specific value that you can't query > isn't > > > > > allowed" > > > > > > > > bothers me) , but I don't see what it has to do with the > > NOTREACHED() > > > > > below? > > > > > > > > > > > > > > That's for whether or not the socket's in the group's list. > > > > > > > > > > > > Sorry, I was thinking the early return was a break. > > > > > > > > > > > > Anyhow, in the name of cleanliness, I'd rather we do one of the > > following: > > > > > > > > > > > > 1) Just disallow setting the priority to be the same. If the consumer > > is > > > > > > setting the API to be what it was before, they're wasting CPU cycles, > > and > > > > > should > > > > > > be smarter. Worth noting that URLRequest already short circuits this > > > case, > > > > so > > > > > > we're just talking about stuff outside net/ that directly uses socket > > > pools. > > > > > > > > > > Which is uncommon (And which are currently second class citizens, and > > > weird, > > > > > as > > > > > > they use socket pooling without really wanting pooled sockets, > anyways. > > > So > > > > > > they're icky). > > > > > > > > > > > > 2) Catch it in ClientSocketHandle. This does require yet another > bloaty > > > > > field. > > > > > > It's not much, alone, but we have so much cruft all over the place, > I'm > > > > still > > > > > > reluctant to add more unless we really needed it. > > > > > > > > > > > > 3) Just move the request to the back of the queue. > > > > > > > > > > So I'm quite happy to do #3; I changed it to this because of your and > > > Charles' > > > > > opposition (see > > > > > > > > > > > > > > > https://codereview.chromium.org/1898133002/diff/40001/net/socket/client_socke...; > > > > > Charles weighed in offline, I believe). I'd rather avoid #2 for the > same > > > > reason > > > > > as you, and I'd rather avoid #1 because looked at in isolation is > strikes > > me > > > > as > > > > > an ugly API (the disallowed value is not probeable, so there's > implicitly > > a > > > > > tracking requirement on e consumer). Just confirming (with both you > and > > > > > Charles): Is #3 ok? It won't change the behavior for most requests > > because > > > of > > > > > the short-circuit at the URLRequest level, as you note (though there may > > > come > > > > a > > > > > time when I want that new behavior, but that'll be a different CL). > > > > > > > > I am OK with #3 as long as it is documented clearly. > > > > > > Could also do a DLOG(WARNING), though if DLOG's are sufficiently discourages > > in > > > favor of DVLOG, I don't think it's worth doing. > > > > It's hard to do a DLOG for the same reason it's hard to do a DCHECK: at the > > point in the code in question I don't know the current priority. Unless I'm > > misunderstanding your suggestion? > > You do know the priority once you've found it in the socket pool, though. > > So it's (int pesudo-code): > > std::unique_ptr<RequestThingy> request = group_thingy->RemoveRequestThingy(); > if (request->priority() == new_priority) > DLOG(...) << ...; > request->set_priority(new_priority); > group_thingy->AddRequest(std::move(request)); I guess that doesn't cover all cases (Namely, when we do nothing in response to the SetPriority call), but seems better than nothing?
On 2017/01/17 20:21:32, mmenke wrote: > On 2017/01/17 20:19:30, mmenke wrote: > > On 2017/01/17 20:14:51, Randy Smith - Not in Mondays wrote: > > > On 2017/01/17 20:11:29, mmenke wrote: > > > > > > > > > > https://codereview.chromium.org/1898133002/diff/220001/net/socket/client_sock... > > > > File net/socket/client_socket_pool_base.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1898133002/diff/220001/net/socket/client_sock... > > > > net/socket/client_socket_pool_base.cc:1434: } > > > > On 2017/01/17 20:04:39, Charlie Harrison wrote: > > > > > On 2017/01/17 19:50:36, Randy Smith - Not in Mondays wrote: > > > > > > On 2017/01/17 18:56:31, mmenke wrote: > > > > > > > On 2017/01/13 23:05:44, Randy Smith - Not in Mondays wrote: > > > > > > > > On 2017/01/05 03:47:18, Randy Smith - Not in Mondays wrote: > > > > > > > > > On 2017/01/03 22:51:22, mmenke wrote: > > > > > > > > > > This entire block is just: > > > > > > > > > > > > > > > > > > > > std::unique_ptr<Request> request = > > FindAndRemovePendingRequest(); > > > > > > > > > > request->set_priority(priority); > > > > > > > > > > InsertPendingRequest(std::move(request)); > > > > > > > > > > > > > > > > > > > > (With some extra DCHECKs) > > > > > > > > > > > > > > > > > > > > As an added bonus, if we ever decide > FindAndRemovePendingRequest > > > is > > > > > too > > > > > > > > weird > > > > > > > > > > and overhead-y as-is, this will benefit from the change as > well > > > (Or > > > > > > > > > vice-versa). > > > > > > > > > > > > > > > > > > > > We lose the "pointer.priority() == priority" early exit, but > > that > > > > case > > > > > > > > should > > > > > > > > > > never happen, per the NOTREACHED below. > > > > > > > > > > > > > > > > > > The entire reason for this code is indeed the case where the > > > priority > > > > is > > > > > > > being > > > > > > > > > set to the already existing priority, in response to yours and > > > > Charles' > > > > > > > > comments > > > > > > > > > about not wanting that case to have any effect (and my > reluctance > > to > > > > > > forbid > > > > > > > > that > > > > > > > > > at this API level and implicitly rely on the short-circuit in > > > > > > > > > URLRequest::SetPriority). If you object to the extra code for a > > > case > > > > > that > > > > > > > > won't > > > > > > > > > happen in the name of API cleanliness, I'll reluctantly fix it > up > > > (an > > > > > API > > > > > > > > > contract saying "This one specific value that you can't query > > isn't > > > > > > allowed" > > > > > > > > > bothers me) , but I don't see what it has to do with the > > > NOTREACHED() > > > > > > below? > > > > > > > > > > > > > > > > That's for whether or not the socket's in the group's list. > > > > > > > > > > > > > > Sorry, I was thinking the early return was a break. > > > > > > > > > > > > > > Anyhow, in the name of cleanliness, I'd rather we do one of the > > > following: > > > > > > > > > > > > > > 1) Just disallow setting the priority to be the same. If the > consumer > > > is > > > > > > > setting the API to be what it was before, they're wasting CPU > cycles, > > > and > > > > > > should > > > > > > > be smarter. Worth noting that URLRequest already short circuits > this > > > > case, > > > > > so > > > > > > > we're just talking about stuff outside net/ that directly uses > socket > > > > pools. > > > > > > > > > > > > Which is uncommon (And which are currently second class citizens, > and > > > > weird, > > > > > > as > > > > > > > they use socket pooling without really wanting pooled sockets, > > anyways. > > > > So > > > > > > > they're icky). > > > > > > > > > > > > > > 2) Catch it in ClientSocketHandle. This does require yet another > > bloaty > > > > > > field. > > > > > > > It's not much, alone, but we have so much cruft all over the place, > > I'm > > > > > still > > > > > > > reluctant to add more unless we really needed it. > > > > > > > > > > > > > > 3) Just move the request to the back of the queue. > > > > > > > > > > > > So I'm quite happy to do #3; I changed it to this because of your and > > > > Charles' > > > > > > opposition (see > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1898133002/diff/40001/net/socket/client_socke...; > > > > > > Charles weighed in offline, I believe). I'd rather avoid #2 for the > > same > > > > > reason > > > > > > as you, and I'd rather avoid #1 because looked at in isolation is > > strikes > > > me > > > > > as > > > > > > an ugly API (the disallowed value is not probeable, so there's > > implicitly > > > a > > > > > > tracking requirement on e consumer). Just confirming (with both you > > and > > > > > > Charles): Is #3 ok? It won't change the behavior for most requests > > > because > > > > of > > > > > > the short-circuit at the URLRequest level, as you note (though there > may > > > > come > > > > > a > > > > > > time when I want that new behavior, but that'll be a different CL). > > > > > > > > > > I am OK with #3 as long as it is documented clearly. > > > > > > > > Could also do a DLOG(WARNING), though if DLOG's are sufficiently > discourages > > > in > > > > favor of DVLOG, I don't think it's worth doing. > > > > > > It's hard to do a DLOG for the same reason it's hard to do a DCHECK: at the > > > point in the code in question I don't know the current priority. Unless I'm > > > misunderstanding your suggestion? > > > > You do know the priority once you've found it in the socket pool, though. > > > > So it's (int pesudo-code): > > > > std::unique_ptr<RequestThingy> request = group_thingy->RemoveRequestThingy(); > > if (request->priority() == new_priority) > > DLOG(...) << ...; > > request->set_priority(new_priority); > > group_thingy->AddRequest(std::move(request)); > > I guess that doesn't cover all cases (Namely, when we do nothing in response to > the SetPriority call), but seems better than nothing? Ah, sorry, quite right. I'd still prefer not to--I'd either like to DCHECK() (which to me means specifying a somewhat ugly API) or spec it as allowed (which fits with my long term belief about where the high level API should go, for all that it doesn't actually fit with what the high level API currently does or is specd to do). Unless you object, I'm just going to go with specing the behavior of it moving to the end of the queue. (Since we've had so much exchange on this topic I'll call out that I still intend to wait for your full review before doing another PS.)
On 2017/01/17 21:08:14, Randy Smith - Not in Mondays wrote: > On 2017/01/17 20:21:32, mmenke wrote: > > On 2017/01/17 20:19:30, mmenke wrote: > > > On 2017/01/17 20:14:51, Randy Smith - Not in Mondays wrote: > > > > On 2017/01/17 20:11:29, mmenke wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1898133002/diff/220001/net/socket/client_sock... > > > > > File net/socket/client_socket_pool_base.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1898133002/diff/220001/net/socket/client_sock... > > > > > net/socket/client_socket_pool_base.cc:1434: } > > > > > On 2017/01/17 20:04:39, Charlie Harrison wrote: > > > > > > On 2017/01/17 19:50:36, Randy Smith - Not in Mondays wrote: > > > > > > > On 2017/01/17 18:56:31, mmenke wrote: > > > > > > > > On 2017/01/13 23:05:44, Randy Smith - Not in Mondays wrote: > > > > > > > > > On 2017/01/05 03:47:18, Randy Smith - Not in Mondays wrote: > > > > > > > > > > On 2017/01/03 22:51:22, mmenke wrote: > > > > > > > > > > > This entire block is just: > > > > > > > > > > > > > > > > > > > > > > std::unique_ptr<Request> request = > > > FindAndRemovePendingRequest(); > > > > > > > > > > > request->set_priority(priority); > > > > > > > > > > > InsertPendingRequest(std::move(request)); > > > > > > > > > > > > > > > > > > > > > > (With some extra DCHECKs) > > > > > > > > > > > > > > > > > > > > > > As an added bonus, if we ever decide > > FindAndRemovePendingRequest > > > > is > > > > > > too > > > > > > > > > weird > > > > > > > > > > > and overhead-y as-is, this will benefit from the change as > > well > > > > (Or > > > > > > > > > > vice-versa). > > > > > > > > > > > > > > > > > > > > > > We lose the "pointer.priority() == priority" early exit, but > > > that > > > > > case > > > > > > > > > should > > > > > > > > > > > never happen, per the NOTREACHED below. > > > > > > > > > > > > > > > > > > > > The entire reason for this code is indeed the case where the > > > > priority > > > > > is > > > > > > > > being > > > > > > > > > > set to the already existing priority, in response to yours and > > > > > Charles' > > > > > > > > > comments > > > > > > > > > > about not wanting that case to have any effect (and my > > reluctance > > > to > > > > > > > forbid > > > > > > > > > that > > > > > > > > > > at this API level and implicitly rely on the short-circuit in > > > > > > > > > > URLRequest::SetPriority). If you object to the extra code for > a > > > > case > > > > > > that > > > > > > > > > won't > > > > > > > > > > happen in the name of API cleanliness, I'll reluctantly fix it > > up > > > > (an > > > > > > API > > > > > > > > > > contract saying "This one specific value that you can't query > > > isn't > > > > > > > allowed" > > > > > > > > > > bothers me) , but I don't see what it has to do with the > > > > NOTREACHED() > > > > > > > below? > > > > > > > > > > > > > > > > > > That's for whether or not the socket's in the group's list. > > > > > > > > > > > > > > > > Sorry, I was thinking the early return was a break. > > > > > > > > > > > > > > > > Anyhow, in the name of cleanliness, I'd rather we do one of the > > > > following: > > > > > > > > > > > > > > > > 1) Just disallow setting the priority to be the same. If the > > consumer > > > > is > > > > > > > > setting the API to be what it was before, they're wasting CPU > > cycles, > > > > and > > > > > > > should > > > > > > > > be smarter. Worth noting that URLRequest already short circuits > > this > > > > > case, > > > > > > so > > > > > > > > we're just talking about stuff outside net/ that directly uses > > socket > > > > > pools. > > > > > > > > > > > > > > Which is uncommon (And which are currently second class citizens, > > and > > > > > weird, > > > > > > > as > > > > > > > > they use socket pooling without really wanting pooled sockets, > > > anyways. > > > > > So > > > > > > > > they're icky). > > > > > > > > > > > > > > > > 2) Catch it in ClientSocketHandle. This does require yet another > > > bloaty > > > > > > > field. > > > > > > > > It's not much, alone, but we have so much cruft all over the > place, > > > I'm > > > > > > still > > > > > > > > reluctant to add more unless we really needed it. > > > > > > > > > > > > > > > > 3) Just move the request to the back of the queue. > > > > > > > > > > > > > > So I'm quite happy to do #3; I changed it to this because of your > and > > > > > Charles' > > > > > > > opposition (see > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1898133002/diff/40001/net/socket/client_socke...; > > > > > > > Charles weighed in offline, I believe). I'd rather avoid #2 for the > > > same > > > > > > reason > > > > > > > as you, and I'd rather avoid #1 because looked at in isolation is > > > strikes > > > > me > > > > > > as > > > > > > > an ugly API (the disallowed value is not probeable, so there's > > > implicitly > > > > a > > > > > > > tracking requirement on e consumer). Just confirming (with both > you > > > and > > > > > > > Charles): Is #3 ok? It won't change the behavior for most requests > > > > because > > > > > of > > > > > > > the short-circuit at the URLRequest level, as you note (though there > > may > > > > > come > > > > > > a > > > > > > > time when I want that new behavior, but that'll be a different CL). > > > > > > > > > > > > I am OK with #3 as long as it is documented clearly. > > > > > > > > > > Could also do a DLOG(WARNING), though if DLOG's are sufficiently > > discourages > > > > in > > > > > favor of DVLOG, I don't think it's worth doing. > > > > > > > > It's hard to do a DLOG for the same reason it's hard to do a DCHECK: at > the > > > > point in the code in question I don't know the current priority. Unless > I'm > > > > misunderstanding your suggestion? > > > > > > You do know the priority once you've found it in the socket pool, though. > > > > > > So it's (int pesudo-code): > > > > > > std::unique_ptr<RequestThingy> request = > group_thingy->RemoveRequestThingy(); > > > if (request->priority() == new_priority) > > > DLOG(...) << ...; > > > request->set_priority(new_priority); > > > group_thingy->AddRequest(std::move(request)); > > > > I guess that doesn't cover all cases (Namely, when we do nothing in response > to > > the SetPriority call), but seems better than nothing? > > Ah, sorry, quite right. > > I'd still prefer not to--I'd either like to DCHECK() (which to me means > specifying a somewhat ugly API) or spec it as allowed (which fits with my long > term belief about where the high level API should go, for all that it doesn't > actually fit with what the high level API currently does or is specd to do). > Unless you object, I'm just going to go with specing the behavior of it moving > to the end of the queue. > > (Since we've had so much exchange on this topic I'll call out that I still > intend to wait for your full review before doing another PS.) Thanks for the reminder, I'm pretty sure I would have forgotten.
https://codereview.chromium.org/1898133002/diff/300001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1898133002/diff/300001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:324: // is null. suggest a blank comment between both comments. https://codereview.chromium.org/1898133002/diff/300001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:1189: LOG(ERROR) << __FUNCTION__ << ":" << __LINE__ << ": Stream set"; Remove comment? DLOG? https://codereview.chromium.org/1898133002/diff/300001/net/http/http_stream_f... File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/1898133002/diff/300001/net/http/http_stream_f... net/http/http_stream_factory_impl_unittest.cc:1546: // does not crash. Hrm...Is there some way we can verify the stream was created? Query the SPDY session or something? Grab the SpdySession and call num_active_streams()? https://codereview.chromium.org/1898133002/diff/300001/net/http/http_stream_f... net/http/http_stream_factory_impl_unittest.cc:1551: ASSERT_TRUE(nullptr != waiter.stream()); I believe you can ASSERT_TRUE(waiter.stream()); now. https://codereview.chromium.org/1898133002/diff/300001/net/http/http_stream_f... net/http/http_stream_factory_impl_unittest.cc:1552: EXPECT_TRUE(nullptr == waiter.websocket_stream()); Same as above (Except EXPECT_FALSE) https://codereview.chromium.org/1898133002/diff/300001/net/socket/client_sock... File net/socket/client_socket_pool_base.cc (right): https://codereview.chromium.org/1898133002/diff/300001/net/socket/client_sock... net/socket/client_socket_pool_base.cc:1406: if (pointer.value()->handle() == handle) { DCHECK_EQ(pointer.priority, pointer.value()->priority())? (Since we can now change priority of request, nice to have a double-check it's in the right place in the RequestQueue) https://codereview.chromium.org/1898133002/diff/300001/net/socket/client_sock... File net/socket/client_socket_pool_base_unittest.cc (right): https://codereview.chromium.org/1898133002/diff/300001/net/socket/client_sock... net/socket/client_socket_pool_base_unittest.cc:1049: request(2)->handle()->SetPriority(LOWEST); Suggest raising this to MEDIUM, to match the priority of the second one, and make sure order doesn't change. https://codereview.chromium.org/1898133002/diff/300001/net/socket/websocket_t... File net/socket/websocket_transport_client_socket_pool.cc (right): https://codereview.chromium.org/1898133002/diff/300001/net/socket/websocket_t... net/socket/websocket_transport_client_socket_pool.cc:402: // connect job. What if it's in stalled_request_map_?
The CQ bit was checked by rdsmith@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...
Matt, PTAL?
Adam, would you comment on the value of having stalled_request_{queue,map}_ in
websockets_transport_client_socket_pool take priorities into account?
https://codereview.chromium.org/1898133002/diff/300001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_job.cc (right):
https://codereview.chromium.org/1898133002/diff/300001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job.cc:324: // is null.
On 2017/01/18 20:37:26, mmenke wrote:
> suggest a blank comment between both comments.
Done.
https://codereview.chromium.org/1898133002/diff/300001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job.cc:1189: LOG(ERROR) << __FUNCTION__ << ":"
<< __LINE__ << ": Stream set";
On 2017/01/18 20:37:26, mmenke wrote:
> Remove comment? DLOG?
Ooops. Done.
https://codereview.chromium.org/1898133002/diff/300001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_unittest.cc (right):
https://codereview.chromium.org/1898133002/diff/300001/net/http/http_stream_f...
net/http/http_stream_factory_impl_unittest.cc:1546: // does not crash.
On 2017/01/18 20:37:26, mmenke wrote:
> Hrm...Is there some way we can verify the stream was created? Query the SPDY
> session or something? Grab the SpdySession and call num_active_streams()?
I've modified the test to confirm that a session has been created synchronously
through the RequestStream(). I tried to test num_active_streams() and couldn't
get it to go non-zero; as best I can tell the SpdySession only creates a stream
on first write, and I don't think there's any way to arrange for a write before
the PostTask/upcall returning the stream occurs. Let me know if you don't think
that's enough and/or if you see some way of testing to make sure a stream has
actually been created.
https://codereview.chromium.org/1898133002/diff/300001/net/http/http_stream_f...
net/http/http_stream_factory_impl_unittest.cc:1551: ASSERT_TRUE(nullptr !=
waiter.stream());
On 2017/01/18 20:37:26, mmenke wrote:
> I believe you can ASSERT_TRUE(waiter.stream()); now.
Done.
https://codereview.chromium.org/1898133002/diff/300001/net/http/http_stream_f...
net/http/http_stream_factory_impl_unittest.cc:1552: EXPECT_TRUE(nullptr ==
waiter.websocket_stream());
On 2017/01/18 20:37:27, mmenke wrote:
> Same as above (Except EXPECT_FALSE)
Done.
https://codereview.chromium.org/1898133002/diff/300001/net/socket/client_sock...
File net/socket/client_socket_pool_base.cc (right):
https://codereview.chromium.org/1898133002/diff/300001/net/socket/client_sock...
net/socket/client_socket_pool_base.cc:1406: if (pointer.value()->handle() ==
handle) {
On 2017/01/18 20:37:27, mmenke wrote:
> DCHECK_EQ(pointer.priority, pointer.value()->priority())? (Since we can now
> change priority of request, nice to have a double-check it's in the right
place
> in the RequestQueue)
Done.
https://codereview.chromium.org/1898133002/diff/300001/net/socket/client_sock...
File net/socket/client_socket_pool_base_unittest.cc (right):
https://codereview.chromium.org/1898133002/diff/300001/net/socket/client_sock...
net/socket/client_socket_pool_base_unittest.cc:1049:
request(2)->handle()->SetPriority(LOWEST);
On 2017/01/18 20:37:27, mmenke wrote:
> Suggest raising this to MEDIUM, to match the priority of the second one, and
> make sure order doesn't change.
Done.
https://codereview.chromium.org/1898133002/diff/300001/net/socket/websocket_t...
File net/socket/websocket_transport_client_socket_pool.cc (right):
https://codereview.chromium.org/1898133002/diff/300001/net/socket/websocket_t...
net/socket/websocket_transport_client_socket_pool.cc:402: // connect job.
On 2017/01/18 20:37:27, mmenke wrote:
> What if it's in stalled_request_map_?
Hmmm. Good point, I missed that.
However, stalled_request_map_ is blissfully priority unaware, even of the
priority that request carries when it comes in, AFAICT. So making SetPriority()
have any effect would almost force stalled_request_map_ over to being a priority
queue, which would be a functional change to WebSockets, which I'm a bit
reluctant to do?
Adam, was this intentional? Are you positive, neutral, or negative about
changing stalled_request_map_ to know about priorities? (Not promising to do it
if you're positive, just trying to understand the lay of the land.)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by rdsmith@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ricea@chromium.org changed reviewers: + ricea@chromium.org
https://codereview.chromium.org/1898133002/diff/300001/net/socket/websocket_t... File net/socket/websocket_transport_client_socket_pool.cc (right): https://codereview.chromium.org/1898133002/diff/300001/net/socket/websocket_t... net/socket/websocket_transport_client_socket_pool.cc:402: // connect job. On 2017/01/22 21:37:00, Randy Smith (OOO 1-21 - 2-10) wrote: > On 2017/01/18 20:37:27, mmenke wrote: > > What if it's in stalled_request_map_? > > Hmmm. Good point, I missed that. > > However, stalled_request_map_ is blissfully priority unaware, even of the > priority that request carries when it comes in, AFAICT. So making SetPriority() > have any effect would almost force stalled_request_map_ over to being a priority > queue, which would be a functional change to WebSockets, which I'm a bit > reluctant to do? > > Adam, was this intentional? Are you positive, neutral, or negative about > changing stalled_request_map_ to know about priorities? (Not promising to do it > if you're positive, just trying to understand the lay of the land.) All WebSocket handshakes happen at DEFAULT_PRIORITY, so supporting SetPriority correctly for WebSocket would be pointless. Unless it makes the code simpler, ignoring priorities is the best option.
LGTM!
Oops, forgot to publish this yesterday (Though as you're out, may not matter). LGTM. https://codereview.chromium.org/1898133002/diff/340001/net/http/http_stream_f... File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/1898133002/diff/340001/net/http/http_stream_f... net/http/http_stream_factory_impl_unittest.cc:1523: MockRead mock_read(SYNCHRONOUS, OK); OK -> ERR_IO_PENDING? I don't think we want to read an EOF from the socket, right?
Thanks! Wow, landing a CL after 10 months. I don't even think that's a record. https://codereview.chromium.org/1898133002/diff/340001/net/http/http_stream_f... File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/1898133002/diff/340001/net/http/http_stream_f... net/http/http_stream_factory_impl_unittest.cc:1523: MockRead mock_read(SYNCHRONOUS, OK); On 2017/01/24 17:30:34, mmenke (Out Feb 4 to March 5) wrote: > OK -> ERR_IO_PENDING? I don't think we want to read an EOF from the socket, > right? Oh, that makes some sense of some of my debugging confusion. Done.
The CQ bit was checked by rdsmith@chromium.org
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/1898133002/#ps380001 (title: "Incorporated comments.")
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": 380001, "attempt_start_ts": 1487287349505150,
"parent_rev": "e428ea9a94c51f8f16af35dac2d03ec97793a100", "commit_rev":
"29dbad12b55c8d2d520b26987735354c25b9590c"}
Message was sent while issue was closed.
Description was changed from ========== Add reprioritization to socket pools. BUG=600839 BUG=166689 ========== to ========== Add reprioritization to socket pools. BUG=600839 BUG=166689 Review-Url: https://codereview.chromium.org/1898133002 Cr-Commit-Position: refs/heads/master@{#451185} Committed: https://chromium.googlesource.com/chromium/src/+/29dbad12b55c8d2d520b26987735... ==========
Message was sent while issue was closed.
Committed patchset #19 (id:380001) as https://chromium.googlesource.com/chromium/src/+/29dbad12b55c8d2d520b26987735... |
