Code Review 6954525: Testcase failure java/net/Authenticator/B4769350.java

[Weijun Wang]

On 06/28/2010 08:04 PM, Chris Hegarty wrote:
> On 06/28/10 08:30 AM, Weijun Wang wrote:
>> Hi Chris
>>
>> I remember the reason that disconnectWeb() (or disconnectInternal()
>> before 6586707) is called is that somewhere in the HTTP spec it's said a
>> disconnect is needed when a 401 error is received. However, when I try
>> to search for this in RFC 2616, I cannot find anything.
>>
>> Do you remember if this is true?
>>
>> If no, maybe your code change
>>
>> - if (usingProxy()) {
>> + if (usingProxy() && http.isKeepingAlive()) {
>>
>> can be further changed to
>>
>> - if (usingProxy()) {
>> + if (http.isKeepingAlive()) {
>>
>> which looks very straightforward: keep alive if isKeepingAlive, and
>> disconnect otherwise.
>
> You added disconnectWeb specifically to allow authentication with a
> server through a proxy which also requires NTLM auth, right? All I want

Yes, disconnectWeb() keeps the connection at the 401 error where there 
is a proxy.

> to do here is to fix a problem in disconnectWeb whereby it tries to keep
> a connection open with the proxy/server explicitly says it is going to
> close it.
>
> If its ok with you I'd like to keep this change as is. We should do

OK. Your change certainly fixes the problem.

> further testing before making your proposed change.

I've run all net-related regression tests on my Linux, and has tried 
several combination of proxy-auth / www-auth. Seems OK at the moment.

>
>> I've tried this on our SQE HTTP server and seems it's working fine. The
>> server does not need a disconnect at all.
>>
>> As for the test, I don't know why you send "Connection: close" in
>> errorReply() but not proxyReply(). Is there any special purpose here?
>
> This is not my test, and is really old. All I want to do here is to fix
> the issues with the test. I added 'Connection: close' to okReply since
> you can clearly see that the server/proxy is closing the connection,
> req.orderlyClose().
>
> Really this test should be rewritten to use the com.sun.httpserver, but
> for now I just want it to be able to run reliably.

I see.

-Max

>
> -Chris.
>
>
>> According to my observation on SQE's MS web/proxy server, "Connection:
>> close" and "Proxy-Connection: close" always come with 407 error, and no
>> connection-related header for 401, no matter it's direct web access or
>> thru proxy.
>>
>> BTW, during my testing, I've created a new system property
>> http.pauth.preference for proxy authentication scheme selection. Do you
>> think it has any practical usage?
>>
>> Thanks
>> Max
>>
>>
>> On 06/28/2010 12:16 PM, Weijun Wang wrote:
>>> Hi Chris
>>>
>>> We just had a 1.5-day team building and a weekend, and I just noticed
>>> this mail.
>>>
>>> The disconnectWeb() method was added in "6586707: NTLM authentication
>>> with proxy fails" to support 2-layer NTLM authentications. There was no
>>> regression test to that fix. I'll need a little time to setup an
>>> environment to check if your new code change has any effect on it.
>>>
>>> Thanks
>>> Max
>>>
>>> On 06/23/2010 10:08 PM, Chris Hegarty wrote:
>>>> Hi Michael, Max,
>>>>
>>>> This is a code review request for CR 6954525: Testcase failure
>>>> java/net/Authenticator/B4769350.java.
>>>>
>>>> Essentially, there is a race among requesting threads in the test. The
>>>> threads are making HTTP requests requiring authentication. This is
>>>> fine,
>>>> and what the test is trying to achieve, but some of the final replies
>>>> from the server/proxy closed the connection after they send the
>>>> response
>>>> without notifying the HTTP client. If these connections go back into
>>>> the
>>>> keep alive cache they may cause problems when/if reused by another
>>>> thread. See bug eval section for more details.
>>>>
>>>> The solution is to include the 'Connection: close' header if the
>>>> server/proxy is going to close the connection.
>>>>
>>>> Also, sun.net.www.protocol.http.HttpURLConnection.disconnectWeb should
>>>> check if the connection is to be kept alive before automatically
>>>> resetting.
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~chegar/6954525/webrev.00/webrev/
>>>>
>>>> Thanks,
>>>> -Chris.