Issue 3432009: Add a new class SpdyProxyClientSocket which implements ClientSocket...

Ryan Hamilton

Hi Guys, Here's my first pass at the SpdyProxyClientSocket. It's not wired into the StreamFactory ...

10 years, 3 months ago (2010-09-16 23:32:09 UTC) #1

Ryan Hamilton

I should have mentioned that there is common functionality between this class and HttpProxyClientSocket which ...

10 years, 3 months ago (2010-09-16 23:33:16 UTC) #2

Paweł Hajdan Jr.

Drive-by with some test comments. http://codereview.chromium.org/3432009/diff/26003/53009 File net/spdy/spdy_proxy_client_socket_unittest.cc (right): http://codereview.chromium.org/3432009/diff/26003/53009#newcode141 net/spdy/spdy_proxy_client_socket_unittest.cc:141: MessageLoop::current()->RunAllPending(); Running RunAllPending twice ...

10 years, 3 months ago (2010-09-16 23:59:42 UTC) #3

Ryan Hamilton

Thanks for the suggestions! http://codereview.chromium.org/3432009/diff/26003/53009 File net/spdy/spdy_proxy_client_socket_unittest.cc (right): http://codereview.chromium.org/3432009/diff/26003/53009#newcode141 net/spdy/spdy_proxy_client_socket_unittest.cc:141: MessageLoop::current()->RunAllPending(); On 2010/09/16 23:59:42, Paweł ...

10 years, 3 months ago (2010-09-17 00:22:36 UTC) #4

willchan no longer on Chromium

Stepping back for a second, why is this a ClientSocket instead of a HttpStream? It ...

10 years, 3 months ago (2010-09-17 00:57:26 UTC) #6

rch (use chromium not google)

On Thu, Sep 16, 2010 at 5:57 PM, <willchan@chromium.org> wrote: > Stepping back for a ...

10 years, 3 months ago (2010-09-17 03:29:31 UTC) #7

On Thu, Sep 16, 2010 at 5:57 PM, <willchan@chromium.org> wrote:

> Stepping back for a second, why is this a ClientSocket instead of a
> HttpStream?
>

It's a ClientSocket not an HttpStream because it needs to be a Socket and
not a Stream :>

But seriously, the point of the SpdyHttpProxyClientSocket is much like that
of the HttpProxyClient socket (when used to create a tunnel).  After
Connect() is called, we need something that we can use to perform an SSL
handshake over.  Through *that* SSL connection, we might even end up doing
SPDY!

> It seems like what you want is to do proxy resolution in HttpStreamRequest,
> determine that you want to use a SPDY proxy, get a SpdySession from the
> SpdySessionPool if it exists or create a new one (this reminds me to add
> late
> binding support to SpdySessionPool), get a SpdyStream from the SpdySession,

I'm definitely with you up until this point.  This is basically how I intend
to integrate this code into the StreamFactory.  There is one slight wrinkle
in that we don't know if we're going to use a SPDY proxy (versus a vanilla
HTTPS proxy) until after we have the SSL connection to the proxy.  As you
say, this is very much like the late binding in the direct case.

> wrap
> this SpdyStream in a new subtype of HttpStream (SpdyProxyHttpStream?) a la
> SpdyHttpStream, and then call
> SendRequest()/ReadResponseHeaders()/ReadResponseBody() on it.

Are you thinking that I want to call SendRequest/ReadResponse in order to
perform the CONNECT, but that we would then do something else to read/write
the subsequent data (of, say, the SSL handshake and then the various HTTP or
SPDY transactions that happen on top of it?)  I suppose I could change my
implementation to *also* implement HttpStream, but I'm not sure that I
understand what that would buy us?

> ClientSockets are
> basically always managed by a ClientSocketPool in our code, but I don't
> think
> you want to do that.  It seems like you want the stream factory to request
> streams from the SpdySession that is connected to the proxy, and then wrap
> those
> SpdyStreams in some bridge class to HttpStream, similar to SpdyHttpStream.
>

Well, I think this needs to be a ClientSocket, because I think this is going
to be used as the transport_socket for an SSLClientSocket.  I don't think
there is any obvious alternative.  Does that make sense?  I think it may
well need to be managed by a pool.  If you're configured to use a SPDY proxy
and you fetch https://www.google.com/, that'll end up creating an SSL tunnel
through the proxy to www.google.com.  After you're finished with the request
for /, you'll probably want to re-use the connection for and subsequent
https://www.google.com pages.

> Also, this changelist has a similar weak dependency on
> HttpProxyClientSocket as
> the previous changelist did wrt building headers.  How about moving the
> authentication header creation to net/http/http_utils.h?

Sounds good.  Actually, how about net/http_proxy_utils.h since the method is
specific to using an HTTP (or HTTPS) Proxy?

rch (use chromium not google)

On Thu, Sep 16, 2010 at 8:29 PM, Ryan Hamilton <rch@google.com> wrote: > > Sounds ...

10 years, 3 months ago (2010-09-17 03:48:01 UTC) #8

willchan no longer on Chromium

On 2010/09/17 03:29:31, rch wrote: > On Thu, Sep 16, 2010 at 5:57 PM, <mailto:willchan@chromium.org> ...

10 years, 3 months ago (2010-09-17 04:14:54 UTC) #9

On 2010/09/17 03:29:31, rch wrote:
> On Thu, Sep 16, 2010 at 5:57 PM, <mailto:willchan@chromium.org> wrote:
> 
> > Stepping back for a second, why is this a ClientSocket instead of a
> > HttpStream?
> >
> 
> It's a ClientSocket not an HttpStream because it needs to be a Socket and
> not a Stream :>
> 
> But seriously, the point of the SpdyHttpProxyClientSocket is much like that
> of the HttpProxyClient socket (when used to create a tunnel).  After
> Connect() is called, we need something that we can use to perform an SSL
> handshake over.  Through *that* SSL connection, we might even end up doing
> SPDY!

Wowza!  I sat here for 10 minutes just trying to grok all of this.  I have many
questions.  Some don't belong here.  Some do.  Overall, I agree.  You're right,
ClientSocket is the right abstraction here.

How do you imagine a SPDY proxy working?  Should users be opening up a new
SpdyStream for each http transaction?  The SPDY proxy could try to do the smarts
of keeping HTTP keep-alive connections to the destination.  But I guess that we
want to be reusing the same SpdyStream if we're going to tunnel SSL over it. 
But things get weird if we don't know if the proxy is an HTTPS proxy or a SPDY
proxy until NPN, because if we use a pool to manage reuse of
SpdyProxyClientSockets, the pool needs to know beforehand what its connection
per host and total connection limit should be.  Clearly these policies should be
different for a SPDY proxy vs a standard HTTPS proxy.  Are you sure you want to
rely on NPN for this?  I guess the only other solution would be to use a spdy://
scheme or something to explicitly specify the protocol.  Wow my brain hurts.

No need to answer these questions all here.  This is roughly the right approach
for now.  I'm just wondering how it'll integrate in the future.

I didn't really read through your code in much detail, but I notice you keep a
reference to the SpdySession.  I think that's suboptimal.  It looks like you do
it for the NetLog and to get the peer address.  I think I'd rather you plumb the
peer address through the SpdyStream.  And the NetLog should come from the
SpdyStream, not the SpdySession.  SpdySession has its own special NetLog.  You
want the one tied to the URLRequest, which is plumbed through SpdyStream.

> 
> 
> > It seems like what you want is to do proxy resolution in HttpStreamRequest,
> > determine that you want to use a SPDY proxy, get a SpdySession from the
> > SpdySessionPool if it exists or create a new one (this reminds me to add
> > late
> > binding support to SpdySessionPool), get a SpdyStream from the SpdySession,
> 
> 
> I'm definitely with you up until this point.  This is basically how I intend
> to integrate this code into the StreamFactory.  There is one slight wrinkle
> in that we don't know if we're going to use a SPDY proxy (versus a vanilla
> HTTPS proxy) until after we have the SSL connection to the proxy.  As you
> say, this is very much like the late binding in the direct case.
> 
> 
> > wrap
> > this SpdyStream in a new subtype of HttpStream (SpdyProxyHttpStream?) a la
> > SpdyHttpStream, and then call
> > SendRequest()/ReadResponseHeaders()/ReadResponseBody() on it.
> 
> 
> Are you thinking that I want to call SendRequest/ReadResponse in order to
> perform the CONNECT, but that we would then do something else to read/write
> the subsequent data (of, say, the SSL handshake and then the various HTTP or
> SPDY transactions that happen on top of it?)  I suppose I could change my
> implementation to *also* implement HttpStream, but I'm not sure that I
> understand what that would buy us?
> 
> 
> > ClientSockets are
> > basically always managed by a ClientSocketPool in our code, but I don't
> > think
> > you want to do that.  It seems like you want the stream factory to request
> > streams from the SpdySession that is connected to the proxy, and then wrap
> > those
> > SpdyStreams in some bridge class to HttpStream, similar to SpdyHttpStream.
> >
> 
> Well, I think this needs to be a ClientSocket, because I think this is going
> to be used as the transport_socket for an SSLClientSocket.  I don't think
> there is any obvious alternative.  Does that make sense?  I think it may
> well need to be managed by a pool.  If you're configured to use a SPDY proxy
> and you fetch https://www.google.com/, that'll end up creating an SSL tunnel
> through the proxy to http://www.google.com.  After you're finished with the
request
> for /, you'll probably want to re-use the connection for and subsequent
> https://www.google.com pages.
> 
> 
> > Also, this changelist has a similar weak dependency on
> > HttpProxyClientSocket as
> > the previous changelist did wrt building headers.  How about moving the
> > authentication header creation to net/http/http_utils.h?
> 
> 
> Sounds good.  Actually, how about net/http_proxy_utils.h since the method is
> specific to using an HTTP (or HTTPS) Proxy?
>

rch (use chromium not google)

On Thu, Sep 16, 2010 at 9:14 PM, <willchan@chromium.org> wrote: > On 2010/09/17 03:29:31, rch ...

10 years, 3 months ago (2010-09-17 04:52:45 UTC) #10

On Thu, Sep 16, 2010 at 9:14 PM, <willchan@chromium.org> wrote:

> On 2010/09/17 03:29:31, rch wrote:
>
>  On Thu, Sep 16, 2010 at 5:57 PM, <mailto:willchan@chromium.org> wrote:
>
>  It's a ClientSocket not an HttpStream because it needs to be a Socket and
>> not a Stream :>
>>
>
>  But seriously, the point of the SpdyHttpProxyClientSocket is much like
>> that
>> of the HttpProxyClient socket (when used to create a tunnel).  After
>> Connect() is called, we need something that we can use to perform an SSL
>> handshake over.  Through *that* SSL connection, we might even end up doing
>> SPDY!
>>
>
> Wowza!  I sat here for 10 minutes just trying to grok all of this.  I have
> many
> questions.  Some don't belong here.  Some do.  Overall, I agree.  You're
> right,
> ClientSocket is the right abstraction here.
>

Heh, I've had that experience several times with this project :>  I'd be
happy to chat in more detail over the phone or IM tomorrow if that would be
useful...

> How do you imagine a SPDY proxy working?  Should users be opening up a new
> SpdyStream for each http transaction?  The SPDY proxy could try to do the
> smarts
> of keeping HTTP keep-alive connections to the destination.  But I guess
> that we
> want to be reusing the same SpdyStream if we're going to tunnel SSL over
> it.
> But things get weird if we don't know if the proxy is an HTTPS proxy or a
> SPDY
> proxy until NPN, because if we use a pool to manage reuse of
> SpdyProxyClientSockets, the pool needs to know beforehand what its
> connection
> per host and total connection limit should be.  Clearly these policies
> should be
> different for a SPDY proxy vs a standard HTTPS proxy.  Are you sure you
> want to
> rely on NPN for this?  I guess the only other solution would be to use a
> spdy://
> scheme or something to explicitly specify the protocol.  Wow my brain
> hurts.
>

Here's how I think it should work (and a bit of what chrome already does).
 If the user has an HTTPS proxy configured, we will establish an SSL
connection to the proxy.  If we do not NPN negotiate SPDY, then we'll treat
the proxy just like we treat an HTTP proxy (except that we're talking to it
over SSL).  We'll do a single request at a time over a given proxy
connection, which can be subsequently reused for new requests.  When we need
to make an HTTPS request, we will use the CONNECT method to create an SSL
tunnel (over the SSL connection to the proxy) to the origin server.  At this
point, this connection can only be reused for other HTTPS connections to the
same origin server.  Afaik, this all works today.

Now, if the proxy does NPN negotiate SPDY, then we'll create a single SPDY
session for communicating with the proxy.  (Its' possible that there is some
performance reason to create multiple sessions, but let's ignore that for
now).  For each request the browser makes, a new SPDY stream will be
created.  The semantics of SPDY pretty much require this.  A single stream
can only have a single pair of SYN_STREAM/SYN_REPLY frames.  (Now, how the
proxy chooses to deal with these requests is up to the proxy.  Even though
it receives a SPDY request, it might issues a vanilla HTTP request to the
origin server.)  When Chrome attempts to make an HTTPS request, it's going
to issue a CONNECT request in a new SPDY stream through the proxy to the
origin server.  Once the proxy opens up the TCP connection to the origin
server, it'll send back a SYN_REPLY 200 COnnection Established frame to
Chrome.  At this point, Chrome will treat that SPDY stream as a "socket",
with the caveat that the data will be encoded in data frames of the spdy
stream.

Does that help?

You raise a good question about the implications of replying on NPN to
decide if we're using a SPDY or an HTTP proxy.  When Mike and I came up with
this scheme a few months back, I didn't think about the implications for the
pools.  I'll have to think about this a bit more.  Do you think we would
ever need/want to create more that 1 *connection* to a SPDY proxy?  Do you
think we need a limit on the number of outstanding *streams* to a proxy?  Do
we limit streams (other than with SPDY flow control) when going direct over
SPDY.

No need to answer these questions all here.  This is roughly the right
> approach
> for now.  I'm just wondering how it'll integrate in the future.
>

Agreed :>

I didn't really read through your code in much detail, but I notice you keep
> a
> reference to the SpdySession.  I think that's suboptimal.  It looks like
> you do
> it for the NetLog and to get the peer address.

Wow, you're right.  For some reason, I thought I used it in more places.

>  I think I'd rather you plumb the
> peer address through the SpdyStream.  And the NetLog should come from the
> SpdyStream, not the SpdySession.  SpdySession has its own special NetLog.
>  You
> want the one tied to the URLRequest, which is plumbed through SpdyStream.

Sounds like a good idea, and it should be fairly simple to implement.  I'll
give that a shot...

willchan no longer on Chromium

On Thu, Sep 16, 2010 at 9:52 PM, Ryan Hamilton <rch@google.com> wrote: > On Thu, ...

10 years, 3 months ago (2010-09-17 05:24:20 UTC) #11

On Thu, Sep 16, 2010 at 9:52 PM, Ryan Hamilton <rch@google.com> wrote:

> On Thu, Sep 16, 2010 at 9:14 PM, <willchan@chromium.org> wrote:
>
>> On 2010/09/17 03:29:31, rch wrote:
>>
>>  On Thu, Sep 16, 2010 at 5:57 PM, <mailto:willchan@chromium.org> wrote:
>>
>>  It's a ClientSocket not an HttpStream because it needs to be a Socket and
>>> not a Stream :>
>>>
>>
>>  But seriously, the point of the SpdyHttpProxyClientSocket is much like
>>> that
>>> of the HttpProxyClient socket (when used to create a tunnel).  After
>>> Connect() is called, we need something that we can use to perform an SSL
>>> handshake over.  Through *that* SSL connection, we might even end up
>>> doing
>>> SPDY!
>>>
>>
>> Wowza!  I sat here for 10 minutes just trying to grok all of this.  I have
>> many
>> questions.  Some don't belong here.  Some do.  Overall, I agree.  You're
>> right,
>> ClientSocket is the right abstraction here.
>>
>
> Heh, I've had that experience several times with this project :>  I'd be
> happy to chat in more detail over the phone or IM tomorrow if that would be
> useful...
>
>
>> How do you imagine a SPDY proxy working?  Should users be opening up a new
>> SpdyStream for each http transaction?  The SPDY proxy could try to do the
>> smarts
>> of keeping HTTP keep-alive connections to the destination.  But I guess
>> that we
>> want to be reusing the same SpdyStream if we're going to tunnel SSL over
>> it.
>> But things get weird if we don't know if the proxy is an HTTPS proxy or a
>> SPDY
>> proxy until NPN, because if we use a pool to manage reuse of
>> SpdyProxyClientSockets, the pool needs to know beforehand what its
>> connection
>> per host and total connection limit should be.  Clearly these policies
>> should be
>> different for a SPDY proxy vs a standard HTTPS proxy.  Are you sure you
>> want to
>> rely on NPN for this?  I guess the only other solution would be to use a
>> spdy://
>> scheme or something to explicitly specify the protocol.  Wow my brain
>> hurts.
>>
>
> Here's how I think it should work (and a bit of what chrome already does).
>  If the user has an HTTPS proxy configured, we will establish an SSL
> connection to the proxy.  If we do not NPN negotiate SPDY, then we'll treat
> the proxy just like we treat an HTTP proxy (except that we're talking to it
> over SSL).  We'll do a single request at a time over a given proxy
> connection, which can be subsequently reused for new requests.  When we need
> to make an HTTPS request, we will use the CONNECT method to create an SSL
> tunnel (over the SSL connection to the proxy) to the origin server.  At this
> point, this connection can only be reused for other HTTPS connections to the
> same origin server.  Afaik, this all works today.
>
> Now, if the proxy does NPN negotiate SPDY, then we'll create a single SPDY
> session for communicating with the proxy.  (Its' possible that there is some
> performance reason to create multiple sessions, but let's ignore that for
> now).  For each request the browser makes, a new SPDY stream will be
> created.  The semantics of SPDY pretty much require this.  A single stream
> can only have a single pair of
>

I don't understand this part.  Why do we need a new SPDY stream per URL
request?  I thought we just need at least one SPDY stream per destination
endpoint, since we conceivably can reuse SpdyProxyClientSockets across URL
requests, right?


> SYN_STREAM/SYN_REPLY frames.  (Now, how the proxy chooses to deal with
> these requests is up to the proxy.  Even though it receives a SPDY request,
> it might issues a vanilla HTTP request to the origin server.)  When Chrome
> attempts to make an HTTPS request, it's going to issue a CONNECT request in
> a new SPDY stream through the proxy to the origin server.  Once the proxy
> opens up the TCP connection to the origin server, it'll send back a
> SYN_REPLY 200 COnnection Established frame to Chrome.  At this point, Chrome
> will treat that SPDY stream as a "socket", with the caveat that the data
> will be encoded in data frames of the spdy stream.
>
> Does that help?
>
> You raise a good question about the implications of replying on NPN to
> decide if we're using a SPDY or an HTTP proxy.  When Mike and I came up with
> this scheme a few months back, I didn't think about the implications for the
> pools.  I'll have to think about this a bit more.  Do you think we would
> ever need/want to create more that 1 *connection* to a SPDY proxy?  Do you
> think we need a limit on the
>

No.  But we probably will have more than 1 *connection* to an HTTPS.  NPN
will make this confusing.


> number of outstanding *streams* to a proxy?  Do we limit streams (other
> than with SPDY flow control) when going direct over SPDY.
>

There's no need to have a hardcoded limit in the browser for the number of
outstanding *streams* to a SPDY proxy, since SPDY proxies can use the max
concurrent streams SETTINGS frame to do this.


>
> No need to answer these questions all here.  This is roughly the right
>> approach
>> for now.  I'm just wondering how it'll integrate in the future.
>>
>
> Agreed :>
>
> I didn't really read through your code in much detail, but I notice you
>> keep a
>> reference to the SpdySession.  I think that's suboptimal.  It looks like
>> you do
>> it for the NetLog and to get the peer address.
>
>
> Wow, you're right.  For some reason, I thought I used it in more places.
>
>
>>  I think I'd rather you plumb the
>> peer address through the SpdyStream.  And the NetLog should come from the
>> SpdyStream, not the SpdySession.  SpdySession has its own special NetLog.
>>  You
>> want the one tied to the URLRequest, which is plumbed through SpdyStream.
>
>
> Sounds like a good idea, and it should be fairly simple to implement.  I'll
> give that a shot...
>
>

rch (use chromium not google)

On Thu, Sep 16, 2010 at 10:24 PM, William Chan (陈智昌) <willchan@chromium.org>wrote: > > > ...

10 years, 3 months ago (2010-09-17 17:45:12 UTC) #12

On Thu, Sep 16, 2010 at 10:24 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

>
>
> On Thu, Sep 16, 2010 at 9:52 PM, Ryan Hamilton <rch@google.com> wrote:
>>
>> Here's how I think it should work (and a bit of what chrome already does).
>>  If the user has an HTTPS proxy configured, we will establish an SSL
>> connection to the proxy.  If we do not NPN negotiate SPDY, then we'll treat
>> the proxy just like we treat an HTTP proxy (except that we're talking to it
>> over SSL).  We'll do a single request at a time over a given proxy
>> connection, which can be subsequently reused for new requests.  When we need
>> to make an HTTPS request, we will use the CONNECT method to create an SSL
>> tunnel (over the SSL connection to the proxy) to the origin server.  At this
>> point, this connection can only be reused for other HTTPS connections to the
>> same origin server.  Afaik, this all works today.
>>
>> Now, if the proxy does NPN negotiate SPDY, then we'll create a single SPDY
>> session for communicating with the proxy.  (Its' possible that there is some
>> performance reason to create multiple sessions, but let's ignore that for
>> now).  For each request the browser makes, a new SPDY stream will be
>> created.  The semantics of SPDY pretty much require this.  A single stream
>> can only have a single pair of
>>
>
> I don't understand this part.  Why do we need a new SPDY stream per URL
> request?  I thought we just need at least one SPDY stream per destination
> endpoint, since we conceivably can reuse SpdyProxyClientSockets across URL
> requests, right?
>

Ah, so I glossed over an important detail.  In the case of fetching an
http:// url, there is no need to create an SSL tunnel to the origin server,
and hence no need for a SpdyProxyClientSocket.  Perhaps this suggest the
name should be SpdyProxyTunnelClientSocket.  It's only when we fetch an
https:// url that we need to create a tunnel to the origin.  If we're not
setting up a tunnel, then each URL requested will take place over a
different SPDY stream between the client and the proxy.  If we *are* setting
up a tunnel, then all traffic between the client and the origin wil take
place through the tunnel, and hence on the same SPDY stream.  Of course,
just to add one level of complication to the mix, the SSL tunnel to the
origin may or may not negotiate SPDY, so it turns out there are 6 different
ways to fetch a url from an origin server via an HTTPS proxy.  Isn't this
exciting!!! :>  Here's a page I created which attempts to document all of
these cases in gory detail.

<goog_377287722>
http://sites.google.com/a/chromium.org/dev/spdy/spdy-proxy-examples

> SYN_STREAM/SYN_REPLY frames.  (Now, how the proxy chooses to deal with
>> these requests is up to the proxy.  Even though it receives a SPDY request,
>> it might issues a vanilla HTTP request to the origin server.)  When Chrome
>> attempts to make an HTTPS request, it's going to issue a CONNECT request in
>> a new SPDY stream through the proxy to the origin server.  Once the proxy
>> opens up the TCP connection to the origin server, it'll send back a
>> SYN_REPLY 200 COnnection Established frame to Chrome.  At this point, Chrome
>> will treat that SPDY stream as a "socket", with the caveat that the data
>> will be encoded in data frames of the spdy stream.
>>
>> Does that help?
>>
>> You raise a good question about the implications of replying on NPN to
>> decide if we're using a SPDY or an HTTP proxy.  When Mike and I came up with
>> this scheme a few months back, I didn't think about the implications for the
>> pools.  I'll have to think about this a bit more.  Do you think we would
>> ever need/want to create more that 1 *connection* to a SPDY proxy?  Do you
>> think we need a limit on the
>>
>
> No.  But we probably will have more than 1 *connection* to an HTTPS.  NPN
> will make this confusing.
>

Ok, in this case, I think we might be able to put off worrying about this
issue for a while.  Lets say that every socket to the HTTPS proxy will come
from an HttpsProxyClientSocketPool, which has the same limit as the
HttpProxyClientSocketPool.  If it happens that the proxy negotiates NPN,
we'll create a new SpdySession out of it, and add that to the
SpdySessionPool.  Subsequent requests will create new streams from that
session.  It'll happen that we never attempt to create an additional
Connection/SpdySession.

> number of outstanding *streams* to a proxy?  Do we limit streams (other
>> than with SPDY flow control) when going direct over SPDY.
>>
>
> There's no need to have a hardcoded limit in the browser for the number of
> outstanding *streams* to a SPDY proxy, since SPDY proxies can use the max
> concurrent streams SETTINGS frame to do this.
>

Perfect.  That avoid a level of complexity.

 I think I'd rather you plumb the
>>> peer address through the SpdyStream.  And the NetLog should come from the
>>> SpdyStream, not the SpdySession.  SpdySession has its own special NetLog.
>>>  You
>>> want the one tied to the URLRequest, which is plumbed through SpdyStream.
>>
>>
>> Sounds like a good idea, and it should be fairly simple to implement.
>>  I'll give that a shot...
>>
>
Ok, I've uploaded a new patch in which the SpdySession is not passed in to
the SpdyProxyClientSocket constructor, and both the PeerAddress and NetLog
come from the SpdyStream.

willchan no longer on Chromium

I've started on a more detailed review. I haven't read your test much at all ...

10 years, 3 months ago (2010-09-18 01:03:26 UTC) #13

Ryan Hamilton

http://codereview.chromium.org/3432009/diff/21004/44012 File net/http/http_proxy_utils.cc (right): http://codereview.chromium.org/3432009/diff/21004/44012#newcode9 net/http/http_proxy_utils.cc:9: #include "net/base/auth.h" On 2010/09/18 01:03:26, willchan wrote: > Where ...

10 years, 3 months ago (2010-09-20 19:45:42 UTC) #14

http://codereview.chromium.org/3432009/diff/21004/44012
File net/http/http_proxy_utils.cc (right):

http://codereview.chromium.org/3432009/diff/21004/44012#newcode9
net/http/http_proxy_utils.cc:9: #include "net/base/auth.h"
On 2010/09/18 01:03:26, willchan wrote:
> Where is this used?

Done.

http://codereview.chromium.org/3432009/diff/21004/44012#newcode17
net/http/http_proxy_utils.cc:17: const HttpRequestInfo* request_info,
On 2010/09/18 01:03:26, willchan wrote:
> We don't use |request_info| except to get the url.  How about just passing in
> the GURL?

Looks like we also use it for the user-agent:

std::string user_agent;
if (request_info->extra_headers.GetHeader(HttpRequestHeaders::kUserAgent,
                                        &user_agent))
request_headers->SetHeader(HttpRequestHeaders::kUserAgent, user_agent);

I can change the signature so that it takes the user agent too, but I wonder if
that's just going to make more work for the caller?  What do you think?

http://codereview.chromium.org/3432009/diff/21004/44012#newcode19
net/http/http_proxy_utils.cc:19: const HostPortPair& endpoint, std::string*
request_line,
On 2010/09/18 01:03:26, willchan wrote:
> I realize that you probably just copied this.  But I'm hitting you on nits as
> you move code over.  Sorry!
> 
> There was a chromium-dev email about this.  Please do 1 per line, unless the
> parameters are related.

Done.  Ah!  That sounds great.  I thought that the style was 1 per line, but I'd
seen some instances where that was not the case.  Looks like I made the wrong
conclusion.  (Was this a recent chromium-dev thread?  I don't think I've seen
much traffic since I joined)

http://codereview.chromium.org/3432009/diff/21004/44015
File net/spdy/spdy_framer.h (right):

http://codereview.chromium.org/3432009/diff/21004/44015#newcode32
net/spdy/spdy_framer.h:32: class SpdyProxyClientSocketTest;
On 2010/09/18 01:03:26, willchan wrote:
> Just because I'm OCD like some other people, this should be alphabetized.  I
> know the other ones aren't either.  I'd sort them all.
> 
> Meta-comment.  This is annoying, because we have to keep this in sync with the
> server-side code.  We probably should have one "peer" object that is friended.

> Then we wouldn't have to do this for every new test fixture.  I'd create a
> SpdyFramerPeer class in a separate file (so changes to it don't have to be
kept
> in sync with server side changes).  You don't have to do this though.

Done.  Is it a manual process to keep this in sync with the server side?

http://codereview.chromium.org/3432009/diff/21004/44017
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/3432009/diff/21004/44017#newcode23
net/spdy/spdy_proxy_client_socket.cc:23: const HostPortPair& endpoint, const
GURL& url,
On 2010/09/18 01:03:26, willchan wrote:
> 1 param per line.  See
> http://dev.chromium.org/developers/coding-style#Code_Formatting.

Done.

http://codereview.chromium.org/3432009/diff/21004/44017#newcode177
net/spdy/spdy_proxy_client_socket.cc:177: if (spdy_stream_->closed())
On 2010/09/18 01:03:26, willchan wrote:
> Er, isn't this an error if we receive a Write() when the stream is closed? 
And
> if you listen to my suggestion below about releasing the stream in OnClose(),
> then perhaps you should check for NULL instead and if it's non-NULL,
> DCHECK(spdy_stream_->closed()).

Ok, I misunderstood the semantics here.  I thought that Write() was intended
behave like Read(), which is to say, return 0 because 0 on EOF.  How does
ERR_FAILED sound to you?

http://codereview.chromium.org/3432009/diff/21004/44017#newcode291
net/spdy/spdy_proxy_client_socket.cc:291:
auth_->AddAuthorizationHeader(&authorization_headers);
On 2010/09/18 01:03:26, willchan wrote:
> indentation is off here.

Done.

http://codereview.chromium.org/3432009/diff/21004/44017#newcode299
net/spdy/spdy_proxy_client_socket.cc:299: net_log_.AddEvent(
On 2010/09/18 01:03:26, willchan wrote:
> indentation is off here, 2 spaces, not 4

Done.

http://codereview.chromium.org/3432009/diff/21004/44017#newcode377
net/spdy/spdy_proxy_client_socket.cc:377: // TODO(rch): figure out why
auth_->HandleAuthChallenge() returns
On 2010/09/18 01:03:26, willchan wrote:
> Have you figured this out?

No, not yet.  Steve suggested I talk to cbentzel after he gets back in the
office.  It's possible that this behavior is an artifact of some inadequacy in
my unit test setup, or that I need to tweak something in the auth code.  I'm not
sure.

http://codereview.chromium.org/3432009/diff/21004/44017#newcode395
net/spdy/spdy_proxy_client_socket.cc:395: DCHECK(next_state_ ==
STATE_SEND_REQUEST_COMPLETE);
On 2010/09/18 01:03:26, willchan wrote:
> Use DCHECK_EQ.

Done.

http://codereview.chromium.org/3432009/diff/21004/44017#newcode436
net/spdy/spdy_proxy_client_socket.cc:436: IOBufferWithSize* io_buffer = new
IOBufferWithSize(length);
On 2010/09/18 01:03:26, willchan wrote:
> Perhaps you should use DrainableIOBuffer so you don't have to do all this
index
> management yourself.

Done!  Excellent!  I was able to use DrainableIOBuffers both here and for the
user's read buffer.  This is much cleaner!

http://codereview.chromium.org/3432009/diff/21004/44017#newcode456
net/spdy/spdy_proxy_client_socket.cc:456: DCHECK(write_buffer_len_  >=
num_bytes_written_);
On 2010/09/18 01:03:26, willchan wrote:
> DCHECK_GE

Done.

http://codereview.chromium.org/3432009/diff/21004/44017#newcode458
net/spdy/spdy_proxy_client_socket.cc:458: // TODO(rch): what about short write
on stream abort?
On 2010/09/18 01:03:26, willchan wrote:
> What does this mean?  You'll get an OnDataSent() for whatever got sent.  After
> that, you'll get an OnClose() with the error.

Good point.  I misunderstood the interaction between Write and Close.

http://codereview.chromium.org/3432009/diff/21004/44017#newcode471
net/spdy/spdy_proxy_client_socket.cc:471: OnDataReceived(NULL, 0);
On 2010/09/18 01:03:26, willchan wrote:
> Wait, is this what we want?  What if you have a write pending?  How are you
> going to report an error there?  You should probably write code here to try
the
> read callback if it exists, otherwise try the write callback.  I haven't read
> your tests yet, but make sure you cover this case.
> 
> Also, you may want to try setting stream_ to NULL at this point to release the
> memory.

Done.  We chatted over IM, and I believe I understand the semantics around close
more clearly.  I wanted to call the read callback to flush any buffered read
data, but you pointed out that we should not attempt to buffer up a full read so
there's no need to flush here.

http://codereview.chromium.org/3432009/diff/21004/44018
File net/spdy/spdy_proxy_client_socket.h (right):

http://codereview.chromium.org/3432009/diff/21004/44018#newcode44
net/spdy/spdy_proxy_client_socket.h:44:
SpdyProxyClientSocket(scoped_refptr<SpdyStream> spdy_stream,
On 2010/09/18 01:03:26, willchan wrote:
> According to chromium-dev, the function prototype should take a raw pointer
> instead.

To make sure I understand, we want the prototype to be SpdyStream*, but the
member field can be scoped_refpr<SpdyStream> spdy_stream_? Done.

Do you have a link to the thread?  I've love to understand this issue better.

http://codereview.chromium.org/3432009/diff/21004/44018#newcode174
net/spdy/spdy_proxy_client_socket.h:174: int user_buffer_idx_;  // index of next
free byte
On 2010/09/18 01:03:26, willchan wrote:
> Our style guide recommends against names such as |idx|.  Use index instead.

Done.  (Well, then I switched to a DrainableIOBuffer :>)

http://codereview.chromium.org/3432009/diff/21004/44019
File net/spdy/spdy_proxy_client_socket_unittest.cc (right):

http://codereview.chromium.org/3432009/diff/21004/44019#newcode29
net/spdy/spdy_proxy_client_socket_unittest.cc:29: static const char* const kUrl
= "https://www.google.com/";
On 2010/09/18 01:03:26, willchan wrote:
> You should probably just do something like
> const char kUrl[] = ...;
> 
> static is unnecessary (but optional, so include if you want) since it's in an
> anonymous namespace.
> 
> as far as the [] vs a pointer, the pointer requires the extra 32/64 bits.  I
> don't know that it's very useful unless you ever wanted to pass the address of
> the pointer itself.  Usually you just want the array of data.

Done.

http://codereview.chromium.org/3432009/diff/21004/44019#newcode67
net/spdy/spdy_proxy_client_socket_unittest.cc:67: const char * status = "200
Connection Established");
On 2010/09/18 01:03:26, willchan wrote:
> Be consistent with your formatting of pointers/references.  In this case, you
> probably don't need the space between char and *.

Done.  Sorry, I thought I had cured my self of "type *var", but it looks like I
had a relapse :<

http://codereview.chromium.org/3432009/diff/21004/44019#newcode74
net/spdy/spdy_proxy_client_socket_unittest.cc:74: void
CheckAsyncReadEquals(const char* data, int len, int num_writes = 1);
On 2010/09/18 01:03:26, willchan wrote:
> Default argument values are against the Google C++ style guide.

Done.  (No method overloading, and no default arguments?  *whimper*)

willchan no longer on Chromium

Sorry for the delay, here are some preliminary answers to your questions. I'll do another ...

10 years, 3 months ago (2010-09-21 17:55:31 UTC) #15

Sorry for the delay, here are some preliminary answers to your questions.  I'll
do another pass shortly.

http://codereview.chromium.org/3432009/diff/21004/44012
File net/http/http_proxy_utils.cc (right):

http://codereview.chromium.org/3432009/diff/21004/44012#newcode17
net/http/http_proxy_utils.cc:17: const HttpRequestInfo* request_info,
On 2010/09/20 19:45:42, Ryan Hamilton wrote:
> On 2010/09/18 01:03:26, willchan wrote:
> > We don't use |request_info| except to get the url.  How about just passing
in
> > the GURL?
> 
> Looks like we also use it for the user-agent:
> 
> std::string user_agent;
> if (request_info->extra_headers.GetHeader(HttpRequestHeaders::kUserAgent,
>                                         &user_agent))
> request_headers->SetHeader(HttpRequestHeaders::kUserAgent, user_agent);
> 
> I can change the signature so that it takes the user agent too, but I wonder
if
> that's just going to make more work for the caller?  What do you think?

Oops, I missed that.  I'm fine with either way.  My personal stylistic
preference is to be specific.  I'm a masochistic glutton for pain and
cumbersomeness though.

http://codereview.chromium.org/3432009/diff/21004/44012#newcode19
net/http/http_proxy_utils.cc:19: const HostPortPair& endpoint, std::string*
request_line,
http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre...

On 2010/09/20 19:45:42, Ryan Hamilton wrote:
> On 2010/09/18 01:03:26, willchan wrote:
> > I realize that you probably just copied this.  But I'm hitting you on nits
as
> > you move code over.  Sorry!
> > 
> > There was a chromium-dev email about this.  Please do 1 per line, unless the
> > parameters are related.
> 
> Done.  Ah!  That sounds great.  I thought that the style was 1 per line, but
I'd
> seen some instances where that was not the case.  Looks like I made the wrong
> conclusion.  (Was this a recent chromium-dev thread?  I don't think I've seen
> much traffic since I joined)

http://codereview.chromium.org/3432009/diff/21004/44017
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/3432009/diff/21004/44017#newcode177
net/spdy/spdy_proxy_client_socket.cc:177: if (spdy_stream_->closed())
On 2010/09/20 19:45:42, Ryan Hamilton wrote:
> On 2010/09/18 01:03:26, willchan wrote:
> > Er, isn't this an error if we receive a Write() when the stream is closed? 
> And
> > if you listen to my suggestion below about releasing the stream in
OnClose(),
> > then perhaps you should check for NULL instead and if it's non-NULL,
> > DCHECK(spdy_stream_->closed()).
> 
> Ok, I misunderstood the semantics here.  I thought that Write() was intended
> behave like Read(), which is to say, return 0 because 0 on EOF.  How does
> ERR_FAILED sound to you?

wtc is trying to get rid of all ERR_FAILEDs since they're too generic.  It's
deprecated (perhaps wtc should mark it as such, or file a bug, or both).

This should be a ERR_UNEXPECTED.  That indicates a programming error.  I would
combine it with a NOTREACHED().

http://codereview.chromium.org/3432009/diff/21004/44018
File net/spdy/spdy_proxy_client_socket.h (right):

http://codereview.chromium.org/3432009/diff/21004/44018#newcode44
net/spdy/spdy_proxy_client_socket.h:44:
SpdyProxyClientSocket(scoped_refptr<SpdyStream> spdy_stream,
On 2010/09/20 19:45:42, Ryan Hamilton wrote:
> On 2010/09/18 01:03:26, willchan wrote:
> > According to chromium-dev, the function prototype should take a raw pointer
> > instead.
> 
> To make sure I understand, we want the prototype to be SpdyStream*, but the
> member field can be scoped_refpr<SpdyStream> spdy_stream_? Done.
> 
> Do you have a link to the thread?  I've love to understand this issue better.

Yes, that's right.  There wasn't complete consensus on it, but that's what I
interpret the general stance to be.  Note that I personally prefer const
scoped_refptr<T>&.

http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre...

willchan no longer on Chromium

Sorry for taking so long. This is a pretty hefty changelist. There are still areas ...

10 years, 3 months ago (2010-09-21 19:20:58 UTC) #16

Sorry for taking so long.  This is a pretty hefty changelist.  There are still
areas that I just skimmed (especially the proxy-specific [auth, etc] stuff,
which I hope vandebo or mbelshe will review in more detail).

http://codereview.chromium.org/3432009/diff/10011/34006
File net/http/http_proxy_utils.cc (right):

http://codereview.chromium.org/3432009/diff/10011/34006#newcode16
net/http/http_proxy_utils.cc:16: const HttpRequestInfo* request_info,
I'm mean for nitting you for code you're moving, but whenever I see it, my OCD
tendencies come out.  If you feel so inclined, then |request_info_| should be a
const reference, not a pointer.  It helps document you don't need to NULL check
it.

http://codereview.chromium.org/3432009/diff/10011/34011
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/3432009/diff/10011/34011#newcode39
net/spdy/spdy_proxy_client_socket.cc:39: session)),
You're going to have to merge my change at some point.  You shouldn't need
|session| anymore, as we previously discussed.

http://codereview.chromium.org/3432009/diff/10011/34011#newcode82
net/spdy/spdy_proxy_client_socket.cc:82: spdy_stream_->Cancel();
Afterwards, you should also NULL the spdy_stream_ so you release it.

http://codereview.chromium.org/3432009/diff/10011/34011#newcode95
net/spdy/spdy_proxy_client_socket.cc:95: // TODO(rch): what should this
implementation be?
GOOD question.  I think the implementation will become more clear when you
integrate into http_stream_request.  I don't want to go through the mental
exercise all how you're plumbing everything through the SpdySessionPool and
ClientSocketPools.  We use this for histogram stats to learn how effective our
preconnection speculation is.

http://codereview.chromium.org/3432009/diff/10011/34011#newcode102
net/spdy/spdy_proxy_client_socket.cc:102: bool
SpdyProxyClientSocket::WasEverUsed() const {
I think this needs to return true even after Disconnect() is called.  I think
that if you ever successfully connected, then this is true, since we must have
used the transport layer socket then.

http://codereview.chromium.org/3432009/diff/10011/34011#newcode112
net/spdy/spdy_proxy_client_socket.cc:112: return 0;
This should be ERR_CONNECTION_CLOSED.  Read() should only return 0 once.  Maybe
we need to change the comment to clarify this.

http://codereview.chromium.org/3432009/diff/10011/34011#newcode166
net/spdy/spdy_proxy_client_socket.cc:166: return ERR_FAILED;
ERR_CONNECTION_CLOSED

http://codereview.chromium.org/3432009/diff/10011/34011#newcode180
net/spdy/spdy_proxy_client_socket.cc:180: rv =
spdy_stream_->WriteStreamData(iobuf, len, spdy::DATA_FLAG_NONE);
You need to handle ERR_IO_PENDING and partial reads due to SPDY per-stream flow
control.  Also, you're only returning rv.  If you iterate through this loop
multiple times, don't you need to accumulate in rv?

http://codereview.chromium.org/3432009/diff/10011/34011#newcode185
net/spdy/spdy_proxy_client_socket.cc:185: bool
SpdyProxyClientSocket::SetReceiveBufferSize(int32 size) {
We need to revisit this at some point.  This is done as a performance
optimization.  I think for Windows.  You might want to ask mbelshe about this. 
Please at least add a TODO to revisit.

http://codereview.chromium.org/3432009/diff/10011/34011#newcode193
net/spdy/spdy_proxy_client_socket.cc:193: int
SpdyProxyClientSocket::GetPeerAddress(AddressList* address) const {
I'm not sure if we document that this can only happen when IsConnected() is
true.  I think that it only makes sense for that to be the case.  Perhaps you
should return ERR_UNEXPECTED if that's not true here, and otherwise read
spdy_stream_.  And file a bug to clarify this in the interface and fix all
socket implementations to conform.

http://codereview.chromium.org/3432009/diff/10011/34011#newcode273
net/spdy/spdy_proxy_client_socket.cc:273: int
SpdyProxyClientSocket::DoSendRequest() {
I have to say, this code duplication mildly pains me.  I seems to me like it's
possible to prevent the duplication by using composition and providing delegates
to bridge the http/spdy differences.

That said, perhaps it's too much work to require here.  And hopefully this code
won't change too much.  But if it does, then keeping the two versions in sync
will suck.

http://codereview.chromium.org/3432009/diff/10011/34011#newcode456
net/spdy/spdy_proxy_client_socket.cc:456: spdy_stream_ = NULL;
It's probably appropriate to DCHECK(spdy_stream_), since you shouldn't get this
call unless you have a spdy_stream_ (afaict).

Ryan Hamilton

http://codereview.chromium.org/3432009/diff/21004/44017 File net/spdy/spdy_proxy_client_socket.cc (right): http://codereview.chromium.org/3432009/diff/21004/44017#newcode177 net/spdy/spdy_proxy_client_socket.cc:177: if (spdy_stream_->closed()) On 2010/09/21 17:55:31, willchan wrote: > On ...

10 years, 3 months ago (2010-09-21 21:20:26 UTC) #17

http://codereview.chromium.org/3432009/diff/21004/44017
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/3432009/diff/21004/44017#newcode177
net/spdy/spdy_proxy_client_socket.cc:177: if (spdy_stream_->closed())
On 2010/09/21 17:55:31, willchan wrote:
> On 2010/09/20 19:45:42, Ryan Hamilton wrote:
> > On 2010/09/18 01:03:26, willchan wrote:
> > > Er, isn't this an error if we receive a Write() when the stream is closed?

> > And
> > > if you listen to my suggestion below about releasing the stream in
> OnClose(),
> > > then perhaps you should check for NULL instead and if it's non-NULL,
> > > DCHECK(spdy_stream_->closed()).
> > 
> > Ok, I misunderstood the semantics here.  I thought that Write() was intended
> > behave like Read(), which is to say, return 0 because 0 on EOF.  How does
> > ERR_FAILED sound to you?
> 
> wtc is trying to get rid of all ERR_FAILEDs since they're too generic.  It's
> deprecated (perhaps wtc should mark it as such, or file a bug, or both).
> 
> This should be a ERR_UNEXPECTED.  That indicates a programming error.  I would
> combine it with a NOTREACHED().

Done.

http://codereview.chromium.org/3432009/diff/21004/44018
File net/spdy/spdy_proxy_client_socket.h (right):

http://codereview.chromium.org/3432009/diff/21004/44018#newcode44
net/spdy/spdy_proxy_client_socket.h:44:
SpdyProxyClientSocket(scoped_refptr<SpdyStream> spdy_stream,
On 2010/09/21 17:55:31, willchan wrote:
> On 2010/09/20 19:45:42, Ryan Hamilton wrote:
> > On 2010/09/18 01:03:26, willchan wrote:
> > > According to chromium-dev, the function prototype should take a raw
pointer
> > > instead.
> > 
> > To make sure I understand, we want the prototype to be SpdyStream*, but the
> > member field can be scoped_refpr<SpdyStream> spdy_stream_? Done.
> > 
> > Do you have a link to the thread?  I've love to understand this issue
better.
> 
> Yes, that's right.  There wasn't complete consensus on it, but that's what I
> interpret the general stance to be.  Note that I personally prefer const
> scoped_refptr<T>&.
> 
>
http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre...

Great, thanks!  This also pointed out that I was not subscribed to chromium-dev.
 I've remedied that as well.

http://codereview.chromium.org/3432009/diff/10011/34006
File net/http/http_proxy_utils.cc (right):

http://codereview.chromium.org/3432009/diff/10011/34006#newcode16
net/http/http_proxy_utils.cc:16: const HttpRequestInfo* request_info,
On 2010/09/21 19:20:58, willchan wrote:
> I'm mean for nitting you for code you're moving, but whenever I see it, my OCD
> tendencies come out.  If you feel so inclined, then |request_info_| should be
a
> const reference, not a pointer.  It helps document you don't need to NULL
check
> it.

Done.  I appreciate the OCD.  I helps keep the codebase clean :>

http://codereview.chromium.org/3432009/diff/10011/34011
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/3432009/diff/10011/34011#newcode39
net/spdy/spdy_proxy_client_socket.cc:39: session)),
On 2010/09/21 19:20:58, willchan wrote:
> You're going to have to merge my change at some point.  You shouldn't need
> |session| anymore, as we previously discussed.

I think I did this in patch set 8 when I rebased to tip of trunk.  It was very
straightforward.

http://codereview.chromium.org/3432009/diff/10011/34011#newcode82
net/spdy/spdy_proxy_client_socket.cc:82: spdy_stream_->Cancel();
On 2010/09/21 19:20:58, willchan wrote:
> Afterwards, you should also NULL the spdy_stream_ so you release it.

Interesting conundrum.  I'm not sure we need to do this.  The way I read
SpdyStream::Cancel() is that it will SpdySession::ResetStream() which will call
SpdySession::DeleteStream() which will call SpdyStream::OnClose().  OnClose
already takes care of NULLing out spdy_stream_.  Does that sound right to you?

http://codereview.chromium.org/3432009/diff/10011/34011#newcode95
net/spdy/spdy_proxy_client_socket.cc:95: // TODO(rch): what should this
implementation be?
On 2010/09/21 19:20:58, willchan wrote:
> GOOD question.  I think the implementation will become more clear when you
> integrate into http_stream_request.  I don't want to go through the mental
> exercise all how you're plumbing everything through the SpdySessionPool and
> ClientSocketPools.  We use this for histogram stats to learn how effective our
> preconnection speculation is.

*nod*  I'll leave this as a TODO for now, and once I plumb this into the
StreamRequest, I'll revisit.

http://codereview.chromium.org/3432009/diff/10011/34011#newcode102
net/spdy/spdy_proxy_client_socket.cc:102: bool
SpdyProxyClientSocket::WasEverUsed() const {
On 2010/09/21 19:20:58, willchan wrote:
> I think this needs to return true even after Disconnect() is called.  I think
> that if you ever successfully connected, then this is true, since we must have
> used the transport layer socket then.

Done.

http://codereview.chromium.org/3432009/diff/10011/34011#newcode112
net/spdy/spdy_proxy_client_socket.cc:112: return 0;
On 2010/09/21 19:20:58, willchan wrote:
> This should be ERR_CONNECTION_CLOSED.  Read() should only return 0 once. 
Maybe
> we need to change the comment to clarify this.

If Read() on a closed socket needs to return 0 the first time, and then
ERR_CONNECTION_CLOSED, then I need to keep track of a bit more state.  I've
added a has_eof_been_read_ member field (and a test).

I think we should definitely elaborate on the Socket/ClientSocket interface
comments.  I've filed crbug.com/56423.

http://codereview.chromium.org/3432009/diff/10011/34011#newcode166
net/spdy/spdy_proxy_client_socket.cc:166: return ERR_FAILED;
On 2010/09/21 19:20:58, willchan wrote:
> ERR_CONNECTION_CLOSED

Hm... From your previous comment, I thought you wanted me to do ERR_UNEXPECTED 
+ NOTREACHED?  That being said, ERR_CONNECTION_CLOSED sounds better to me, so
I've made the change (and tested it).

http://codereview.chromium.org/3432009/diff/10011/34011#newcode180
net/spdy/spdy_proxy_client_socket.cc:180: rv =
spdy_stream_->WriteStreamData(iobuf, len, spdy::DATA_FLAG_NONE);
On 2010/09/21 19:20:58, willchan wrote:
> You need to handle ERR_IO_PENDING and partial reads due to SPDY per-stream
flow
> control.  Also, you're only returning rv.  If you iterate through this loop
> multiple times, don't you need to accumulate in rv?

Did you mean partial writes, not partial reads?  Is it possible to have a
partial write in the context of SPDY?

In any case, the way I read SpdyStream::WriteStreamData(), it can *only* return
ERR_IO_PENDING (well, or ERR_INVALID_STREAM, but we shouldn't hit that path). 
How 'bout this...  I've modified SpdyProxyClientSocket::Write to always return
ERR_IO_PENDING, and added a DCHECK() that WriteStreamData returned
ERR_IO_PENDING.

http://codereview.chromium.org/3432009/diff/10011/34011#newcode185
net/spdy/spdy_proxy_client_socket.cc:185: bool
SpdyProxyClientSocket::SetReceiveBufferSize(int32 size) {
On 2010/09/21 19:20:58, willchan wrote:
> We need to revisit this at some point.  This is done as a performance
> optimization.  I think for Windows.  You might want to ask mbelshe about this.

> Please at least add a TODO to revisit.

Actually, I did ask mbelshe about this earlier and he suggested the "return
false" implementation.  I've added comments to these two methods.  Does that
work for you?  (The crux of the matter is that this ClientSocket wraps a
*shared* socket, so if a consumer of this ClientSocket attempts to change the
send/receive buffer size, they'll actually be changing life for all the other
users of the shared session, and that's probably not a good idea)

http://codereview.chromium.org/3432009/diff/10011/34011#newcode193
net/spdy/spdy_proxy_client_socket.cc:193: int
SpdyProxyClientSocket::GetPeerAddress(AddressList* address) const {
On 2010/09/21 19:20:58, willchan wrote:
> I'm not sure if we document that this can only happen when IsConnected() is
> true.  I think that it only makes sense for that to be the case.  Perhaps you
> should return ERR_UNEXPECTED if that's not true here, and otherwise read
> spdy_stream_.  And file a bug to clarify this in the interface and fix all
> socket implementations to conform.

Done.  I added a test of GetPeerAddress, and filed crbug/56426.

http://codereview.chromium.org/3432009/diff/10011/34011#newcode273
net/spdy/spdy_proxy_client_socket.cc:273: int
SpdyProxyClientSocket::DoSendRequest() {
On 2010/09/21 19:20:58, willchan wrote:
> I have to say, this code duplication mildly pains me.  I seems to me like it's
> possible to prevent the duplication by using composition and providing
delegates
> to bridge the http/spdy differences.
> 
> That said, perhaps it's too much work to require here.  And hopefully this
code
> won't change too much.  But if it does, then keeping the two versions in sync
> will suck.

It midly offends my sense of aesthetics as well.  An alternative to a delegate
would be to make SpdyProxyClientSocket and HttpProxyClientSocket both extend a
new HttpProxyClientSocketBase class which could perform the common
functionality.  How 'bout I file a bug to do this in another CL?

http://codereview.chromium.org/3432009/diff/10011/34011#newcode456
net/spdy/spdy_proxy_client_socket.cc:456: spdy_stream_ = NULL;
On 2010/09/21 19:20:58, willchan wrote:
> It's probably appropriate to DCHECK(spdy_stream_), since you shouldn't get
this
> call unless you have a spdy_stream_ (afaict).

Done.

vandebo (ex-Chrome)

I haven't looked at your unit tests yet. http://codereview.chromium.org/3432009/diff/68003/63007 File net/spdy/spdy_framer.h (right): http://codereview.chromium.org/3432009/diff/68003/63007#newcode31 net/spdy/spdy_framer.h:31: class ...

10 years, 3 months ago (2010-09-22 18:55:33 UTC) #18

Ryan Hamilton

http://codereview.chromium.org/3432009/diff/68003/63007 File net/spdy/spdy_framer.h (right): http://codereview.chromium.org/3432009/diff/68003/63007#newcode31 net/spdy/spdy_framer.h:31: class SpdyProxyClientSocketTest; On 2010/09/22 18:55:33, vandebo wrote: > Alphabetize ...

10 years, 3 months ago (2010-09-22 22:09:23 UTC) #19

http://codereview.chromium.org/3432009/diff/68003/63007
File net/spdy/spdy_framer.h (right):

http://codereview.chromium.org/3432009/diff/68003/63007#newcode31
net/spdy/spdy_framer.h:31: class SpdyProxyClientSocketTest;
On 2010/09/22 18:55:33, vandebo wrote:
> Alphabetize please

Done (in an earlier patch)

http://codereview.chromium.org/3432009/diff/68003/63007#newcode252
net/spdy/spdy_framer.h:252: friend class net::SpdyNetworkTransactionTest;
On 2010/09/22 18:55:33, vandebo wrote:
> Since there are so many, should these friends be alphabetized?

Done.

http://codereview.chromium.org/3432009/diff/68003/63009
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/3432009/diff/68003/63009#newcode27
net/spdy/spdy_proxy_client_socket.cc:27: const
scoped_refptr<HttpNetworkSession>& session)
On 2010/09/22 18:55:33, vandebo wrote:
> Because of Will's recent change, you should take auth_cache and
> http_auth_handler_factory as arguments instead of session.

Done (in patch 8)

http://codereview.chromium.org/3432009/diff/68003/63009#newcode32
net/spdy/spdy_proxy_client_socket.cc:32: user_callback_(NULL),
On 2010/09/22 18:55:33, vandebo wrote:
> user_callback_ is only used during connect.  Maybe use read_callback_ there
> instead?

Done.

http://codereview.chromium.org/3432009/diff/68003/63009#newcode73
net/spdy/spdy_proxy_client_socket.cc:73: void
SpdyProxyClientSocket::LogBlockedTunnelResponse(int response_code) const {
On 2010/09/22 18:55:33, vandebo wrote:
> I'm not sure this is needed.  Since your transport socket is over SSL you are
> safe from MITM attacks.

Ah, ok.  That sounds plausible.  So we call this method in two places...  First,
when we receive a response that is neither 200 nor 407.   The second is when
Read() is called and the state is not STATE_DONE (i.e. we are trying to read the
407 body).

In the case that the proxy returns some status other than 200 or 407, (where we
currently call this method, and return ERR_TUNNEL_CONNECTION_FAILED) what would
you recommend we do instead?   For example, if we wanted to allow the body of a
403 or 501, or whatnot, to be displayed to the user, what would the right return
value be here?

In the second case, I think you're right.  We can go ahead and allow the body of
response to be read.

http://codereview.chromium.org/3432009/diff/68003/63009#newcode94
net/spdy/spdy_proxy_client_socket.cc:94: return next_state_ == STATE_DONE &&
!spdy_stream_->is_idle();
On 2010/09/22 18:55:33, vandebo wrote:
> If IsConnected has to check for a null spdy_stream_, so does this.

Done.

http://codereview.chromium.org/3432009/diff/68003/63009#newcode106
net/spdy/spdy_proxy_client_socket.cc:106: return has_been_used_;
On 2010/09/22 18:55:33, vandebo wrote:
> This relates to the previous two methods.  All of which are for tracking
> pre-connect stats.  I suspect if you return true here, the previous two
methods
> will never be called.  Is there a way of figuring out if you have the first
spdy
> stream for a spdy session?

Well, since we're talking about client initiated streams, I believe the first
stream will be assigned stream_id 1.  I'm not sure that I understand how knowing
that we have the first stream in a session relates to this method (or the
previous), though?

http://codereview.chromium.org/3432009/diff/68003/63009#newcode122
net/spdy/spdy_proxy_client_socket.cc:122: // We're trying to read the body of
the response but we're still trying
On 2010/09/22 18:55:33, vandebo wrote:
> As I said above, you may be able to return the 407 error page to the user.

Done.

http://codereview.chromium.org/3432009/diff/68003/63009#newcode151
net/spdy/spdy_proxy_client_socket.cc:151: int bytes_read = 0;
On 2010/09/22 18:55:33, vandebo wrote:
> bytes_read isn't used.

Done.

http://codereview.chromium.org/3432009/diff/68003/63009#newcode172
net/spdy/spdy_proxy_client_socket.cc:172: if (!spdy_stream_)
On 2010/09/22 18:55:33, vandebo wrote:
> DCHECK(write_callback_ == NULL) ?

Done.

http://codereview.chromium.org/3432009/diff/68003/63009#newcode176
net/spdy/spdy_proxy_client_socket.cc:176: write_buffer_len_ = buf_len;
On 2010/09/22 18:55:33, vandebo wrote:
> Does the interface guarantees that the caller will wait for one Write to
finish
> before issuing another? As far as I see it doesn't, but maybe the interface
> comments just need to be updated.  

I think the comment needs to be updated, judging by the implementation of
SSLClientSocketNSSL::Write() which also does a DCHECK(user_write_callback_).

> Consider using a field that counts bytes outstanding instead.

Because we would rather count down from len to 0, instead of up from 0 to len? 
Done.

http://codereview.chromium.org/3432009/diff/68003/63009#newcode185
net/spdy/spdy_proxy_client_socket.cc:185: memcpy(iobuf->data(), buf->data() + i,
len);
On 2010/09/22 18:55:33, vandebo wrote:
> It's a shame that we have to copy the data.  Would it be worthwhile to create
> IOBufferSegment(IOBuffer* base, off_t offset), that would hold a reference to
> base and point data at base->data + offset ?

Good thinking.  Turns out we can effectively do that with a new
DrainableIOBuffer (which does not copy) and a call to DidConsume().  I've done
that.

http://codereview.chromium.org/3432009/diff/68003/63009#newcode187
net/spdy/spdy_proxy_client_socket.cc:187: DCHECK_EQ(rv, ERR_IO_PENDING);
On 2010/09/22 18:55:33, vandebo wrote:
> Could this return success or an error?  Probably want to return what ever the
> last one returned and bail on an error.

No, I don't believe that it can return anything other than ERR_IO_PENDING.  I
mentioned this in an earlier discussion with willchan.  (Actually, it can return
ERR_INVALID_STREAM, but only if we've done something terribly wrong on the
programming side).

SpdyStream::WriteStreamData() simply calls SpdySession::WriteSessionData()
and that appears to only return ERR_IO_PENDING, or ERR_INVALID_STREAM.  Cool?

http://codereview.chromium.org/3432009/diff/68003/63009#newcode308
net/spdy/spdy_proxy_client_socket.cc:308:
CreateSpdyHeadersFromHttpRequest(request_, headers.get(), true);
On 2010/09/22 18:55:33, vandebo wrote:
> The get() suggests that you don't really want to use a linked_ptr.

a linked_ptr is not needed in the call to CreateSpdyHeadersFromHttpRequest, but
it is used a few lines down in set_spdy_headers :(

http://codereview.chromium.org/3432009/diff/68003/63009#newcode310
net/spdy/spdy_proxy_client_socket.cc:310: (*headers)["url"] =
endpoint_.ToString();
On 2010/09/22 18:55:33, vandebo wrote:
> Would it be easier to synthesize a different request struct with the
appropriate
> information instead?

You mean a different HttpRequestInfo, right?  I tried this initially, but
couldn't figure out how to make it work.  The basic problem is that a GURL
doesn't seem to want to store host:port.  (This makes sense, because that's not
really a valid URL anywhere except as an argument to CONNECT).  It didn't seem
worth it to try to add such support to GURL.

An alternative would be to add a URL header to the HttpRequestHeader and then
modify CreateSpdyHeadersFromHttpRequest to read the value of this header.  But
since this logic would only be used by a single caller (this one) and it would
require more code than the current one line, I'm tempted to leave it alone. 
What do you think?

http://codereview.chromium.org/3432009/diff/68003/63009#newcode327
net/spdy/spdy_proxy_client_socket.cc:327: // Wait for SYN_REPLY frame from the
server
On 2010/09/22 18:55:33, vandebo wrote:
> I'm not sure it's necessary to create this effectively empty method.  It seems
> ok to just handle the progression in OnSendHeadersComplete with a comment in
the
> next and previous state methods.

Done.

http://codereview.chromium.org/3432009/diff/68003/63009#newcode366
net/spdy/spdy_proxy_client_socket.cc:366: // request.
On 2010/09/22 18:55:33, vandebo wrote:
> This should be updated, since the transport is secure.

Sounds good.  What is the right return value here?  (Not OK, not
ERR_PROXY_AUTH_REQUESTED, obviously)

http://codereview.chromium.org/3432009/diff/68003/63009#newcode379
net/spdy/spdy_proxy_client_socket.cc:379: // TODO(rch): figure out why
auth_->HandleAuthChallenge() returns
On 2010/09/22 18:55:33, vandebo wrote:
> I consider this a pre-checkin TODO, please explain if you disagree.

Totally agree.  I discussed this with cbentzel earlier today, and he pointed out
a problem in my unit test.  I've corrected it, and uploaded a new patch where
this method does the right thing.

http://codereview.chromium.org/3432009/diff/68003/63009#newcode399
net/spdy/spdy_proxy_client_socket.cc:399: OnIOComplete(status);
On 2010/09/22 18:55:33, vandebo wrote:
> I don't understand spdy internals. Should this return a success/fail status
for
> propagation to the return value.

No, I don't think so.  For two reasons.  The declaration of
SpdyStream::Delegate::OnSendHeadersComplete says

    // Called when SYN frame has been sent.
    // Returns true if no more data to be sent after SYN frame.

In looking at where it is called, the return value is only used to determine
which of the two states (HTTP Request or WebSocket) the SpdyStream is going to
go into.  We always want it to go into STATE_OPEN.

vandebo (ex-Chrome)

http://codereview.chromium.org/3432009/diff/68003/63007 File net/spdy/spdy_framer.h (right): http://codereview.chromium.org/3432009/diff/68003/63007#newcode31 net/spdy/spdy_framer.h:31: class SpdyProxyClientSocketTest; On 2010/09/22 22:09:23, Ryan Hamilton wrote: > ...

10 years, 3 months ago (2010-09-23 00:23:36 UTC) #20

http://codereview.chromium.org/3432009/diff/68003/63007
File net/spdy/spdy_framer.h (right):

http://codereview.chromium.org/3432009/diff/68003/63007#newcode31
net/spdy/spdy_framer.h:31: class SpdyProxyClientSocketTest;
On 2010/09/22 22:09:23, Ryan Hamilton wrote:
> On 2010/09/22 18:55:33, vandebo wrote:
> > Alphabetize please
> 
> Done (in an earlier patch)

H < N < P

http://codereview.chromium.org/3432009/diff/68003/63007#newcode252
net/spdy/spdy_framer.h:252: friend class net::SpdyNetworkTransactionTest;
On 2010/09/22 22:09:23, Ryan Hamilton wrote:
> On 2010/09/22 18:55:33, vandebo wrote:
> > Since there are so many, should these friends be alphabetized?
> 
> Done.

You got it right here :-)

http://codereview.chromium.org/3432009/diff/68003/63009
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/3432009/diff/68003/63009#newcode27
net/spdy/spdy_proxy_client_socket.cc:27: const
scoped_refptr<HttpNetworkSession>& session)
On 2010/09/22 22:09:23, Ryan Hamilton wrote:
> On 2010/09/22 18:55:33, vandebo wrote:
> > Because of Will's recent change, you should take auth_cache and
> > http_auth_handler_factory as arguments instead of session.
> 
> Done (in patch 8)

Nope, the argument still says "const scoped_refptr<HttpNetworkSession>&
session)"

http://codereview.chromium.org/3432009/diff/68003/63009#newcode73
net/spdy/spdy_proxy_client_socket.cc:73: void
SpdyProxyClientSocket::LogBlockedTunnelResponse(int response_code) const {
On 2010/09/22 22:09:23, Ryan Hamilton wrote:
> On 2010/09/22 18:55:33, vandebo wrote:
> > I'm not sure this is needed.  Since your transport socket is over SSL you
are
> > safe from MITM attacks.
> 
> Ah, ok.  That sounds plausible.  So we call this method in two places... 
First,
> when we receive a response that is neither 200 nor 407.   The second is when
> Read() is called and the state is not STATE_DONE (i.e. we are trying to read
the
> 407 body).
> 
> In the case that the proxy returns some status other than 200 or 407, (where
we
> currently call this method, and return ERR_TUNNEL_CONNECTION_FAILED) what
would
> you recommend we do instead?   For example, if we wanted to allow the body of
a
> 403 or 501, or whatnot, to be displayed to the user, what would the right
return
> value be here?

It's probably cleanest to add an error code.  OK will make
HttpNetworkTransaction continue and try to send a request, an error will cause
the higher level to display the error page instead of the content.  But I'm not
entirely clear on this point.

> In the second case, I think you're right.  We can go ahead and allow the body
of
> response to be read.

This check is effectively a safeguard that the caller doesn't display the error
page in spite of getting ERR_TUNNEL_CONNECTION_FAILED.

http://codereview.chromium.org/3432009/diff/68003/63009#newcode106
net/spdy/spdy_proxy_client_socket.cc:106: return has_been_used_;
On 2010/09/22 22:09:23, Ryan Hamilton wrote:
> On 2010/09/22 18:55:33, vandebo wrote:
> > This relates to the previous two methods.  All of which are for tracking
> > pre-connect stats.  I suspect if you return true here, the previous two
> methods
> > will never be called.  Is there a way of figuring out if you have the first
> spdy
> > stream for a spdy session?
> 
> Well, since we're talking about client initiated streams, I believe the first
> stream will be assigned stream_id 1.  I'm not sure that I understand how
knowing
> that we have the first stream in a session relates to this method (or the
> previous), though?

Iff the stream_id is 1, then the stream was never used before.

http://codereview.chromium.org/3432009/diff/68003/63009#newcode185
net/spdy/spdy_proxy_client_socket.cc:185: memcpy(iobuf->data(), buf->data() + i,
len);
On 2010/09/22 22:09:23, Ryan Hamilton wrote:
> On 2010/09/22 18:55:33, vandebo wrote:
> > It's a shame that we have to copy the data.  Would it be worthwhile to
create
> > IOBufferSegment(IOBuffer* base, off_t offset), that would hold a reference
to
> > base and point data at base->data + offset ?
> 
> Good thinking.  Turns out we can effectively do that with a new
> DrainableIOBuffer (which does not copy) and a call to DidConsume().  I've done
> that.

Cool, I didn't consider that.

http://codereview.chromium.org/3432009/diff/68003/63009#newcode187
net/spdy/spdy_proxy_client_socket.cc:187: DCHECK_EQ(rv, ERR_IO_PENDING);
On 2010/09/22 22:09:23, Ryan Hamilton wrote:
> On 2010/09/22 18:55:33, vandebo wrote:
> > Could this return success or an error?  Probably want to return what ever
the
> > last one returned and bail on an error.
> 
> No, I don't believe that it can return anything other than ERR_IO_PENDING.  I
> mentioned this in an earlier discussion with willchan.  (Actually, it can
return
> ERR_INVALID_STREAM, but only if we've done something terribly wrong on the
> programming side).
> 
> SpdyStream::WriteStreamData() simply calls SpdySession::WriteSessionData()
> and that appears to only return ERR_IO_PENDING, or ERR_INVALID_STREAM.  Cool?

It's brittle to depend on behavior like this in a lower level.  It'd be better
to spend the extra few lines to accumulate the return value: stop on error and
return it, otherwise just return the last value seen.

http://codereview.chromium.org/3432009/diff/13007/37010#newcode120
net/spdy/spdy_proxy_client_socket.cc:120: 
Why can't we read a 500 for example?  I think the only thing special we need to
do is that if we don't get a 200, the 'connection'(stream) shouldn't be reused.

http://codereview.chromium.org/3432009/diff/13007/37010#newcode174
net/spdy/spdy_proxy_client_socket.cc:174: 
SetOffset (does the same thing in this case) might make better logical sense.

http://codereview.chromium.org/3432009/diff/13007/37010#newcode199
net/spdy/spdy_proxy_client_socket.cc:199: // Since this ClientSocket sits on top
of a shared SpdySession, it
This is only used once and is specific to the connet/read case.

http://codereview.chromium.org/3432009/diff/13007/37011
File net/spdy/spdy_proxy_client_socket.h (right):

http://codereview.chromium.org/3432009/diff/13007/37011#newcode133
net/spdy/spdy_proxy_client_socket.h:133: int DoReadReply();
Extra now.

Ryan Hamilton

http://codereview.chromium.org/3432009/diff/68003/63007 File net/spdy/spdy_framer.h (right): http://codereview.chromium.org/3432009/diff/68003/63007#newcode31 net/spdy/spdy_framer.h:31: class SpdyProxyClientSocketTest; On 2010/09/23 00:23:36, vandebo wrote: > On ...

10 years, 3 months ago (2010-09-23 04:40:46 UTC) #21

http://codereview.chromium.org/3432009/diff/68003/63007
File net/spdy/spdy_framer.h (right):

http://codereview.chromium.org/3432009/diff/68003/63007#newcode31
net/spdy/spdy_framer.h:31: class SpdyProxyClientSocketTest;
On 2010/09/23 00:23:36, vandebo wrote:
> On 2010/09/22 22:09:23, Ryan Hamilton wrote:
> > On 2010/09/22 18:55:33, vandebo wrote:
> > > Alphabetize please
> > 
> > Done (in an earlier patch)
> 
> H < N < P
> 

Wow, that's embarrassing.  Done.

http://codereview.chromium.org/3432009/diff/68003/63009
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/3432009/diff/68003/63009#newcode27
net/spdy/spdy_proxy_client_socket.cc:27: const
scoped_refptr<HttpNetworkSession>& session)
On 2010/09/23 00:23:36, vandebo wrote:
> On 2010/09/22 22:09:23, Ryan Hamilton wrote:
> > On 2010/09/22 18:55:33, vandebo wrote:
> > > Because of Will's recent change, you should take auth_cache and
> > > http_auth_handler_factory as arguments instead of session.
> > 
> > Done (in patch 8)
> 
> Nope, the argument still says "const scoped_refptr<HttpNetworkSession>&
> session)"

Gah!  Sorry, I misread your initial comment as "need to" not "should" and since
I rebased after his commit and it still compiled, I thought I did what you
suggested.  Done.

http://codereview.chromium.org/3432009/diff/68003/63009#newcode73
net/spdy/spdy_proxy_client_socket.cc:73: void
SpdyProxyClientSocket::LogBlockedTunnelResponse(int response_code) const {
On 2010/09/23 00:23:36, vandebo wrote:
> On 2010/09/22 22:09:23, Ryan Hamilton wrote:
> > On 2010/09/22 18:55:33, vandebo wrote:
> > > I'm not sure this is needed.  Since your transport socket is over SSL you
> are
> > > safe from MITM attacks.
> > 
> > Ah, ok.  That sounds plausible.  So we call this method in two places... 
> First,
> > when we receive a response that is neither 200 nor 407.   The second is when
> > Read() is called and the state is not STATE_DONE (i.e. we are trying to read
> the
> > 407 body).
> > 
> > In the case that the proxy returns some status other than 200 or 407, (where
> we
> > currently call this method, and return ERR_TUNNEL_CONNECTION_FAILED) what
> would
> > you recommend we do instead?   For example, if we wanted to allow the body
of
> a
> > 403 or 501, or whatnot, to be displayed to the user, what would the right
> return
> > value be here?
> 
> It's probably cleanest to add an error code.  OK will make
> HttpNetworkTransaction continue and try to send a request, an error will cause
> the higher level to display the error page instead of the content.  But I'm
not
> entirely clear on this point.
> 
> > In the second case, I think you're right.  We can go ahead and allow the
body
> of
> > response to be read.

In that case, it sounds like ERR_TUNNEL_CONNECTION_FAILED is the right error to
use.  Does that work for you?

> This check is effectively a safeguard that the caller doesn't display the
error
> page in spite of getting ERR_TUNNEL_CONNECTION_FAILED.

Which check?

http://codereview.chromium.org/3432009/diff/68003/63009#newcode106
net/spdy/spdy_proxy_client_socket.cc:106: return has_been_used_;
On 2010/09/23 00:23:36, vandebo wrote:
> On 2010/09/22 22:09:23, Ryan Hamilton wrote:
> > On 2010/09/22 18:55:33, vandebo wrote:
> > > This relates to the previous two methods.  All of which are for tracking
> > > pre-connect stats.  I suspect if you return true here, the previous two
> > methods
> > > will never be called.  Is there a way of figuring out if you have the
first
> > spdy
> > > stream for a spdy session?
> > 
> > Well, since we're talking about client initiated streams, I believe the
first
> > stream will be assigned stream_id 1.  I'm not sure that I understand how
> knowing
> > that we have the first stream in a session relates to this method (or the
> > previous), though?
> 
> Iff the stream_id is 1, then the stream was never used before.

I don't think I'm following you.  Do you mean, if the stream_id is 1, then the
*session* was never used?  Is it possible that a SETTINGS frame might have been
sent before the first stream was created?

How 'bout if we simply delegate to the SpdySession, and have the SpdySession
delegate to the transport socket?  I've updated the patch to do this.

http://codereview.chromium.org/3432009/diff/68003/63009#newcode187
net/spdy/spdy_proxy_client_socket.cc:187: DCHECK_EQ(rv, ERR_IO_PENDING);
On 2010/09/23 00:23:36, vandebo wrote:
> On 2010/09/22 22:09:23, Ryan Hamilton wrote:
> > On 2010/09/22 18:55:33, vandebo wrote:
> > > Could this return success or an error?  Probably want to return what ever
> the
> > > last one returned and bail on an error.
> > 
> > No, I don't believe that it can return anything other than ERR_IO_PENDING. 
I
> > mentioned this in an earlier discussion with willchan.  (Actually, it can
> return
> > ERR_INVALID_STREAM, but only if we've done something terribly wrong on the
> > programming side).
> > 
> > SpdyStream::WriteStreamData() simply calls SpdySession::WriteSessionData()
> > and that appears to only return ERR_IO_PENDING, or ERR_INVALID_STREAM. 
Cool?
> 
> It's brittle to depend on behavior like this in a lower level.  It'd be better
> to spend the extra few lines to accumulate the return value: stop on error and
> return it, otherwise just return the last value seen.

I appreciate your point.  Accumulating rv is a bit more complicated, I think. 
I've updated the patch to do the following.
If WriteStreamData returns a positive number, then decrement the outstanding
bytes by that value.  Otherwise, if it's not ERR_IO_PENDING, return that value.

When finished, with the loop, if there are still bytes outstanding, then return
ERR_IO_PENDING, otherwise return the number of bytes written.

How does that sound?

http://codereview.chromium.org/3432009/diff/13007/37010#newcode120
net/spdy/spdy_proxy_client_socket.cc:120: 
On 2010/09/23 00:23:36, vandebo wrote:
> Why can't we read a 500 for example? 

Fair enough.  Done.

> I think the only thing special we need to
> do is that if we don't get a 200, the 'connection'(stream) shouldn't be
reused.

I *think* that will be true since Connect will only return OK in the 200 case. 
Do you concur?

http://codereview.chromium.org/3432009/diff/13007/37010#newcode174
net/spdy/spdy_proxy_client_socket.cc:174: 
On 2010/09/23 00:23:36, vandebo wrote:
> SetOffset (does the same thing in this case) might make better logical sense.

Done.

http://codereview.chromium.org/3432009/diff/13007/37010#newcode199
net/spdy/spdy_proxy_client_socket.cc:199: // Since this ClientSocket sits on top
of a shared SpdySession, it
On 2010/09/23 00:23:36, vandebo wrote:
> This is only used once and is specific to the connet/read case.

Well, it's invoked by DoLoop, and passed in to auth_->MaybeGenerateAuthToken(). 
Does this seems suboptimal to you?  I thought this was the standard event loop
idiom that is used, for example, in the HttpProxyClientSocket, but if there is
something better, I will happily use that.

http://codereview.chromium.org/3432009/diff/13007/37011
File net/spdy/spdy_proxy_client_socket.h (right):

http://codereview.chromium.org/3432009/diff/13007/37011#newcode133
net/spdy/spdy_proxy_client_socket.h:133: int DoReadReply();
On 2010/09/23 00:23:36, vandebo wrote:
> Extra now.

Good point.  Why is it not a compile error to have a method declared
non-virtual, but not defined?

willchan no longer on Chromium

Still haven't looked at your test yet. Sorry, I suck. http://codereview.chromium.org/3432009/diff/10011/34011 File net/spdy/spdy_proxy_client_socket.cc (right): http://codereview.chromium.org/3432009/diff/10011/34011#newcode82 ...

10 years, 3 months ago (2010-09-23 23:20:30 UTC) #22

Still haven't looked at your test yet.  Sorry, I suck.

http://codereview.chromium.org/3432009/diff/10011/34011
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/3432009/diff/10011/34011#newcode82
net/spdy/spdy_proxy_client_socket.cc:82: spdy_stream_->Cancel();
On 2010/09/21 21:20:26, Ryan Hamilton wrote:
> On 2010/09/21 19:20:58, willchan wrote:
> > Afterwards, you should also NULL the spdy_stream_ so you release it.
> 
> Interesting conundrum.  I'm not sure we need to do this.  The way I read
> SpdyStream::Cancel() is that it will SpdySession::ResetStream() which will
call
> SpdySession::DeleteStream() which will call SpdyStream::OnClose().  OnClose
> already takes care of NULLing out spdy_stream_.  Does that sound right to you?

I see.  Ok, sounds good.  Might need a comment.

http://codereview.chromium.org/3432009/diff/10011/34011#newcode180
net/spdy/spdy_proxy_client_socket.cc:180: rv =
spdy_stream_->WriteStreamData(iobuf, len, spdy::DATA_FLAG_NONE);
On 2010/09/21 21:20:26, Ryan Hamilton wrote:
> On 2010/09/21 19:20:58, willchan wrote:
> > You need to handle ERR_IO_PENDING and partial reads due to SPDY per-stream
> flow
> > control.  Also, you're only returning rv.  If you iterate through this loop
> > multiple times, don't you need to accumulate in rv?
> 
> Did you mean partial writes, not partial reads?  Is it possible to have a
> partial write in the context of SPDY?
> 
> In any case, the way I read SpdyStream::WriteStreamData(), it can *only*
return
> ERR_IO_PENDING (well, or ERR_INVALID_STREAM, but we shouldn't hit that path). 
> How 'bout this...  I've modified SpdyProxyClientSocket::Write to always return
> ERR_IO_PENDING, and added a DCHECK() that WriteStreamData returned
> ERR_IO_PENDING.
> 

I prefer not to make undocumented assumptions.  This isn't something that is
promised in the comments.  The new code that you've written is acceptable
though.

http://codereview.chromium.org/3432009/diff/10011/34011#newcode185
net/spdy/spdy_proxy_client_socket.cc:185: bool
SpdyProxyClientSocket::SetReceiveBufferSize(int32 size) {
On 2010/09/21 21:20:26, Ryan Hamilton wrote:
> On 2010/09/21 19:20:58, willchan wrote:
> > We need to revisit this at some point.  This is done as a performance
> > optimization.  I think for Windows.  You might want to ask mbelshe about
this.
> 
> > Please at least add a TODO to revisit.
> 
> Actually, I did ask mbelshe about this earlier and he suggested the "return
> false" implementation.  I've added comments to these two methods.  Does that
> work for you?  (The crux of the matter is that this ClientSocket wraps a
> *shared* socket, so if a consumer of this ClientSocket attempts to change the
> send/receive buffer size, they'll actually be changing life for all the other
> users of the shared session, and that's probably not a good idea)

Ok.

http://codereview.chromium.org/3432009/diff/10011/34011#newcode273
net/spdy/spdy_proxy_client_socket.cc:273: int
SpdyProxyClientSocket::DoSendRequest() {
On 2010/09/21 21:20:26, Ryan Hamilton wrote:
> On 2010/09/21 19:20:58, willchan wrote:
> > I have to say, this code duplication mildly pains me.  I seems to me like
it's
> > possible to prevent the duplication by using composition and providing
> delegates
> > to bridge the http/spdy differences.
> > 
> > That said, perhaps it's too much work to require here.  And hopefully this
> code
> > won't change too much.  But if it does, then keeping the two versions in
sync
> > will suck.
> 
> It midly offends my sense of aesthetics as well.  An alternative to a delegate
> would be to make SpdyProxyClientSocket and HttpProxyClientSocket both extend a
> new HttpProxyClientSocketBase class which could perform the common
> functionality.  How 'bout I file a bug to do this in another CL?

Sure, although I don't know that I'd agree with implementation inheritance here.
 Perhaps composition.

http://codereview.chromium.org/3432009/diff/33004/13010
File net/http/http_proxy_utils.h (right):

http://codereview.chromium.org/3432009/diff/33004/13010#newcode27
net/http/http_proxy_utils.h:27: 
Nit: extra unnecessary newline.

http://codereview.chromium.org/3432009/diff/33004/13014
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/3432009/diff/33004/13014#newcode177
net/spdy/spdy_proxy_client_socket.cc:177: LOG(INFO) << "write failed, rv: " <<
rv;
You should get rid of this LOG.

http://codereview.chromium.org/3432009/diff/33004/13014#newcode353
net/spdy/spdy_proxy_client_socket.cc:353:
LogBlockedTunnelResponse(response_.headers->response_code());
I feel like we should be writing this out to the net_log_.  I think
HttpProxyClientSocket should be doing this too.  This will be helpful for you to
test and to debug user bug reports once people start reporting this type of
error.

http://codereview.chromium.org/3432009/diff/33004/13014#newcode386
net/spdy/spdy_proxy_client_socket.cc:386: NOTIMPLEMENTED();
No need for NOTIMPLEMENTED().  We used that mainly when porting, to indicate
things we still had to implement.  This should be a NOTREACHED().

http://codereview.chromium.org/3432009/diff/33004/13014#newcode404
net/spdy/spdy_proxy_client_socket.cc:404: DCHECK(next_state_ =
STATE_READ_REPLY_COMPLETE);
Is this a bug?  Should it be DCHECK_EQ()?

vandebo (ex-Chrome)

http://codereview.chromium.org/3432009/diff/68003/63009 File net/spdy/spdy_proxy_client_socket.cc (right): http://codereview.chromium.org/3432009/diff/68003/63009#newcode73 net/spdy/spdy_proxy_client_socket.cc:73: void SpdyProxyClientSocket::LogBlockedTunnelResponse(int response_code) const { On 2010/09/23 04:40:58, Ryan ...

10 years, 3 months ago (2010-09-24 00:58:36 UTC) #23

http://codereview.chromium.org/3432009/diff/68003/63009
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/3432009/diff/68003/63009#newcode73
net/spdy/spdy_proxy_client_socket.cc:73: void
SpdyProxyClientSocket::LogBlockedTunnelResponse(int response_code) const {
On 2010/09/23 04:40:58, Ryan Hamilton wrote:
> On 2010/09/23 00:23:36, vandebo wrote:
> > This check is effectively a safeguard that the caller doesn't display the
> error
> > page in spite of getting ERR_TUNNEL_CONNECTION_FAILED.
> 
> Which check?

The one in Read().  You took care of it.

http://codereview.chromium.org/3432009/diff/13007/37010#newcode120
net/spdy/spdy_proxy_client_socket.cc:120: 
On 2010/09/23 04:40:58, Ryan Hamilton wrote:
> On 2010/09/23 00:23:36, vandebo wrote:
> > Why can't we read a 500 for example? 
> 
> Fair enough.  Done.
> 
> > I think the only thing special we need to
> > do is that if we don't get a 200, the 'connection'(stream) shouldn't be
> reused.
> 
> I *think* that will be true since Connect will only return OK in the 200 case.

> Do you concur?

I'm not sure... the none 407 http error case is still fuzzy to me.

http://codereview.chromium.org/3432009/diff/13007/37010#newcode199
net/spdy/spdy_proxy_client_socket.cc:199: // Since this ClientSocket sits on top
of a shared SpdySession, it
On 2010/09/23 04:40:58, Ryan Hamilton wrote:
> On 2010/09/23 00:23:36, vandebo wrote:
> > This is only used once and is specific to the connet/read case.
> 
> Well, it's invoked by DoLoop, and passed in to
auth_->MaybeGenerateAuthToken(). 
> Does this seems suboptimal to you?  I thought this was the standard event loop
> idiom that is used, for example, in the HttpProxyClientSocket, but if there is
> something better, I will happily use that.

You have both a read and write callback.  You've integrated the WriteDoCallback
code into OnDataSent, which seems reasonable.  Since this is the only used from
OnIOComplete, I was suggesting integrating it there.

http://codereview.chromium.org/3432009/diff/13007/37011
File net/spdy/spdy_proxy_client_socket.h (right):

http://codereview.chromium.org/3432009/diff/13007/37011#newcode133
net/spdy/spdy_proxy_client_socket.h:133: int DoReadReply();
On 2010/09/23 04:40:58, Ryan Hamilton wrote:
> On 2010/09/23 00:23:36, vandebo wrote:
> > Extra now.
> 
> Good point.  Why is it not a compile error to have a method declared
> non-virtual, but not defined?

I think it's because it could just be in a different compilation unit (i.e. a
different file).  The only place it comes all together in C/C++ is the linker. 
At that point, if no one is using a declared method, why complain that it
doesn't exist?

http://codereview.chromium.org/3432009/diff/33004/13014
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/3432009/diff/33004/13014#newcode14
net/spdy/spdy_proxy_client_socket.cc:14: #include
"net/http/http_auth_handler_factory.h"
Need net/http/http_auth_cache.h

http://codereview.chromium.org/3432009/diff/33004/13014#newcode16
net/spdy/spdy_proxy_client_socket.cc:16: #include
"net/http/http_network_session.h"
Don't need this anymore.

http://codereview.chromium.org/3432009/diff/33004/13014#newcode101
net/spdy/spdy_proxy_client_socket.cc:101: // TODO(rch): what should this
implementation be?
I guess these should pass through the the spdy_stream transport socket as well. 
jar@ can tell you for sure what he'd like done.

http://codereview.chromium.org/3432009/diff/33004/13014#newcode105
net/spdy/spdy_proxy_client_socket.cc:105: return IsConnected() &&
spdy_stream_->WasEverUsed();
I'm not sure IsConnected() && will do the right thing... What happens when
you've Connected and Disconnected?

http://codereview.chromium.org/3432009/diff/33004/13014#newcode120
net/spdy/spdy_proxy_client_socket.cc:120: DCHECK(next_state_ == STATE_DONE);
I think this DCHECK is false if we hit an http error.

http://codereview.chromium.org/3432009/diff/33004/13014#newcode164
net/spdy/spdy_proxy_client_socket.cc:164: return
spdy_stream_->WriteStreamData(buf, buf_len, spdy::DATA_FLAG_NONE);
This code assumes that WriteSteamData will return ERR_IO_PENDING since it sets
write_callback_ unconditionally.

http://codereview.chromium.org/3432009/diff/33004/13014#newcode182
net/spdy/spdy_proxy_client_socket.cc:182: LOG(INFO) << "io pending";
And this one

http://codereview.chromium.org/3432009/diff/33004/13014#newcode187
net/spdy/spdy_proxy_client_socket.cc:187: LOG(INFO) << "done";
And this one

http://codereview.chromium.org/3432009/diff/33004/13014#newcode343
net/spdy/spdy_proxy_client_socket.cc:343: next_state_ = STATE_DONE;
We're not done here - we may need to restart with auth.

http://codereview.chromium.org/3432009/diff/33004/13014#newcode347
net/spdy/spdy_proxy_client_socket.cc:347: // For all other status codes, we
conservatively fail the CONNECT
This doesn't apply in this case, since we're not vulnerable to a MITM.  You
should return an error such that the high level (HttpNetworkTransaction) doesn't
try to send a request on the connection, but returns success so that the level
above (HttpUrlRequestJob, etc.) will request and render the proxy error page.

http://codereview.chromium.org/3432009/diff/33004/13015
File net/spdy/spdy_proxy_client_socket.h (right):

http://codereview.chromium.org/3432009/diff/33004/13015#newcode62
net/spdy/spdy_proxy_client_socket.h:62: }
Where's the RestartWithAuth type method?

http://codereview.chromium.org/3432009/diff/33004/13016
File net/spdy/spdy_proxy_client_socket_unittest.cc (right):

http://codereview.chromium.org/3432009/diff/33004/13016#newcode85
net/spdy/spdy_proxy_client_socket_unittest.cc:85: HttpNetworkSession* session_;
Some of these should probably be private, no?

http://codereview.chromium.org/3432009/diff/33004/13016#newcode128
net/spdy/spdy_proxy_client_socket_unittest.cc:128: void
SpdyProxyClientSocketTest::SetUp() {
You don't need this, it will already do what you've written.

http://codereview.chromium.org/3432009/diff/33004/13016#newcode133
net/spdy/spdy_proxy_client_socket_unittest.cc:133: if (session_ != NULL)
Do you really want this in TearDown() instead of the destructor?

http://codereview.chromium.org/3432009/diff/33004/13016#newcode162
net/spdy/spdy_proxy_client_socket_unittest.cc:162: EXPECT_EQ(OK,
spdy_session_->Connect(kOriginHostPort, tcp_params_, LOWEST));
Do you want this to be ASSERT?  The rest of the test will fail if this does,
right?

http://codereview.chromium.org/3432009/diff/33004/13016#newcode190
net/spdy/spdy_proxy_client_socket_unittest.cc:190: void
SpdyProxyClientSocketTest::CheckConnectionEstablished() {
Should these be named AssertFoo or something since they assert, whereas
CheckConnetSucceeds, for example, expects?

http://codereview.chromium.org/3432009/diff/33004/13016#newcode211
net/spdy/spdy_proxy_client_socket_unittest.cc:211: // Dummy write to un-block
the read
Writing to read could have side effects that makes your test work in unit tests
but not in the real world.  Would a mock object work better?  Or some kind of
Spdy data provider?

http://codereview.chromium.org/3432009/diff/33004/13016#newcode215
net/spdy/spdy_proxy_client_socket_unittest.cc:215:
EXPECT_TRUE(sock_->IsConnected());
redundant

http://codereview.chromium.org/3432009/diff/33004/13016#newcode338
net/spdy/spdy_proxy_client_socket_unittest.cc:338: scoped_ptr<spdy::SpdyFrame>
conn(ConstructConnectRequestFrame());
You could make this take a scoped_ptr<spdy::SpdyFrame>* and return a MockWrite.

http://codereview.chromium.org/3432009/diff/33004/13016#newcode350
net/spdy/spdy_proxy_client_socket_unittest.cc:350: 
To check that your framework works properly, you might want to check that you're
not connected here.

http://codereview.chromium.org/3432009/diff/33004/13016#newcode393
net/spdy/spdy_proxy_client_socket_unittest.cc:393: AddAuthToCache();
Probably also want a test case that gets a 407, and then tries again with the
correct auth.

http://codereview.chromium.org/3432009/diff/33004/13016#newcode401
net/spdy/spdy_proxy_client_socket_unittest.cc:401:
TEST_F(SpdyProxyClientSocketTest, ConnectWaitsForResponse) {
How is this different than the connect test?

http://codereview.chromium.org/3432009/diff/33004/13016#newcode539
net/spdy/spdy_proxy_client_socket_unittest.cc:539:
EXPECT_EQ(ERR_PROXY_AUTH_REQUESTED, callback_.WaitForResult());
I don't think this is how higher level network code will interact with this
code.  This suggests that you want an integration test as well.  Maybe an
HttpNetworkTransaction test.

http://codereview.chromium.org/3432009/diff/33004/13016#newcode647
net/spdy/spdy_proxy_client_socket_unittest.cc:647: CheckSyncReadEquals(kMsg33,
kLen33);
I don't see a test that reads part of the reply data and then reads more/the
rest with a subsequent read.

http://codereview.chromium.org/3432009/diff/33004/13016#newcode785
net/spdy/spdy_proxy_client_socket_unittest.cc:785: }  // namespace net
There are probably a few more failure tests you could add. i.e. failing to
connect, etc.

Ryan Hamilton

http://codereview.chromium.org/3432009/diff/10011/34011 File net/spdy/spdy_proxy_client_socket.cc (right): http://codereview.chromium.org/3432009/diff/10011/34011#newcode82 net/spdy/spdy_proxy_client_socket.cc:82: spdy_stream_->Cancel(); On 2010/09/23 23:20:30, willchan wrote: > On 2010/09/21 ...

10 years, 3 months ago (2010-09-24 18:25:57 UTC) #24

http://codereview.chromium.org/3432009/diff/10011/34011
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/3432009/diff/10011/34011#newcode82
net/spdy/spdy_proxy_client_socket.cc:82: spdy_stream_->Cancel();
On 2010/09/23 23:20:30, willchan wrote:
> On 2010/09/21 21:20:26, Ryan Hamilton wrote:
> > On 2010/09/21 19:20:58, willchan wrote:
> > > Afterwards, you should also NULL the spdy_stream_ so you release it.
> > 
> > Interesting conundrum.  I'm not sure we need to do this.  The way I read
> > SpdyStream::Cancel() is that it will SpdySession::ResetStream() which will
> call
> > SpdySession::DeleteStream() which will call SpdyStream::OnClose().  OnClose
> > already takes care of NULLing out spdy_stream_.  Does that sound right to
you?
> 
> I see.  Ok, sounds good.  Might need a comment.

Done.

http://codereview.chromium.org/3432009/diff/10011/34011#newcode273
net/spdy/spdy_proxy_client_socket.cc:273: int
SpdyProxyClientSocket::DoSendRequest() {
On 2010/09/23 23:20:30, willchan wrote:
> On 2010/09/21 21:20:26, Ryan Hamilton wrote:
> > On 2010/09/21 19:20:58, willchan wrote:
> > > I have to say, this code duplication mildly pains me.  I seems to me like
> it's
> > > possible to prevent the duplication by using composition and providing
> > delegates
> > > to bridge the http/spdy differences.
> > > 
> > > That said, perhaps it's too much work to require here.  And hopefully this
> > code
> > > won't change too much.  But if it does, then keeping the two versions in
> sync
> > > will suck.
> > 
> > It midly offends my sense of aesthetics as well.  An alternative to a
delegate
> > would be to make SpdyProxyClientSocket and HttpProxyClientSocket both extend
a
> > new HttpProxyClientSocketBase class which could perform the common
> > functionality.  How 'bout I file a bug to do this in another CL?
> 
> Sure, although I don't know that I'd agree with implementation inheritance
here.
>  Perhaps composition.

*nod*  I haven't thought about it too deeply.  Composition could be a better
choice...

http://codereview.chromium.org/3432009/diff/13007/37010#newcode120
net/spdy/spdy_proxy_client_socket.cc:120: // We reach this case when the user
cancels a 407 proxy auth prompt.
On 2010/09/24 00:58:36, vandebo wrote:
> On 2010/09/23 04:40:58, Ryan Hamilton wrote:
> > On 2010/09/23 00:23:36, vandebo wrote:
> > > Why can't we read a 500 for example? 
> > 
> > Fair enough.  Done.
> > 
> > > I think the only thing special we need to
> > > do is that if we don't get a 200, the 'connection'(stream) shouldn't be
> > reused.
> > 
> > I *think* that will be true since Connect will only return OK in the 200
case.
> 
> > Do you concur?
> 
> I'm not sure... the none 407 http error case is still fuzzy to me.

*nod*  I hope the the implementation I describe in a later comment works for
you...

http://codereview.chromium.org/3432009/diff/13007/37010#newcode199
net/spdy/spdy_proxy_client_socket.cc:199: DCHECK(user_callback_);
On 2010/09/24 00:58:36, vandebo wrote:
> On 2010/09/23 04:40:58, Ryan Hamilton wrote:
> > On 2010/09/23 00:23:36, vandebo wrote:
> > > This is only used once and is specific to the connet/read case.
> > 
> > Well, it's invoked by DoLoop, and passed in to
> auth_->MaybeGenerateAuthToken(). 
> > Does this seems suboptimal to you?  I thought this was the standard event
loop
> > idiom that is used, for example, in the HttpProxyClientSocket, but if there
is
> > something better, I will happily use that.
> 
> You have both a read and write callback.  You've integrated the
WriteDoCallback
> code into OnDataSent, which seems reasonable.  Since this is the only used
from
> OnIOComplete, I was suggesting integrating it there.

Done.

http://codereview.chromium.org/3432009/diff/13007/37011
File net/spdy/spdy_proxy_client_socket.h (right):

http://codereview.chromium.org/3432009/diff/13007/37011#newcode133
net/spdy/spdy_proxy_client_socket.h:133: int DoReadReply();
On 2010/09/24 00:58:36, vandebo wrote:
> On 2010/09/23 04:40:58, Ryan Hamilton wrote:
> > On 2010/09/23 00:23:36, vandebo wrote:
> > > Extra now.
> > 
> > Good point.  Why is it not a compile error to have a method declared
> > non-virtual, but not defined?
> 
> I think it's because it could just be in a different compilation unit (i.e. a
> different file).  The only place it comes all together in C/C++ is the linker.

> At that point, if no one is using a declared method, why complain that it
> doesn't exist?

Right! Of course!  You can defined the method in a different compilation unit! 
Once more C++ != Java :>  Thanks!

http://codereview.chromium.org/3432009/diff/33004/13010
File net/http/http_proxy_utils.h (right):

http://codereview.chromium.org/3432009/diff/33004/13010#newcode27
net/http/http_proxy_utils.h:27: 
On 2010/09/23 23:20:31, willchan wrote:
> Nit: extra unnecessary newline.

Done.

http://codereview.chromium.org/3432009/diff/33004/13014
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/3432009/diff/33004/13014#newcode14
net/spdy/spdy_proxy_client_socket.cc:14: #include
"net/http/http_auth_handler_factory.h"
On 2010/09/24 00:58:37, vandebo wrote:
> Need net/http/http_auth_cache.h

Done.

http://codereview.chromium.org/3432009/diff/33004/13014#newcode16
net/spdy/spdy_proxy_client_socket.cc:16: #include
"net/http/http_network_session.h"
On 2010/09/24 00:58:37, vandebo wrote:
> Don't need this anymore.

Done.

http://codereview.chromium.org/3432009/diff/33004/13014#newcode101
net/spdy/spdy_proxy_client_socket.cc:101: // TODO(rch): what should this
implementation be?
On 2010/09/24 00:58:37, vandebo wrote:
> I guess these should pass through the the spdy_stream transport socket as
well. 
> jar@ can tell you for sure what he'd like done.

I'll ask...

http://codereview.chromium.org/3432009/diff/33004/13014#newcode105
net/spdy/spdy_proxy_client_socket.cc:105: return IsConnected() &&
spdy_stream_->WasEverUsed();
On 2010/09/24 00:58:37, vandebo wrote:
> I'm not sure IsConnected() && will do the right thing... What happens when
> you've Connected and Disconnected?

The interface does not clearly specify behavior in that circumstance;

  // Returns true if the underlying transport socket ever had any reads or
  // writes.  ClientSockets layered on top of transport sockets should forward
  // this call to the transport socket.
  virtual bool WasEverUsed() const = 0;

In particular, if we Disconnect, then we no longer *have* a transport socket. 
So it's possible that in the time between when Disconnect is called and when
WasEverUsed() is called that the transport socket's state might have changed. 

But I guess the most conservative thing to do would be to save the transport's
value on disconnect and return that.  Cool?

http://codereview.chromium.org/3432009/diff/33004/13014#newcode120
net/spdy/spdy_proxy_client_socket.cc:120: DCHECK(next_state_ == STATE_DONE);
On 2010/09/24 00:58:37, vandebo wrote:
> I think this DCHECK is false if we hit an http error.

Yeah, after our last exchange, I wasn't entirely sure what behavior you were
expecting in the case of an HTTP error.  I've added the following comment to the
Connect method.  I think (hope) you agree:

// Sends a SYN_STREAM frame to the proxy with a CONNECT request
// for the specified endpoint.  Waits for the server to send back
// a SYN_REPLY frame.  OK will be returned if the status is 200.
// ERR_PROXY_AUTH_REQUESTED will be returned if the status is 407.
// ERR_TUNNEL_CONNECTION_FAILED will be returned for any other status.
// In any of these cases, Read() may be called to retrieve the HTTP
// response body.  Any other return values should be considered fatal. 

I've added a test that, for example, if the proxy returns an HTTP 500, that we
return ERR_TUNNEL_CONNECTION_FAILED and can read the body of the response.  I
think this is what you're asking for, right?

http://codereview.chromium.org/3432009/diff/33004/13014#newcode164
net/spdy/spdy_proxy_client_socket.cc:164: return
spdy_stream_->WriteStreamData(buf, buf_len, spdy::DATA_FLAG_NONE);
On 2010/09/24 00:58:37, vandebo wrote:
> This code assumes that WriteSteamData will return ERR_IO_PENDING since it sets
> write_callback_ unconditionally.

Good catch!

http://codereview.chromium.org/3432009/diff/33004/13014#newcode177
net/spdy/spdy_proxy_client_socket.cc:177: LOG(INFO) << "write failed, rv: " <<
rv;
On 2010/09/23 23:20:31, willchan wrote:
> You should get rid of this LOG.

Done.

http://codereview.chromium.org/3432009/diff/33004/13014#newcode182
net/spdy/spdy_proxy_client_socket.cc:182: LOG(INFO) << "io pending";
On 2010/09/24 00:58:37, vandebo wrote:
> And this one

Done.

http://codereview.chromium.org/3432009/diff/33004/13014#newcode187
net/spdy/spdy_proxy_client_socket.cc:187: LOG(INFO) << "done";
On 2010/09/24 00:58:37, vandebo wrote:
> And this one

Done.

http://codereview.chromium.org/3432009/diff/33004/13014#newcode343
net/spdy/spdy_proxy_client_socket.cc:343: next_state_ = STATE_DONE;
On 2010/09/24 00:58:37, vandebo wrote:
> We're not done here - we may need to restart with auth.

As currently implemented, it is not possible to restart with auth.  Recall that
with a SPDY stream, you only get 1 SYN_STREAM frame.  Once we've sent that
frame, and received a response, we can not send it again.

Now we could change the implementation of this class so that instead of being
constructed with a SpdyStream, we could require the SpdySession, and construct
the stream on demand.  In this case, if we decide to restart, we could
disconnect the spdy stream, and create a new one.

All of that being said, I don't see where the requirement for restarting with
auth comes from.  Is your point that nothing at a higher level is able to do the
restart, so this class must do it?  I thought that if HttpNetworkTransaction
sees proxy auth required, it will retry.

http://codereview.chromium.org/3432009/diff/33004/13014#newcode347
net/spdy/spdy_proxy_client_socket.cc:347: // For all other status codes, we
conservatively fail the CONNECT
On 2010/09/24 00:58:37, vandebo wrote:
> This doesn't apply in this case, since we're not vulnerable to a MITM.  You
> should return an error such that the high level (HttpNetworkTransaction)
doesn't
> try to send a request on the connection, but returns success so that the level
> above (HttpUrlRequestJob, etc.) will request and render the proxy error page.

Agreed.  With your most recent comments, I think I (now) understand the behavior
you're requesting in the non-200, non-407 case.  Hopefully the code does what
you want now :>

http://codereview.chromium.org/3432009/diff/33004/13014#newcode353
net/spdy/spdy_proxy_client_socket.cc:353:
LogBlockedTunnelResponse(response_.headers->response_code());
On 2010/09/23 23:20:31, willchan wrote:
> I feel like we should be writing this out to the net_log_.  I think
> HttpProxyClientSocket should be doing this too.  This will be helpful for you
to
> test and to debug user bug reports once people start reporting this type of
> error.

So with the changes vandebo suggests, this code path is going away.  Cool?

http://codereview.chromium.org/3432009/diff/33004/13014#newcode386
net/spdy/spdy_proxy_client_socket.cc:386: NOTIMPLEMENTED();
On 2010/09/23 23:20:31, willchan wrote:
> No need for NOTIMPLEMENTED().  We used that mainly when porting, to indicate
> things we still had to implement.  This should be a NOTREACHED().

Done.

http://codereview.chromium.org/3432009/diff/33004/13014#newcode404
net/spdy/spdy_proxy_client_socket.cc:404: DCHECK(next_state_ =
STATE_READ_REPLY_COMPLETE);
On 2010/09/23 23:20:31, willchan wrote:
> Is this a bug?  Should it be DCHECK_EQ()?

I've switched it to DCHECK_EQ (my brain seems to only remember that DCHECK
exists.  argh).  Is that the bug you were asking about?

http://codereview.chromium.org/3432009/diff/33004/13016
File net/spdy/spdy_proxy_client_socket_unittest.cc (right):

http://codereview.chromium.org/3432009/diff/33004/13016#newcode85
net/spdy/spdy_proxy_client_socket_unittest.cc:85: HttpNetworkSession* session_;
On 2010/09/24 00:58:37, vandebo wrote:
> Some of these should probably be private, no?

Done.

http://codereview.chromium.org/3432009/diff/33004/13016#newcode128
net/spdy/spdy_proxy_client_socket_unittest.cc:128: void
SpdyProxyClientSocketTest::SetUp() {
On 2010/09/24 00:58:37, vandebo wrote:
> You don't need this, it will already do what you've written.

Done.

http://codereview.chromium.org/3432009/diff/33004/13016#newcode133
net/spdy/spdy_proxy_client_socket_unittest.cc:133: if (session_ != NULL)
On 2010/09/24 00:58:37, vandebo wrote:
> Do you really want this in TearDown() instead of the destructor?

I'm not sure :>  session_ is only non-null after Initialize is called, which
must take place after SetUp() has been called.  That made me think that TearDown
is the logical place to clean it up.  What's the convention for distributing
cleanup tasks between TearDown and the destructor?  (In java you always put such
code in TearDown, but in java you mostly don't use destructors :>)

http://codereview.chromium.org/3432009/diff/33004/13016#newcode162
net/spdy/spdy_proxy_client_socket_unittest.cc:162: EXPECT_EQ(OK,
spdy_session_->Connect(kOriginHostPort, tcp_params_, LOWEST));
On 2010/09/24 00:58:37, vandebo wrote:
> Do you want this to be ASSERT?  The rest of the test will fail if this does,
> right?

Done.

http://codereview.chromium.org/3432009/diff/33004/13016#newcode190
net/spdy/spdy_proxy_client_socket_unittest.cc:190: void
SpdyProxyClientSocketTest::CheckConnectionEstablished() {
On 2010/09/24 00:58:37, vandebo wrote:
> Should these be named AssertFoo or something since they assert, whereas
> CheckConnetSucceeds, for example, expects?

Done.  Franly, CheckConnectSucceeds should be asserting as well, so I've changed
it to use ASSERT, and renamed it AssertConnectSucceeds.

http://codereview.chromium.org/3432009/diff/33004/13016#newcode211
net/spdy/spdy_proxy_client_socket_unittest.cc:211: // Dummy write to un-block
the read
On 2010/09/24 00:58:37, vandebo wrote:
> Writing to read could have side effects that makes your test work in unit
tests
> but not in the real world.  Would a mock object work better?  Or some kind of
> Spdy data provider?

Ugh.  I think it has taken me as much time to learn how to get these
OrderedSocketData providers to do what I've wanted as it has to write the actual
implementation of this class :>

I see your point, and I think it's totally valid.  A much cleaner setup would be
to use a socket data provider of some sort that has method like SendNextWrite()
/ SendNextRead().  I am happy to write such a class and modify the tests
accordingly.  However, I fear that this is likely to be very time consuming. 
Can I do this in a second CL?

http://codereview.chromium.org/3432009/diff/33004/13016#newcode215
net/spdy/spdy_proxy_client_socket_unittest.cc:215:
EXPECT_TRUE(sock_->IsConnected());
On 2010/09/24 00:58:37, vandebo wrote:
> redundant

Done.

http://codereview.chromium.org/3432009/diff/33004/13016#newcode338
net/spdy/spdy_proxy_client_socket_unittest.cc:338: scoped_ptr<spdy::SpdyFrame>
conn(ConstructConnectRequestFrame());
On 2010/09/24 00:58:37, vandebo wrote:
> You could make this take a scoped_ptr<spdy::SpdyFrame>* and return a
MockWrite.

I'm not sure what "this" is?  The description you provided is precisely what
CreateMockWrite() does.  The trick is that the Frame need to live until the
MockWrite actually happens, since the MockWrite merely holds a pointer to the
Frame's data.  I thought about having the framework have a method that turns a
Frame into a Read/Write by first adding the frame to a list of active frames,
and then returning CreateMockRead/CreateMockWrite.  But looking at the other
Spdy tests, I decided to follow the existing convention.

http://codereview.chromium.org/3432009/diff/33004/13016#newcode350
net/spdy/spdy_proxy_client_socket_unittest.cc:350: 
On 2010/09/24 00:58:37, vandebo wrote:
> To check that your framework works properly, you might want to check that
you're
> not connected here.

Done.

http://codereview.chromium.org/3432009/diff/33004/13016#newcode393
net/spdy/spdy_proxy_client_socket_unittest.cc:393: AddAuthToCache();
On 2010/09/24 00:58:37, vandebo wrote:
> Probably also want a test case that gets a 407, and then tries again with the
> correct auth.

As I mentioned in your comment about restarting with auth, this is not possible
with a single socket.  Perhaps this is a big problem?

http://codereview.chromium.org/3432009/diff/33004/13016#newcode401
net/spdy/spdy_proxy_client_socket_unittest.cc:401:
TEST_F(SpdyProxyClientSocketTest, ConnectWaitsForResponse) {
On 2010/09/24 00:58:37, vandebo wrote:
> How is this different than the connect test?

You're right, it's not.  As I cleaned up the test framework, I removed the
distinction without realizing it.  I've removed this test.

http://codereview.chromium.org/3432009/diff/33004/13016#newcode539
net/spdy/spdy_proxy_client_socket_unittest.cc:539:
EXPECT_EQ(ERR_PROXY_AUTH_REQUESTED, callback_.WaitForResult());
On 2010/09/24 00:58:37, vandebo wrote:
> I don't think this is how higher level network code will interact with this
> code.  This suggests that you want an integration test as well.  Maybe an
> HttpNetworkTransaction test.

Can you explain how you think the higher level network code will interact with
this code?  Is there some interface description that I should read?

I will certainly write such a test when this class is integrated into the stream
factory and, hence, HttpNetworkTransaction.  My objective with this CL is to get
the functionality of the SpdyProxyClientSocket working and then to a second CL
(that is now sitting in my work area waiting for this to land) to integrated it.
 Landing/reviewing complicated CLs seems to be fairly time consuming so I
thought it best to do this in two parts, and mbelshe agreed.  However, if you
think this is the wrong approach, I can attempt to add the integration into this
CL. I would *much* prefer to get this in, and then do a second CL to integrate
it, even if it requires changes to this class.

http://codereview.chromium.org/3432009/diff/33004/13016#newcode647
net/spdy/spdy_proxy_client_socket_unittest.cc:647: CheckSyncReadEquals(kMsg33,
kLen33);
On 2010/09/24 00:58:37, vandebo wrote:
> I don't see a test that reads part of the reply data and then reads more/the
> rest with a subsequent read.

Done.

http://codereview.chromium.org/3432009/diff/33004/13016#newcode785
net/spdy/spdy_proxy_client_socket_unittest.cc:785: }  // namespace net
On 2010/09/24 00:58:37, vandebo wrote:
> There are probably a few more failure tests you could add. i.e. failing to
> connect, etc.

I added one with where Connect() fails because the connection is closed.  Does
that work for you?

willchan no longer on Chromium

I think everything roughly looks good now. I'm giving my LGTM, but you should wait ...

10 years, 2 months ago (2010-09-27 20:09:51 UTC) #25

Ryan Hamilton

http://codereview.chromium.org/3432009/diff/12027/82010 File net/spdy/spdy_proxy_client_socket.cc (right): http://codereview.chromium.org/3432009/diff/12027/82010#newcode113 net/spdy/spdy_proxy_client_socket.cc:113: DCHECK(!read_callback_); On 2010/09/27 20:09:51, willchan wrote: > Include "base/logging.h" ...

10 years, 2 months ago (2010-09-27 20:28:01 UTC) #26

vandebo (ex-Chrome)

LGTM as long as you fix up the last comment. http://codereview.chromium.org/3432009/diff/33004/13014 File net/spdy/spdy_proxy_client_socket.cc (right): http://codereview.chromium.org/3432009/diff/33004/13014#newcode343 ...

10 years, 2 months ago (2010-09-27 21:27:34 UTC) #27

LGTM as long as you fix up the last comment.

http://codereview.chromium.org/3432009/diff/33004/13014
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/3432009/diff/33004/13014#newcode343
net/spdy/spdy_proxy_client_socket.cc:343: next_state_ = STATE_DONE;
On 2010/09/24 18:25:58, Ryan Hamilton wrote:
> On 2010/09/24 00:58:37, vandebo wrote:
> > We're not done here - we may need to restart with auth.
> 
> As currently implemented, it is not possible to restart with auth.  Recall
that
> with a SPDY stream, you only get 1 SYN_STREAM frame.  Once we've sent that
> frame, and received a response, we can not send it again.
> 
> Now we could change the implementation of this class so that instead of being
> constructed with a SpdyStream, we could require the SpdySession, and construct
> the stream on demand.  In this case, if we decide to restart, we could
> disconnect the spdy stream, and create a new one.
> 
> All of that being said, I don't see where the requirement for restarting with
> auth comes from.  Is your point that nothing at a higher level is able to do
the
> restart, so this class must do it?  I thought that if HttpNetworkTransaction
> sees proxy auth required, it will retry.

rch and I talked about this and have postponed https proxy auth for later work.

http://codereview.chromium.org/3432009/diff/33004/13014#newcode386
net/spdy/spdy_proxy_client_socket.cc:386: NOTIMPLEMENTED();
On 2010/09/24 18:25:58, Ryan Hamilton wrote:
> On 2010/09/23 23:20:31, willchan wrote:
> > No need for NOTIMPLEMENTED().  We used that mainly when porting, to indicate
> > things we still had to implement.  This should be a NOTREACHED().
> 
> Done.

NOTREACHED() is a macro you should call her.

http://codereview.chromium.org/3432009/diff/33004/13014#newcode393
net/spdy/spdy_proxy_client_socket.cc:393: NOTIMPLEMENTED();
And here.

http://codereview.chromium.org/3432009/diff/33004/13014#newcode404
net/spdy/spdy_proxy_client_socket.cc:404: DCHECK(next_state_ =
STATE_READ_REPLY_COMPLETE);
On 2010/09/24 18:25:58, Ryan Hamilton wrote:
> On 2010/09/23 23:20:31, willchan wrote:
> > Is this a bug?  Should it be DCHECK_EQ()?
> 
> I've switched it to DCHECK_EQ (my brain seems to only remember that DCHECK
> exists.  argh).  Is that the bug you were asking about?

DCHECK_EQ fixes it, but the bug was = vs ==

http://codereview.chromium.org/3432009/diff/33004/13016
File net/spdy/spdy_proxy_client_socket_unittest.cc (right):

http://codereview.chromium.org/3432009/diff/33004/13016#newcode211
net/spdy/spdy_proxy_client_socket_unittest.cc:211: // Dummy write to un-block
the read
On 2010/09/24 18:25:58, Ryan Hamilton wrote:
> On 2010/09/24 00:58:37, vandebo wrote:
> > Writing to read could have side effects that makes your test work in unit
> tests
> > but not in the real world.  Would a mock object work better?  Or some kind
of
> > Spdy data provider?
> 
> Ugh.  I think it has taken me as much time to learn how to get these
> OrderedSocketData providers to do what I've wanted as it has to write the
actual
> implementation of this class :>
> 
> I see your point, and I think it's totally valid.  A much cleaner setup would
be
> to use a socket data provider of some sort that has method like
SendNextWrite()
> / SendNextRead().  I am happy to write such a class and modify the tests
> accordingly.  However, I fear that this is likely to be very time consuming. 
> Can I do this in a second CL?

Seems fine.

http://codereview.chromium.org/3432009/diff/33004/13016#newcode338
net/spdy/spdy_proxy_client_socket_unittest.cc:338: scoped_ptr<spdy::SpdyFrame>
conn(ConstructConnectRequestFrame());
On 2010/09/24 18:25:58, Ryan Hamilton wrote:
> On 2010/09/24 00:58:37, vandebo wrote:
> > You could make this take a scoped_ptr<spdy::SpdyFrame>* and return a
> MockWrite.
> 
> I'm not sure what "this" is?  The description you provided is precisely what
> CreateMockWrite() does.  The trick is that the Frame need to live until the
> MockWrite actually happens, since the MockWrite merely holds a pointer to the
> Frame's data.  I thought about having the framework have a method that turns a
> Frame into a Read/Write by first adding the frame to a list of active frames,
> and then returning CreateMockRead/CreateMockWrite.  But looking at the other
> Spdy tests, I decided to follow the existing convention.
> 

If the rest of the tests do this, then you should probably leave it this way.  I
was suggesting:

scoped_ptr<spdy::SpdyFrame> conn;
MockWrite writes[] = { CreateMockWrite(&conn, 0), };

http://codereview.chromium.org/3432009/diff/31015/99007
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/3432009/diff/31015/99007#newcode64
net/spdy/spdy_proxy_client_socket.cc:64: // ERR_TUNNEL_CONNECTION_FAILED will be
returned for any other status.
I think you need to add a new error code because no error code implies the
behavior you want.  You can leave this as a todo/bug though.  More specifically:
with an https proxy, it's ok to read the error page, but you should try to issue
the GET/PUT after the CONNECT fails.  Returning OK would get the url request
context to read the error page, but the http network transaction would try to
issue the actual request first.  Similarly, with ERR_TUNNEL_CONNECTION_FAILED
(or any other error code, as far as a simple scan reveals), http network
transaction won't send the actual request, but the url request context won't get
the error page either.

http://codereview.chromium.org/3432009/diff/31015/99007#newcode419
net/spdy/spdy_proxy_client_socket.cc:419: if (next_state_ > STATE_NONE &&
next_state_ != STATE_DONE) {
probably should be !=, not >

http://codereview.chromium.org/3432009/diff/31015/99008
File net/spdy/spdy_proxy_client_socket.h (right):

http://codereview.chromium.org/3432009/diff/31015/99008#newcode174
net/spdy/spdy_proxy_client_socket.h:174: bool was_ever_used_;
This needs to be initialized.

Ryan Hamilton

Thanks for the LGTM. I've addressed the issues, and will commit after the trys return ...

10 years, 2 months ago (2010-09-27 21:55:58 UTC) #28

Thanks for the LGTM.  I've addressed the issues, and will commit after the trys
return green (for this most recent iteration of the patch).

http://codereview.chromium.org/3432009/diff/33004/13014
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/3432009/diff/33004/13014#newcode386
net/spdy/spdy_proxy_client_socket.cc:386: NOTIMPLEMENTED();
On 2010/09/27 21:27:34, vandebo wrote:
> On 2010/09/24 18:25:58, Ryan Hamilton wrote:
> > On 2010/09/23 23:20:31, willchan wrote:
> > > No need for NOTIMPLEMENTED().  We used that mainly when porting, to
indicate
> > > things we still had to implement.  This should be a NOTREACHED().
> > 
> > Done.
> 
> NOTREACHED() is a macro you should call her.

Done.

http://codereview.chromium.org/3432009/diff/33004/13014#newcode393
net/spdy/spdy_proxy_client_socket.cc:393: NOTIMPLEMENTED();
On 2010/09/27 21:27:34, vandebo wrote:
> And here.

Done.

http://codereview.chromium.org/3432009/diff/33004/13014#newcode404
net/spdy/spdy_proxy_client_socket.cc:404: DCHECK(next_state_ =
STATE_READ_REPLY_COMPLETE);
On 2010/09/27 21:27:34, vandebo wrote:
> On 2010/09/24 18:25:58, Ryan Hamilton wrote:
> > On 2010/09/23 23:20:31, willchan wrote:
> > > Is this a bug?  Should it be DCHECK_EQ()?
> > 
> > I've switched it to DCHECK_EQ (my brain seems to only remember that DCHECK
> > exists.  argh).  Is that the bug you were asking about?
> 
> DCHECK_EQ fixes it, but the bug was = vs ==

oof!  I didn't even notice that.  Thanks, guys!

http://codereview.chromium.org/3432009/diff/31015/99007#newcode64
net/spdy/spdy_proxy_client_socket.cc:64: DCHECK_EQ(STATE_NONE, next_state_);
On 2010/09/27 21:27:34, vandebo wrote:
> I think you need to add a new error code because no error code implies the
> behavior you want.  You can leave this as a todo/bug though.  More
specifically:
> with an https proxy, it's ok to read the error page, but you should try to
issue
> the GET/PUT after the CONNECT fails.  Returning OK would get the url request
> context to read the error page, but the http network transaction would try to
> issue the actual request first.  Similarly, with ERR_TUNNEL_CONNECTION_FAILED
> (or any other error code, as far as a simple scan reveals), http network
> transaction won't send the actual request, but the url request context won't
get
> the error page either.

Sounds good.

http://codereview.chromium.org/3432009/diff/31015/99007#newcode419
net/spdy/spdy_proxy_client_socket.cc:419: 
On 2010/09/27 21:27:34, vandebo wrote:
> probably should be !=, not >

Done.  Given that STATE_NONE is the first state, they're both equivalent, but I
like your version better.

http://codereview.chromium.org/3432009/diff/31015/99008
File net/spdy/spdy_proxy_client_socket.h (right):

http://codereview.chromium.org/3432009/diff/31015/99008#newcode174
net/spdy/spdy_proxy_client_socket.h:174: bool was_ever_used_;
On 2010/09/27 21:27:34, vandebo wrote:
> This needs to be initialized.

I think it is initialized, but in the body of the constructor, not in the
initialization list.

  54   was_ever_used_ = spdy_stream_->WasEverUsed();

I believe I have a test which verifies SpdyProxyClientSocket::WasEverUsed()
returns false on a newly created socket.

vandebo (ex-Chrome)

10 years, 2 months ago (2010-09-27 22:00:26 UTC) #29

LG

http://codereview.chromium.org/3432009/diff/31015/99008
File net/spdy/spdy_proxy_client_socket.h (right):

http://codereview.chromium.org/3432009/diff/31015/99008#newcode174
net/spdy/spdy_proxy_client_socket.h:174: bool was_ever_used_;
On 2010/09/27 21:55:58, Ryan Hamilton wrote:
> On 2010/09/27 21:27:34, vandebo wrote:
> > This needs to be initialized.
> 
> I think it is initialized, but in the body of the constructor, not in the
> initialization list.
> 
>   54   was_ever_used_ = spdy_stream_->WasEverUsed();
> 
> I believe I have a test which verifies SpdyProxyClientSocket::WasEverUsed()
> returns false on a newly created socket.
> 

Oops, didn't notice that was the constructor.

Issue 3432009: Add a new class SpdyProxyClientSocket which implements ClientSocket... (Closed)

Description

Patch Set 1 : '' #

Patch Set 2 : '' #

Patch Set 3 : '' #

Patch Set 4 : '' #

Patch Set 5 : Used scoped_refptr to store the SpdyStream #

Patch Set 6 : '' #

Patch Set 7 : '' #

Patch Set 8 : Rebasing to HEAD #

Patch Set 9 : '' #

Patch Set 10 : Added spdy_stream.{cc,h} to the patch #

Patch Set 11 : Fix return value of OnSendBodyComplete to make windows build happy #

Patch Set 12 : Fix Proxy-Authenticate handling #

Patch Set 13 : '' #

Patch Set 14 : Rebase, and address vandebo's comments #

Patch Set 15 : '' #

Patch Set 16 : Remove 407 handling until the protocol is more solid #

Patch Set 17 : Rebasing #

Patch Set 18 : '' #

Patch Set 19 : '' #

Messages