After draining the body of a 401/407 response, verify that...

After draining the body of a 401/407 response, verify that
the keep-alive connection is still connected and idle before
reusing it for authentication restart.  An impatient server
may have closed the connection while waiting for the user to
enter the username and password.

In socket_test_util.cc, return the mock ERR_UNEXPECTED error
synchronously.

R=eroman
BUG=21675
TEST=new unit test

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

[wtc, 2009-11-11 04:00:17]

willchan: I have a question for you in the TODO comment in
http_network_transaction.cc.  You don't need to review this
CL.

eroman: please review this CL.  Would you be willing to let
me check in the fix (http_network_transaction.cc only) first?
The problem with the new unit test is that it's not easy to
mock the IsConnectedAndIdle method.  It requires the Peek
method, but the Peek method is unsafe if we don't know the
size of the MockRead array.  So I still need to do more
work to pass the size of the MockRead and MockWrite arrays
to StaticMockSocket.

http://codereview.chromium.org/389007/diff/1/4
File net/socket/socket_test_util.cc (right):

http://codereview.chromium.org/389007/diff/1/4#newcode227
Line 227: return reads_[read_index_];
Without knowing the size of the reads_ array, this
implementation of the Peek method is unsafe.

[wtc, 2009-11-11 04:00:35]

Reviewers: eroman,

Message:
willchan: I have a question for you in the TODO comment in
http_network_transaction.cc.  You don't need to review this
CL.

eroman: please review this CL.  Would you be willing to let
me check in the fix (http_network_transaction.cc only) first?
The problem with the new unit test is that it's not easy to
mock the IsConnectedAndIdle method.  It requires the Peek
method, but the Peek method is unsafe if we don't know the
size of the MockRead array.  So I still need to do more
work to pass the size of the MockRead and MockWrite arrays
to StaticMockSocket.


http://codereview.chromium.org/389007/diff/1/4
File net/socket/socket_test_util.cc (right):

http://codereview.chromium.org/389007/diff/1/4#newcode227
Line 227: return reads_[read_index_];
Without knowing the size of the reads_ array, this
implementation of the Peek method is unsafe.

Description:
After draining the body of a 401/407 response, verify that
the keep-alive connection is still connected and idle before
reusing it for authentication restart.  An impatient server
may have closed the connection while waiting for the user to
enter the username and password.

R=eroman
BUG=21675
TEST=new unit test

Please review this at http://codereview.chromium.org/389007

SVN Base: svn://chrome-svn/chrome/trunk/src/

Affected files:
   M     net/http/http_network_transaction.cc
   M     net/http/http_network_transaction_unittest.cc
   M     net/socket/socket_test_util.h
   M     net/socket/socket_test_util.cc

[eroman, 2009-11-11 05:11:44]

lgtm

http://codereview.chromium.org/389007/diff/1/4
File net/socket/socket_test_util.cc (right):

http://codereview.chromium.org/389007/diff/1/4#newcode277
Line 277: return MockRead(true, ERR_UNEXPECTED);
This case is a bit awkward.

For DynamicMockSocket (or as it is now called, DynamicSocketDataProvider),
Peek() does not necessarily correspond with the next read will actually return.

I guess you want to return this MockRead() with async=true just so that
IsConnectedAndIdle() will return true?

Perhaps Peek() isn't the best abstraction; what if instead the data provider
directly exposed a IsConnectedAndIdle() function?

[eroman, 2009-11-11 05:12:04]

lgtm


http://codereview.chromium.org/389007/diff/1/4
File net/socket/socket_test_util.cc (right):

http://codereview.chromium.org/389007/diff/1/4#newcode277
Line 277: return MockRead(true, ERR_UNEXPECTED);
This case is a bit awkward.

For DynamicMockSocket (or as it is now called,
DynamicSocketDataProvider), Peek() does not necessarily correspond with
the next read will actually return.

I guess you want to return this MockRead() with async=true just so that
IsConnectedAndIdle() will return true?

Perhaps Peek() isn't the best abstraction; what if instead the data
provider directly exposed a IsConnectedAndIdle() function?

http://codereview.chromium.org/389007

[wtc, 2009-11-12 01:56:55]

http://codereview.chromium.org/389007/diff/1/4
File net/socket/socket_test_util.cc (right):

http://codereview.chromium.org/389007/diff/1/4#newcode277
Line 277: return MockRead(true, ERR_UNEXPECTED);
On 2009/11/11 05:11:44, eroman wrote:
> This case is a bit awkward.
> 
> For DynamicMockSocket (or as it is now called, DynamicSocketDataProvider),
> Peek() does not necessarily correspond with the next read will actually
return.
> 
> I guess you want to return this MockRead() with async=true just so that
> IsConnectedAndIdle() will return true?

No.  Peek() is essentially GetNextRead() without actually
"moving the read pointer".  So I wrote Peek() by copying
the GetNextRead() code and updating it.  This case comes
from GetNextRead(), unchanged.  I didn't change it because
it has no side effect.

Sounds like we should return MockRead(false, ERR_UNEXPECTED)
in both GetNextRead() and Peek()?

> Perhaps Peek() isn't the best abstraction; what if instead the data provider
> directly exposed a IsConnectedAndIdle() function?

That is worth considering.  Let me describe the problem I
need to solve.  I need to make IsConnectedAndIdle() return
false after a particular MockRead in a StaticSocketDataProvider.  So if I add a
IsConnectedAndIdle
function to the data provider, that function and the Read
function will need to return values in lock step.  That
seems a lot of work.  This is why I looked into solutions
that use the MockRead array alone.

Another solution I considered is to add a magic MockRead
value that means "the peer (server) will close the
connection after the next MockRead".  It is used like this:

 MockRead data_reads1[] = {
    MockRead("HTTP/1.1 401 Unauthorized\r\n"),
    MockRead("WWW-Authenticate: Basic realm=\"MyRealm1\"\r\n"),
    MockRead("Content-Type: text/html; charset=iso-8859-1\r\n"),
    MockRead("Content-Length: 14\r\n\r\n"),
    MockRead(false, ERR_TEST_PEER_CLOSE_AFTER_NEXT_MOCK_READ),
    MockRead("Unauthorized\r\n"),
  };

I implemented this solution in the next patch set.
Do you prefer this solution?

Let's talk if my problem description isn't clear.  I
appreciate your help.

[eroman, 2009-11-12 10:26:14]

Lets discuss this in-person tomorrow, or over IM.

[eroman, 2009-11-12 22:08:47]

lgtm

nice improvement!

[wtc, 2009-11-12 23:05:23]

willchan: I added
  connection_.set_is_reused(true);
as we discussed.

eroman: I added some comments and committed the CL.  Thanks!