Chromium Code Reviews
|
|
Chromium Code Reviews
Issue 1898133002:
Add reprioritization to socket pools. (Closed)
Base URL: https://chromium.googlesource.com/chromium/src.git@master| Index: net/socket/client_socket_handle.cc |
| diff --git a/net/socket/client_socket_handle.cc b/net/socket/client_socket_handle.cc |
| index be58c7296d62c1b763888f0392edb43174f1a931..7c7132405e6bf2e07a027fb28223f04321078a25 100644 |
| --- a/net/socket/client_socket_handle.cc |
| +++ b/net/socket/client_socket_handle.cc |
| @@ -31,6 +31,20 @@ ClientSocketHandle::~ClientSocketHandle() { |
| Reset(); |
| } |
| +void ClientSocketHandle::SetPriority(RequestPriority priority) { |
| + if (socket_) { |
| + // The priority of the handle is no longer relevant to the socket pool; |
| + // just return. |
| + // |
| + // TODO (rdsmith): Incorporate change in priority information into TCP |
| + // params on this socket. |
|
mmenke
2017/01/05 20:18:35
TransportSocketParams don't have a priority, no so
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.
Randy Smith (Not in Mondays)
2017/01/13 23:05:44
Sorry, didn't mean TransportSocketParams, more TCP
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.
|
| + return; |
| + } |
| + |
| + if (pool_) |
|
mmenke
2017/01/05 20:18:35
Just destroying the socket seems to return it to t
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.
Randy Smith (Not in Mondays)
2017/01/13 23:05:44
Believe the result of this comment was tracked at
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.
mmenke
2017/01/17 18:56:31
That's right.
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.
|
| + pool_->SetPriority(group_name_, this, priority); |
|
mmenke
2017/01/05 20:18:35
Let me try and get a better...handle... on the li
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.
Randy Smith (Not in Mondays)
2017/01/13 23:05:44
See last comment response :-}.
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 :-}.
|
| +} |
| + |
| void ClientSocketHandle::Reset() { |
| ResetInternal(true); |
| ResetErrorState(); |
