Re: Code Review 6954525: Testcase failure java/net/Authenticator/B4769350.java
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.
Re: Code Review 6954525: Testcase failure java/net/Authenticator/B4769350.java
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.
Re: Code Review 6954525: Testcase failure java/net/Authenticator/B4769350.java
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.
hg: jdk7/tl/jdk: 2 new changesets
Changeset: 6d274503d1b7 Author:chegar Date: 2010-06-28 14:55 +0100 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/6d274503d1b7 6954525: Testcase failure java/net/Authenticator/B4769350.java Reviewed-by: michaelm, weijun ! src/share/classes/sun/net/www/protocol/http/HttpURLConnection.java ! test/java/net/Authenticator/B4769350.java Changeset: a89f8c292a5b Author:chegar Date: 2010-06-28 15:06 +0100 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/a89f8c292a5b Merge
Code Review 6954525: java/net/BindException/Test.java 6 tests(s) failed - see log
Michael, java/net/BindException/Test.java and java/net/ipv6tests/Tests.java have code that iterator over the network interfaces looking for IPv4 addresses to test with. In some cases, it is possible to have an interface that is up with an IPv4 address of 0.0.0.0. This is not suitable for use with these test as the implementation uses it as the wildcard address and the tests fail with an unexpected failure. The solution is to simply not use isAnyLocalAddress. Webrev: http://cr.openjdk.java.net/~chegar/6961029/webrev.00/webrev/ -Chris.
Re: Code Review 6954525: java/net/BindException/Test.java 6 tests(s) failed - see log
Chris Hegarty wrote: Michael, java/net/BindException/Test.java and java/net/ipv6tests/Tests.java have code that iterator over the network interfaces looking for IPv4 addresses to test with. In some cases, it is possible to have an interface that is up with an IPv4 address of 0.0.0.0. This is not suitable for use with these test as the implementation uses it as the wildcard address and the tests fail with an unexpected failure. The solution is to simply not use isAnyLocalAddress. Webrev: http://cr.openjdk.java.net/~chegar/6961029/webrev.00/webrev/ -Chris. Looks fine. - Michael
hg: jdk7/tl/jdk: 6961029: java/net/BindException/Test.java should not use wildcard address
Changeset: 7c3da1f0e17c Author:chegar Date: 2010-06-28 20:52 +0100 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/7c3da1f0e17c 6961029: java/net/BindException/Test.java should not use wildcard address Reviewed-by: michaelm, alanb ! test/ProblemList.txt ! test/java/net/BindException/Test.java ! test/java/net/ipv6tests/Tests.java
hg: jdk7/tl/jdk: 6856415: Enabling java security manager will make programe thrown wrong exception ( main method not found )
Changeset: a9e0a6fb6057 Author:ksrini Date: 2010-06-28 18:25 -0700 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/a9e0a6fb6057 6856415: Enabling java security manager will make programe thrown wrong exception ( main method not found ) Reviewed-by: darcy ! src/share/classes/sun/launcher/LauncherHelper.java + test/tools/launcher/VerifyExceptions.java
