Re: RFR(s): 8169196: [TESTBUG] Three tests from sun/net/www have undeclared dependencies

[Daniel Fuchs]

Hi Sergey,

This looks good now - except for line 223:

 221 Field ntlmSupportedField = 
ntlmProxyClass.getDeclaredField("supported");

 222 ntlmSupportedField.setAccessible(true);
 223 if 
(Boolean.TRUE.equals(ntlmSupportedField.getBoolean(ntlmProxyClass))) {


This should be:

if (ntlmSupportedField.getBoolean(null)) {

best regards,

-- daniel


On 08/11/16 16:50, Sergei Kovalev wrote:

Hi Daniel,

All remarks incorporated into:

http://cr.openjdk.java.net/~skovalev/8169196/webrev.01/

Also I've added a note to the
https://bugs.openjdk.java.net/browse/JDK-8038079

Re: RFR(s): 8169196: [TESTBUG] Three tests from sun/net/www have undeclared dependencies

[Sergei Kovalev]

Hi Daniel,

All remarks incorporated into:

http://cr.openjdk.java.net/~skovalev/8169196/webrev.01/

Also I've added a note to the 
https://bugs.openjdk.java.net/browse/JDK-8038079


--
With best regards,
Sergei


04.11.16 14:20, Daniel Fuchs wrote:

Hi Sergey,

Yes - sorry - I misunderstood the purpose of the NoNTLM test.

The test is supposed to run *only* when NTLM *is not* supported.
When NTLM is supported, it is supposed to return without doing
anything.

Now as it happens, NTLM is supported by default in java.base.
The only case where NTLM is not supported is if some profile
strip down the binaries to exclude the package
sun.net.www.protocol.http.ntlm
(which seems to be the reason why java.base uses reflection to
access the classes in sun.net.www.protocol.http.ntlm).
Basically, this means that NTLM is not supported when
sun.net.www.protocol.http.NTLMAuthenticationProxy.supported == false.

Somehow the test assumes that this will happen if and only if
the java.security.jgss is not linked in (which I think is an obsolete
assertion, because now with --limit-mods and jlink you can exclude
java.security.jgss without necessarily running on a profile
where the binaries have been stripped).

If you add @modules jdk.security.auth to test, then what will happen
is that the test will only run when jdk.security.auth is linked
in, which in turn implies that java.security.jgss is also linked in
because jdk.security.auth requires java.security.jgss.
So the test will either not be run (when jdk.security.auth is
not linked in), or return immediately without doing anything
(when jdk.security.auth is linked in) - so if you add
@modules jdk.security.auth to test, the test serves no purpose
and it's equivalent to simply delete it.

So what you need to do here is replace:

 218 // assume NTLM is not supported when Kerberos is not 
available

 219 try {
 220 Class.forName("javax.security.auth.kerberos.KerberosPrincipal");
 221 System.out.println("Kerberos is present, assuming 
NTLM is supported too");

 222 return;
 223 } catch (ClassNotFoundException okay) { }


with something that mimics what java.base does to figure whether
NTLM is supported:
load sun.net.www.protocol.http.NTLMAuthenticationProxy,
through reflection (use the 3 args Class.forName to make
sure the static initializers are run), then use reflection
to access the "supported" field of that class, make it
accessible (Field.setAccessible), get its value,
and return if  its value is true.
We need to use reflection here because NTLMAuthenticationProxy
is a package class (it is not public).
Then instead of adding @module jdk.security.auth which is a
nonsense, add @module java.base/sun.net.www.protocol.http
to grant the test the power to break in through the strong
encapsulation for testing purposes.

It would also probably be good to log a new defect asking
for this code to be revisited - or drop a note in
https://bugs.openjdk.java.net/browse/JDK-8038079
pointing to this test and mentioning that a better
entry point to figure out whether NTLM is supported or not
might be desirable.

best regards,

-- daniel

On 03/11/16 15:43, Sergei Kovalev wrote:

Daniel,

I case I remove lines 218-223 and disable jdk.security.auth module the
test fails with an Exception:

Expect client to choose: Basic
HTTP/1.1 401 Unauthorized
Content-Length: 0
Connection: close
WWW-Authenticate: Basic realm="wallyworld"
WWW-Authenticate: NTLM


Server received Authorization header: NTLM
TlRMTVNTUAABB4IIAAA=
STDERR:
java.lang.RuntimeException: Unexpected value
at NoNTLM.test(NoNTLM.java:174)
at NoNTLM.main(NoNTLM.java:229)
at
jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(java.base@9-ea/Native 


Method)
at
jdk.internal.reflect.NativeMethodAccessorImpl.invoke(java.base@9-ea/NativeMethodAccessorImpl.java:62) 



at
jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(java.base@9-ea/DelegatingMethodAccessorImpl.java:43) 



at 
java.lang.reflect.Method.invoke(java.base@9-ea/Method.java:537)

at
com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:110) 



at java.lang.Thread.run(java.base@9-ea/Thread.java:844)

JavaTest Message: Test threw exception: java.lang.RuntimeException:
Unexpected value
JavaTest Message: shutting down test

Looks like the module also requires for www authentication in other pice
of code.


Hi Sergey,

Great to get rid of yet another shell script.

The change looks good in general - except for NoNTLM.java:

I don't think the test should have @modules jdk.security.auth
as the test is precisely supposed to be able to run without it
(+ lines 218-223 are probably obsolete and I suspect they should
   be removed - but that is for another day).

best regards,

-- daniel

On 03/11/16 13:48, Sergei Kovalev wrote:

Hi Team,

Please review one more small fix for module dependencies issue.

Bug ID: 

Re: httpserver does not close connections when RejectedExecutionException occurs

[KUBOTA Yuji]

Hi Chris,

If I can add reproduce code as test, could you please teach me a hint to add it?

Thanks,
Yuji.

2016-11-08 18:53 GMT+09:00 KUBOTA Yuji :
> Hi Chris,
>
> Thank you for your review and updating this issues on JBS.
>
> I filed an issue:
> https://bugs.openjdk.java.net/browse/JDK-8169358
>
> I don't assign to me because I'm not a committer.
>
> 2016-11-08 0:28 GMT+09:00 Chris Hegarty :
>>> * patch
>>> diff --git
>>> a/src/jdk.httpserver/share/classes/sun/net/httpserver/ServerImpl.java
>>> b/src/jdk.httpserver/share/classes/sun/net/httpserver/ServerImpl.java
>>> --- a/src/jdk.httpserver/share/classes/sun/net/httpserver/ServerImpl.java
>>> +++ b/src/jdk.httpserver/share/classes/sun/net/httpserver/ServerImpl.java
>>> @@ -442,10 +442,13 @@
>>>  logger.log (Level.TRACE, "Dispatcher (4)", e1);
>>>  closeConnection(conn);
>>>  } catch (IOException e) {
>>>  logger.log (Level.TRACE, "Dispatcher (5)", e);
>>>  closeConnection(conn);
>>> +} catch (RejectedExecutionException e) {
>>> +logger.log (Level.TRACE, "Dispatcher (9)", e);
>>> +closeConnection(conn);
>>>  }
>>>  }
>>>  }
>>> _
>>>  static boolean debug = ServerConfig.debugEnabled ();
>>
>>
>> This looks ok. I wonder if some of these exceptions could be refactored
>> into a catch clause with several exception types?
>
> Yes, I agree to keep simple. I updated my patch as below.
> http://cr.openjdk.java.net/~ykubota/8169358/webrev.01/
> Could you review it again?
>
> Thank you,
> Yuji
>>
>> -Chris.
>>
>>
>>
>>> * steps to reproduce
>>>  1. java -Djava.util.logging.config.file=logging.properties
>>> SmallHttpServer
>>>  2. post tcp connections by curl or other ways
>>>  e.g.: while true; do curl -XPOST --noproxy 127.0.0.1
>>> http://127.0.0.1:8080/; done
>>>  3. wait RejectedExecutionException occurs as below and then
>>> SmallHttpServer stops by this issue.
>>> 
>>> Nov 05, 2016 12:01:48 PM sun.net.httpserver.ServerImpl$Dispatcher run
>>> FINER: Dispatcher (7)
>>> java.util.concurrent.RejectedExecutionException: Task
>>> sun.net.httpserver.ServerImpl$Exchange@37b50d9e rejected from
>>> java.util.concurrent.ThreadPoolExecutor@1b3178d4[Running, pool size =
>>> 1, active threads = 0, queued tasks = 0, completed tasks = 7168]
>>> at
>>> java.util.concurrent.ThreadPoolExecutor$AbortPolicy.rejectedExecution(java.base/ThreadPoolExecutor.java:2076)
>>> at
>>> java.util.concurrent.ThreadPoolExecutor.reject(java.base/ThreadPoolExecutor.java:842)
>>> at
>>> java.util.concurrent.ThreadPoolExecutor.execute(java.base/ThreadPoolExecutor.java:1388)
>>> at
>>> sun.net.httpserver.ServerImpl$Dispatcher.handle(jdk.httpserver/ServerImpl.java:440)
>>> at
>>> sun.net.httpserver.ServerImpl$Dispatcher.run(jdk.httpserver/ServerImpl.java:405)
>>> at java.lang.Thread.run(java.base/Thread.java:844)
>>> (SmallHttpServer is stopping by not closing socket)
>>> 
>>>
>>> *logging.properties
>>> handlers = java.util.logging.ConsoleHandler
>>> com.sun.net.httpserver.level = FINEST
>>> java.util.logging.ConsoleHandler.level = FINEST
>>> java.util.logging.ConsoleHandler.formatter =
>>> java.util.logging.SimpleFormatter
>>>
>>> * SmallHttpServer.java
>>> import com.sun.net.httpserver.HttpExchange;
>>> import com.sun.net.httpserver.HttpHandler;
>>> import com.sun.net.httpserver.HttpServer;
>>>
>>> import java.net.InetSocketAddress;
>>> import java.util.Scanner;
>>> import java.util.concurrent.LinkedBlockingQueue;
>>> import java.util.concurrent.ThreadPoolExecutor;
>>> import java.util.concurrent.TimeUnit;
>>>
>>> public class SmallHttpServer {
>>>
>>> public static void main(String[] args) throws Exception {
>>> int POOL_SIZE = 1;
>>> String HOST = args.length < 1 ? "127.0.0.1" : args[0];
>>> int PORT = args.length < 2 ? 8080 : Integer.valueOf(args[1]);
>>>
>>> // Setup a minimum thread pool to rise
>>> RejectExecutionException in httpserver
>>> ThreadPoolExecutor miniHttpPoolExecutor
>>> = new ThreadPoolExecutor(POOL_SIZE, POOL_SIZE, 0L,
>>> TimeUnit.MICROSECONDS,
>>> new LinkedBlockingQueue<>(1), (Runnable r) -> {
>>> return new Thread(r);
>>> });
>>> HttpServer httpServer = HttpServer.create(new
>>> InetSocketAddress(HOST, PORT), 0);
>>> httpServer.setExecutor(miniHttpPoolExecutor);
>>>
>>> HttpHandler res200handler = (HttpExchange exchange) -> {
>>> exchange.sendResponseHeaders(200, 0);
>>> exchange.close();
>>> };
>>> httpServer.createContext("/", res200handler);
>>>
>>> httpServer.start();
>>> // Wait stdin to exit
>>> Scanner in = new Scanner(System.in);
>>> while (in.hasNext()) {
>>>