ABANDONED Allow SpdySessionPool to find/create SpdySession with IP pooling disabled.

ABANDONED Allow SpdySessionPool to find/create SpdySession with IP pooling disabled.

Abandoned in favor of https://crrev.com/2756503002.

This is in preparation to support HTTP response status 421 Misdirected Request,
when the client may retry the request on a different connection, preferably on
one with IP based pooling disabled.

* Add |enable_ip_based_pooling| argument to
  SpdySessionPool::FindAvailableSession(),
  SpdySessionPool::MapKeyToAvailableSession(),
  SpdySessionPool::UnmapKey() methods.
* Implement container SpdySessionPool::AvailableSessionMap.
* Add unittest; add test helper method.

BUG=546991

[Bence, 2017-03-14 20:08:51]

The CQ bit was checked by bnc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 20:09:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bence, 2017-03-14 21:50:33]

bnc@chromium.org changed reviewers:
+ rch@chromium.org

[Bence, 2017-03-14 21:50:33]

Ryan: PTAL.  Thanks.

[commit-bot: I haz the power, 2017-03-14 22:01:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-14 22:01:04]

Dry run: This issue passed the CQ dry run.

[Ryan Hamilton, 2017-03-14 22:20:20]

Wow, that's some dense code! Thanks for working on correct 421 handling.

https://codereview.chromium.org/2749123002/diff/1/net/spdy/spdy_session_pool.cc
File net/spdy/spdy_session_pool.cc (right):

https://codereview.chromium.org/2749123002/diff/1/net/spdy/spdy_session_pool....
net/spdy/spdy_session_pool.cc:251:
UnmapKey(available_session->spdy_session_key(), /* ip_pooled = */ false);
Can you add a comment for why we use false here, and true below. (1 non-pooled,
and N-pooled entries, I believe)

https://codereview.chromium.org/2749123002/diff/1/net/spdy/spdy_session_pool....
net/spdy/spdy_session_pool.cc:482: // ++(std_map.end()) is undefined, and so is
++(available_session_map.end()).
DCHECK_NE(available_session_map.end(), iterator_);

or some such?

https://codereview.chromium.org/2749123002/diff/1/net/spdy/spdy_session_pool....
net/spdy/spdy_session_pool.cc:485: if (ip_pooled_)
Can you add a comment which explains why this is true? (basically that first we
go through non-pooled, then pooled. So if we're already pooled we can skip the
special logic below. (or something)

https://codereview.chromium.org/2749123002/diff/1/net/spdy/spdy_session_pool....
net/spdy/spdy_session_pool.cc:491: // which yields the internal representation
of available_session_map.end().
I don't understand this comment. Perhaps you're saying that if
ip_pooled_sessions_ is empty() then being() will return end() which is correct?

https://codereview.chromium.org/2749123002/diff/1/net/spdy/spdy_session_pool....
net/spdy/spdy_session_pool.cc:518: } else {
nit: since 517 returned, you can avoid the else here (and the associated
nesting).

https://codereview.chromium.org/2749123002/diff/1/net/spdy/spdy_session_pool....
net/spdy/spdy_session_pool.cc:522: non_ip_pooled_sessions_.cbegin(), /*
ip_pooled = */ false, this);
I think it would make more sense to move all of this logic to the ::Iterator
constructor. In this case, you could pass in only the session map and let the
iterator construct the correct state internally. Since T iterator and ip_pool
are basically private, that would put all the logic in the same place, I think.

https://codereview.chromium.org/2749123002/diff/1/net/spdy/spdy_session_pool....
net/spdy/spdy_session_pool.cc:530: ip_pooled_sessions_.cend(), /* ip_pooled = */
true, this);
Consider implementing end() directly in Iterator 'cause this feel a bit like an
encapsulation violation.

https://codereview.chromium.org/2749123002/diff/1/net/spdy/spdy_session_pool....
net/spdy/spdy_session_pool.cc:548: }
ditto to const begin.

https://codereview.chromium.org/2749123002/diff/1/net/spdy/spdy_session_pool.h
File net/spdy/spdy_session_pool.h (right):

https://codereview.chromium.org/2749123002/diff/1/net/spdy/spdy_session_pool....
net/spdy/spdy_session_pool.h:80: // called if a
FindAvailableSession(enable_ip_based_pooling = false) call
nit: I'd just remove "enable... = " from this, or else wrap it in /**/s so as to
look more like actual code. Oh, but that method takes more arguments. Confusing
perhaps:

.. if a call to Find..Session() with enable_ip_based_pooling false would return
true.

https://codereview.chromium.org/2749123002/diff/1/net/spdy/spdy_session_pool....
net/spdy/spdy_session_pool.h:81: // would return true (or just returned true).
nit: I'd remove the "(or just returned true)" clause here.

https://codereview.chromium.org/2749123002/diff/1/net/spdy/spdy_session_pool....
net/spdy/spdy_session_pool.h:194: template <class T, class U>
I'm sure it's obvious, but why does this need to be templated?

[Bence, 2017-03-15 00:03:47]

The CQ bit was checked by bnc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 00:04:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 01:33:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-15 01:33:19]

Dry run: This issue passed the CQ dry run.

[Bence, 2017-03-15 10:05:27]

PTAL.

https://codereview.chromium.org/2749123002/diff/1/net/spdy/spdy_session_pool.cc
File net/spdy/spdy_session_pool.cc (right):

https://codereview.chromium.org/2749123002/diff/1/net/spdy/spdy_session_pool....
net/spdy/spdy_session_pool.cc:251:
UnmapKey(available_session->spdy_session_key(), /* ip_pooled = */ false);
On 2017/03/14 22:20:20, Ryan Hamilton wrote:
> Can you add a comment for why we use false here, and true below. (1
non-pooled,
> and N-pooled entries, I believe)

Done.

https://codereview.chromium.org/2749123002/diff/1/net/spdy/spdy_session_pool....
net/spdy/spdy_session_pool.cc:482: // ++(std_map.end()) is undefined, and so is
++(available_session_map.end()).
On 2017/03/14 22:20:20, Ryan Hamilton wrote:
> DCHECK_NE(available_session_map.end(), iterator_);
> 
> or some such?

Done.

https://codereview.chromium.org/2749123002/diff/1/net/spdy/spdy_session_pool....
net/spdy/spdy_session_pool.cc:485: if (ip_pooled_)
On 2017/03/14 22:20:20, Ryan Hamilton wrote:
> Can you add a comment which explains why this is true? (basically that first
we
> go through non-pooled, then pooled. So if we're already pooled we can skip the
> special logic below. (or something)

Done.

https://codereview.chromium.org/2749123002/diff/1/net/spdy/spdy_session_pool....
net/spdy/spdy_session_pool.cc:491: // which yields the internal representation
of available_session_map.end().
On 2017/03/14 22:20:20, Ryan Hamilton wrote:
> I don't understand this comment. Perhaps you're saying that if
> ip_pooled_sessions_ is empty() then being() will return end() which is
correct?

Done.

https://codereview.chromium.org/2749123002/diff/1/net/spdy/spdy_session_pool....
net/spdy/spdy_session_pool.cc:518: } else {
On 2017/03/14 22:20:20, Ryan Hamilton wrote:
> nit: since 517 returned, you can avoid the else here (and the associated
> nesting).

Done.

https://codereview.chromium.org/2749123002/diff/1/net/spdy/spdy_session_pool....
net/spdy/spdy_session_pool.cc:522: non_ip_pooled_sessions_.cbegin(), /*
ip_pooled = */ false, this);
On 2017/03/14 22:20:20, Ryan Hamilton wrote:
> I think it would make more sense to move all of this logic to the ::Iterator
> constructor. In this case, you could pass in only the session map and let the
> iterator construct the correct state internally. Since T iterator and ip_pool
> are basically private, that would put all the logic in the same place, I
think.

Done.  Ended up using a static factory method.  Templating allows me to use the
same method for iterator and const_iterator.

https://codereview.chromium.org/2749123002/diff/1/net/spdy/spdy_session_pool....
net/spdy/spdy_session_pool.cc:530: ip_pooled_sessions_.cend(), /* ip_pooled = */
true, this);
On 2017/03/14 22:20:20, Ryan Hamilton wrote:
> Consider implementing end() directly in Iterator 'cause this feel a bit like
an
> encapsulation violation.

Done.

https://codereview.chromium.org/2749123002/diff/1/net/spdy/spdy_session_pool....
net/spdy/spdy_session_pool.cc:548: }
On 2017/03/14 22:20:20, Ryan Hamilton wrote:
> ditto to const begin.

Done.

https://codereview.chromium.org/2749123002/diff/1/net/spdy/spdy_session_pool.h
File net/spdy/spdy_session_pool.h (right):

https://codereview.chromium.org/2749123002/diff/1/net/spdy/spdy_session_pool....
net/spdy/spdy_session_pool.h:80: // called if a
FindAvailableSession(enable_ip_based_pooling = false) call
On 2017/03/14 22:20:20, Ryan Hamilton wrote:
> nit: I'd just remove "enable... = " from this, or else wrap it in /**/s so as
to
> look more like actual code. Oh, but that method takes more arguments.
Confusing
> perhaps:
> 
> .. if a call to Find..Session() with enable_ip_based_pooling false would
return
> true.

Done.

https://codereview.chromium.org/2749123002/diff/1/net/spdy/spdy_session_pool....
net/spdy/spdy_session_pool.h:81: // would return true (or just returned true).
On 2017/03/14 22:20:20, Ryan Hamilton wrote:
> nit: I'd remove the "(or just returned true)" clause here.

Done.

https://codereview.chromium.org/2749123002/diff/1/net/spdy/spdy_session_pool....
net/spdy/spdy_session_pool.h:194: template <class T, class U>
On 2017/03/14 22:20:20, Ryan Hamilton wrote:
> I'm sure it's obvious, but why does this need to be templated?

Good question.  It totally wasn't obvious to me, and I tried hard not to make it
templated.  Turns out that SpdySessionPool::IsSessionAvailable() is marked
const, so it needs const iterators, and its available_sessions_ member also
needs to be treated const.  I felt that it would be wrong to cast away
constness.

https://codereview.chromium.org/2749123002/diff/1/net/spdy/spdy_session_pool....
net/spdy/spdy_session_pool.h:208: friend SpdySessionPool::AvailableSessionMap;
I still need this friend to allow AvailableSessionMap::find() to use the private
constructor.  Does that feel like an encapsulation violation?  Shall I refactor
that to a static Iterator::find(), just like I did with begin() and end()?

[Bence, 2017-03-15 23:58:40]

https://crrev.com/2756503002 might be a better way to solve the very same
problem.

[Bence, 2017-03-16 00:42:23]

Description was changed from

==========
Allow SpdySessionPool to find/create SpdySession with IP pooling disabled.

This is in preparation to support HTTP response status 421 Misdirected Request,
when the client may retry the request on a different connection, preferably on
one with IP based pooling disabled.

* Add |enable_ip_based_pooling| argument to
  SpdySessionPool::FindAvailableSession(),
  SpdySessionPool::MapKeyToAvailableSession(),
  SpdySessionPool::UnmapKey() methods.
* Implement container SpdySessionPool::AvailableSessionMap.
* Add unittest; add test helper method.

BUG=546991
==========

to

==========
ABANDONED Allow SpdySessionPool to find/create SpdySession with IP pooling
disabled.

Abandoned in favor of https://crrev.com/2756503002.

This is in preparation to support HTTP response status 421 Misdirected Request,
when the client may retry the request on a different connection, preferably on
one with IP based pooling disabled.

* Add |enable_ip_based_pooling| argument to
  SpdySessionPool::FindAvailableSession(),
  SpdySessionPool::MapKeyToAvailableSession(),
  SpdySessionPool::UnmapKey() methods.
* Implement container SpdySessionPool::AvailableSessionMap.
* Add unittest; add test helper method.

BUG=546991
==========