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

[Chris Hegarty]

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 
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 
further testing before making your proposed change.

> 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.

-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.