New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 791055 link

Starred by 0 users
Status: Fixed
Owner:
b...@chromium.org
Closed: Dec 2017
Cc:
█ xunji...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 3
Type: Bug

Blocking:
issue 776415



Sign in to add a comment

Implement Http2PushPromiseIndex::Delegate.

Project Member Reported by b...@chromium.org, Dec 1 2017 
SpdySession depends on Http2PushPromiseIndex, and Http2PushPromiseIndex depends on SpdySession.  This kind of circular dependency is considered bad and should be changed.  One reason is testability: in order to solve  issue 776415 , Http2PushPromiseIndex must be able to claim pushed streams (when HttpStreamFactoryImpl::Job requests so), and notify SpdySession.  However, SpdySession might require that the stream actually exists, like it used to do in order to log histogram Net.PushedStreamAlreadyHasResponseHeaders which was recently removed in https://crrev.com/c/771544.  In order to be able to test Http2PushPromiseIndex in isolation, it should interact with a Delegate, instead of SpdySession, and this Delegate can have a trivial implementation for tests, or a mock implementation for ensuring certain methods are called etc.

At least one appropriate DCHECK() cannot be a added to SpdySession [1] because real instances are used in Http2PushPromiseIndexTest.  While this particular one will be solved once  issue 791054  is solved, using a Delegate will allow for other meaningful checks (like stream must be active).

[1] https://chromium-review.googlesource.com/c/chromium/src/+/797438/2/net/spdy/chromium/spdy_session.cc#1365
Project Member

Comment 1 by bugdroid1@chromium.org, Dec 2 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/08800cd152b074783610f16c24bdd16600e8e6d8

commit 08800cd152b074783610f16c24bdd16600e8e6d8
Author: Bence Béky <bnc@chromium.org>
Date: Sat Dec 02 00:41:09 2017

Create Http2PushPromiseIndex::Delegate.

This CL is in preparation for https://crrev.com/c/734223.

Create Http2PushPromiseIndex::Delegate and thus remove circular dependency
between Http2PushPromiseIndex and SpdySession.

Also, change the internal container type of Http2PushPromiseIndex from
map<vector> to set<pair>.  Later, when
SpdySession::UnclaimedStreamContainer is torn out from SpdySession and
is merged into Http2PushPromiseIndex, there will be multile set<tuple>
containers, storing identical data but sorted by different keys, kept in
sync internally (because fast lookup will be required both by GURL and
by SpdySession*).  set<tuple> will be a lot simpler for this purpose
than map<vector>.

Also, unlike in draft https://crrev.com/c/734223, keep only Delegate raw
pointers and use those to generate the WeakPtr<SpdySession>, instead of
keeping a copy of WeakPtr in addition.  Again, this is to simplify
things.

Bug:  791055 
Change-Id: I8c9fcd77c85f3801eb24ca171c66d18ea027965b
Reviewed-on: https://chromium-review.googlesource.com/797438
Commit-Queue: Bence Béky <bnc@chromium.org>
Reviewed-by: Helen Li <xunjieli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521156}
[modify] https://crrev.com/08800cd152b074783610f16c24bdd16600e8e6d8/net/spdy/chromium/http2_push_promise_index.cc
[modify] https://crrev.com/08800cd152b074783610f16c24bdd16600e8e6d8/net/spdy/chromium/http2_push_promise_index.h
[modify] https://crrev.com/08800cd152b074783610f16c24bdd16600e8e6d8/net/spdy/chromium/http2_push_promise_index_test.cc
[modify] https://crrev.com/08800cd152b074783610f16c24bdd16600e8e6d8/net/spdy/chromium/spdy_session.cc
[modify] https://crrev.com/08800cd152b074783610f16c24bdd16600e8e6d8/net/spdy/chromium/spdy_session.h

Comment 2 by b...@chromium.org, Dec 6 2017

Status: Fixed (was: Started)
https://crrev.com/c/809187 landed to clean up Http2PushPromiseTests (remove actual SpdySession instances, add test/mock Delegate implementations etc.).  This issue is now Fixed.
►

Sign in to add a comment