Refactor state machines in HttpPipelinedConnectionImpl.

Refactor state machines in HttpPipelinedConnectionImpl.

Hopefully you agree that this makes the code a whole lot easier to understand.

BUG=None
TEST=net_unittests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110159

[mmenke, 2011-11-11 16:46:35]

http://codereview.chromium.org/8515020/diff/1/net/http/http_pipelined_connect...
File net/http/http_pipelined_connection_impl.cc (right):

http://codereview.chromium.org/8515020/diff/1/net/http/http_pipelined_connect...
net/http/http_pipelined_connection_impl.cc:138: rv = DoSendRequestLoop(OK);
It might be a little simpler just to only set send_request->callback to be
non-null after calling DoSendRequestLoop, if the result is ERR_IO_PENDING.  Then
you wouldn't need two StartRequest functions.  Just only send the message if the
callback is non-null.  And could do the same for reading, of course.

Or not.  Just thinking outloud here.

http://codereview.chromium.org/8515020/diff/1/net/http/http_pipelined_connect...
net/http/http_pipelined_connection_impl.cc:247: if (send_still_on_call_stack_) {
nit:  Maybe add a comment here that in this case, there can be no pending
requests?

http://codereview.chromium.org/8515020/diff/1/net/http/http_pipelined_connect...
net/http/http_pipelined_connection_impl.cc:320: return rv;
nit:  Think it might be nice to have a comment drawing attention to the fact we
return here, rather than breaking.

[James Simonsen, 2011-11-11 19:26:08]

http://codereview.chromium.org/8515020/diff/1/net/http/http_pipelined_connect...
File net/http/http_pipelined_connection_impl.cc (right):

http://codereview.chromium.org/8515020/diff/1/net/http/http_pipelined_connect...
net/http/http_pipelined_connection_impl.cc:138: rv = DoSendRequestLoop(OK);
On 2011/11/11 16:46:35, Matt Menke wrote:
> It might be a little simpler just to only set send_request->callback to be
> non-null after calling DoSendRequestLoop, if the result is ERR_IO_PENDING. 
Then
> you wouldn't need two StartRequest functions.  Just only send the message if
the
> callback is non-null.  And could do the same for reading, of course.
> 
> Or not.  Just thinking outloud here.

It would definitely merge the two cases, which would simplify the code. However,
my thinking was that I should err on the side of being explicit, since I think
that's what was lacking in the old code. For example, in DoSendComplete(), I
think it's easier to understand |send_still_on_call_stack_| than it is to
remember why |callback| is non-null and what that means.

But, that's just my opinion. You and Will are the ones that had to figure this
all out from scratch. Which do you prefer?

http://codereview.chromium.org/8515020/diff/1/net/http/http_pipelined_connect...
net/http/http_pipelined_connection_impl.cc:247: if (send_still_on_call_stack_) {
On 2011/11/11 16:46:35, Matt Menke wrote:
> nit:  Maybe add a comment here that in this case, there can be no pending
> requests?

Done.

http://codereview.chromium.org/8515020/diff/1/net/http/http_pipelined_connect...
net/http/http_pipelined_connection_impl.cc:320: return rv;
On 2011/11/11 16:46:35, Matt Menke wrote:
> nit:  Think it might be nice to have a comment drawing attention to the fact
we
> return here, rather than breaking.

Done.

[willchan no longer on Chromium, 2011-11-11 23:19:34]

Can you flesh out what the changes are supposed to be in the changelist
description or something? I'm in particular curious what the
|send_still_on_call_stack_| business is all about.

[James Simonsen, 2011-11-11 23:57:20]

On 2011/11/11 23:19:34, willchan wrote:
> Can you flesh out what the changes are supposed to be in the changelist
> description or something? I'm in particular curious what the
> |send_still_on_call_stack_| business is all about.

Honestly, I was kinda hoping you could just read the code and figure it out. If
not, that means it needs to be cleaned up further. Perhaps you could just ignore
the diffs and read through the new code and see if the state machines are easier
to understand now?

If you'd still like me to spell out the changes and what they mean, I can do
that too. Just let me know.

To be clear, this is entirely a cleanup. There should be no functionality
changes and all the existing tests still pass.

[mmenke, 2011-11-12 00:59:26]

The issue isn't re-entracy, but that we need different behavior if
something just tried to send data, and we finish synchronously, or if we
finish synchronously, but we were called of an earlier send completing.  In
the second case, we want to use the callback, but in the first case, we
don't.

On Fri, Nov 11, 2011 at 7:00 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> On Fri, Nov 11, 2011 at 3:57 PM, <simonjam@chromium.org> wrote:
>
>> On 2011/11/11 23:19:34, willchan wrote:
>>
>>> Can you flesh out what the changes are supposed to be in the changelist
>>> description or something? I'm in particular curious what the
>>> |send_still_on_call_stack_| business is all about.
>>>
>>
>> Honestly, I was kinda hoping you could just read the code and figure it
>> out. If
>> not, that means it needs to be cleaned up further. Perhaps you could just
>> ignore
>> the diffs and read through the new code and see if the state machines are
>> easier
>> to understand now?
>
>
>> If you'd still like me to spell out the changes and what they mean, I can
>> do
>> that too. Just let me know.
>>
>> To be clear, this is entirely a cleanup. There should be no functionality
>> changes and all the existing tests still pass.
>>
>
> That's fair. Honestly, I am probably just going to defer completely to
> Matt at this point. He seems to be doing a great job with the reviews,
> although I'd like to stay up to date with your design decisions. The main
> thing I wanted to know was why this |send_still_on_call_stack_| variable
> exists. If this is a re-entrancy issue, I'd like to hear the problem and
> the solution explained. I've dealt with a lot of re-entrancy issues in the
> HTTP stack and have some reasonably strong opinions.
>
>
>>
>>
http://codereview.chromium.**org/8515020/<http://codereview.chromium.org/8515...
>>
>
>

[willchan no longer on Chromium, 2011-11-12 01:46:10]

On Fri, Nov 11, 2011 at 3:57 PM, <simonjam@chromium.org> wrote:

> On 2011/11/11 23:19:34, willchan wrote:
>
>> Can you flesh out what the changes are supposed to be in the changelist
>> description or something? I'm in particular curious what the
>> |send_still_on_call_stack_| business is all about.
>>
>
> Honestly, I was kinda hoping you could just read the code and figure it
> out. If
> not, that means it needs to be cleaned up further. Perhaps you could just
> ignore
> the diffs and read through the new code and see if the state machines are
> easier
> to understand now?


> If you'd still like me to spell out the changes and what they mean, I can
> do
> that too. Just let me know.
>
> To be clear, this is entirely a cleanup. There should be no functionality
> changes and all the existing tests still pass.
>

That's fair. Honestly, I am probably just going to defer completely to Matt
at this point. He seems to be doing a great job with the reviews, although
I'd like to stay up to date with your design decisions. The main thing I
wanted to know was why this |send_still_on_call_stack_| variable exists. If
this is a re-entrancy issue, I'd like to hear the problem and the solution
explained. I've dealt with a lot of re-entrancy issues in the HTTP stack
and have some reasonably strong opinions.


>
>
http://codereview.chromium.**org/8515020/<http://codereview.chromium.org/8515...
>

[mmenke, 2011-11-14 16:25:12]

http://codereview.chromium.org/8515020/diff/1/net/http/http_pipelined_connect...
File net/http/http_pipelined_connection_impl.cc (right):

http://codereview.chromium.org/8515020/diff/1/net/http/http_pipelined_connect...
net/http/http_pipelined_connection_impl.cc:138: rv = DoSendRequestLoop(OK);
On 2011/11/11 19:26:08, James Simonsen wrote:
> On 2011/11/11 16:46:35, Matt Menke wrote:
> > It might be a little simpler just to only set send_request->callback to be
> > non-null after calling DoSendRequestLoop, if the result is ERR_IO_PENDING. 
> Then
> > you wouldn't need two StartRequest functions.  Just only send the message if
> the
> > callback is non-null.  And could do the same for reading, of course.
> > 
> > Or not.  Just thinking outloud here.
> 
> It would definitely merge the two cases, which would simplify the code.
However,
> my thinking was that I should err on the side of being explicit, since I think
> that's what was lacking in the old code. For example, in DoSendComplete(), I
> think it's easier to understand |send_still_on_call_stack_| than it is to
> remember why |callback| is non-null and what that means.
> 
> But, that's just my opinion. You and Will are the ones that had to figure this
> all out from scratch. Which do you prefer?

I think that setting the callback after the call to DoSendRequestLoop is a
messy, bad idea, now that I think about it.

Turning DoSendActiveRequest into a subroutine called by the previous two states,
and having DoStartRequestImmediately possibly set the callback to null would
make more sense, but I'm not convinced it's an improvement on this.  So feel
free to keep the code as-is.

I do think adding a comment at the start of DoStartRequestImmediately and the
corresponding read function explaining why separate functions are needed would
help make the code easier to follow, though.

http://codereview.chromium.org/8515020/diff/4001/net/http/http_pipelined_conn...
File net/http/http_pipelined_connection_impl.cc (right):

http://codereview.chromium.org/8515020/diff/4001/net/http/http_pipelined_conn...
net/http/http_pipelined_connection_impl.cc:348: int
HttpPipelinedConnectionImpl::DoStartReadImmediately(int result) {
You could put some CHECKs analogous to the corresponding send functions in this
function and the next:

like CHECK(!active_read_id_), etc

[James Simonsen, 2011-11-14 21:09:49]

I added comments. I'm slightly partial to having separate states for starting to
send and actually sending, so I left the state machine code as is.

http://codereview.chromium.org/8515020/diff/4001/net/http/http_pipelined_conn...
File net/http/http_pipelined_connection_impl.cc (right):

http://codereview.chromium.org/8515020/diff/4001/net/http/http_pipelined_conn...
net/http/http_pipelined_connection_impl.cc:348: int
HttpPipelinedConnectionImpl::DoStartReadImmediately(int result) {
On 2011/11/14 16:25:12, Matt Menke wrote:
> You could put some CHECKs analogous to the corresponding send functions in
this
> function and the next:
> 
> like CHECK(!active_read_id_), etc

Done.

[mmenke, 2011-11-15 16:54:43]

LGTM

[commit-bot: I haz the power, 2011-11-15 19:23:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonjam@chromium.org/8515020/10002

[commit-bot: I haz the power, 2011-11-15 20:36:53]

Change committed as 110159