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

[Weijun Wang]

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.

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