Re: HTTP client API

2016-11-01 Thread Tobias Thierer 
On Mon, Oct 31, 2016 at 6:13 PM, Tobias Thierer  wrote:

> On Fri, Oct 28, 2016 at 12:28 PM, Michael McMahon <
> michael.x.mcma...@oracle.com> wrote:
>>
>> 2.) HttpHeaders: I love that there is a type for this abstraction! But:
>>
>>-
>>
>>Why is the type an interface rather than a concrete, final class?
>>Since this is a pure value type, there doesn’t seem to be much point in
>>allowing alternative implementations?
>>
>> You could conceive of different implementations, with different tradeoffs
>> between parsing early or lazily for instance.
>>
>
> What parsing are you referring to?
>

P.S. In case you were referring to lazy HPACK decompression [RFC 7541
]: I'll have to read up the details to
understand if and how much work could be done lazily, but I can see the
consideration. Such a lazy implementation would need some new (potentially
package private) way to be passed the un-processed header data.

Note that RFC 7540 section 4.3
 mandates that at least
some minimal header processing must be done eagerly to verify the
compression and to maintain the state needed for decompression.

For what it's worth, though, to allow other implementations, HttpHeaders
need not be an interface; it should be sufficient for just the method
Map> asMap() to be abstract or overridable? Of course, if
HttpHeaders were shrunk to only that one method, then an interface would be
fine.

Tobias

Re: RFR(S): 8168771: Remove #ifdef AF_INET6 guards in libnet native coding

2016-11-01 Thread Chris Hegarty 
On 26 Oct 2016, at 10:33, Langer, Christoph  wrote:
> 
> Hi,
>  
> please review the cleanup of “#ifdef AF_INET6” in libnet coding. I have 
> opened a separate JIRA issue as requested
>  
> This is a mailthread where it was already discussed: 
> http://mail.openjdk.java.net/pipermail/net-dev/2016-October/010374.html
> Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8168771.0/
> Bug: https://bugs.openjdk.java.net/browse/JDK-8168771

Thank you Christoph, this looks good.

-Chris.

P.S. I ran the patch through our internal build and test system, no surprises.

RFR(s): 8169002: [TESTBUG] Several java/net/httpclient have undeclared dependency on java.logging module

2016-11-01 Thread Sergei Kovalev 

Hello all,

Please review a small fix for tests.

BugID: https://bugs.openjdk.java.net/browse/JDK-8169002
WebRev: http://cr.openjdk.java.net/~skovalev/8169002/webrev.00/

Issue: Several tests from java/net/httpclient folder have undeclared 
dependency on java.logging module. This issue leads the test to fail in 
case module limitation.

Solution: added module declaration into jtreg header and organized imports.

--
With best regards,
Sergei

Re: RFR(s): 8169002: [TESTBUG] Several java/net/httpclient have undeclared dependency on java.logging module

2016-11-01 Thread Roger Riggs 

Hi Sergei,

I think it would be preferable to convert the tests to use System.getLogger.
Is that possible?

Thanks, Roger


On 11/1/2016 1:15 PM, Sergei Kovalev wrote:

Hello all,

Please review a small fix for tests.

BugID: https://bugs.openjdk.java.net/browse/JDK-8169002
WebRev: http://cr.openjdk.java.net/~skovalev/8169002/webrev.00/

Issue: Several tests from java/net/httpclient folder have undeclared 
dependency on java.logging module. This issue leads the test to fail 
in case module limitation.
Solution: added module declaration into jtreg header and organized 
imports.

Re: RFR(s): 8169002: [TESTBUG] Several java/net/httpclient have undeclared dependency on java.logging module

2016-11-01 Thread Daniel Fuchs 

Hi Roger,

On 01/11/16 17:21, Roger Riggs wrote:

Hi Sergei,

I think it would be preferable to convert the tests to use
System.getLogger.
Is that possible?


Some of the tests want to configure the logging, rather
than simply produce traces - so they will need java.logging
to do that:

 670 Logger logger = Logger.getLogger("com.sun.net.httpserver");
 671 ConsoleHandler ch = new ConsoleHandler();
 672 logger.setLevel(Level.ALL);
 673 ch.setLevel(Level.ALL);
 674 logger.addHandler(ch);

It's recommended to use System.Logger to log messages,
but you will have to use java.util.logging if you want to configure
the logging framework. Of course a library shouldn't do that,
but a test is well in its right to configure logging to make
sure the traces will appear in the log.
Unless you do want to run the test in a VM that does not have
java.logging linked in.

cheers,

-- daniel




Thanks, Roger


On 11/1/2016 1:15 PM, Sergei Kovalev wrote:

Hello all,

Please review a small fix for tests.

BugID: https://bugs.openjdk.java.net/browse/JDK-8169002
WebRev: http://cr.openjdk.java.net/~skovalev/8169002/webrev.00/

Issue: Several tests from java/net/httpclient folder have undeclared
dependency on java.logging module. This issue leads the test to fail
in case module limitation.
Solution: added module declaration into jtreg header and organized
imports.

Re: RFR(s): 8169002: [TESTBUG] Several java/net/httpclient have undeclared dependency on java.logging module

2016-11-01 Thread Roger Riggs 

Hi Daniel,

It seemed useful to be able to run the test in as many environments as 
possible

though realistically java.util.logging may be there too.

I don't see that setting the logging levels is intrinsic to the tests 
and would be used

for debugging so perhaps that function can be dropped or configured via the
java.util.logging.config.file system property if/when needed.

$.02, Roger



On 11/1/2016 1:53 PM, Daniel Fuchs wrote:

Hi Roger,

On 01/11/16 17:21, Roger Riggs wrote:

Hi Sergei,

I think it would be preferable to convert the tests to use
System.getLogger.
Is that possible?


Some of the tests want to configure the logging, rather
than simply produce traces - so they will need java.logging
to do that:

 670 Logger logger = Logger.getLogger("com.sun.net.httpserver");
 671 ConsoleHandler ch = new ConsoleHandler();
 672 logger.setLevel(Level.ALL);
 673 ch.setLevel(Level.ALL);
 674 logger.addHandler(ch);

It's recommended to use System.Logger to log messages,
but you will have to use java.util.logging if you want to configure
the logging framework. Of course a library shouldn't do that,
but a test is well in its right to configure logging to make
sure the traces will appear in the log.
Unless you do want to run the test in a VM that does not have
java.logging linked in.

cheers,

-- daniel




Thanks, Roger


On 11/1/2016 1:15 PM, Sergei Kovalev wrote:

Hello all,

Please review a small fix for tests.

BugID: https://bugs.openjdk.java.net/browse/JDK-8169002
WebRev: http://cr.openjdk.java.net/~skovalev/8169002/webrev.00/

Issue: Several tests from java/net/httpclient folder have undeclared
dependency on java.logging module. This issue leads the test to fail
in case module limitation.
Solution: added module declaration into jtreg header and organized
imports.

Re: RFR(s): 8169002: [TESTBUG] Several java/net/httpclient have undeclared dependency on java.logging module

2016-11-01 Thread Daniel Fuchs 

Hi Roger,

I think we agree :-)

On 01/11/16 18:01, Roger Riggs wrote:

Hi Daniel,

It seemed useful to be able to run the test in as many environments as
possible
though realistically java.util.logging may be there too.

I don't see that setting the logging levels is intrinsic to the tests
and would be used
for debugging so perhaps that function can be dropped or configured via the
java.util.logging.config.file system property if/when needed.


For the java.util.logging.config.file system property to work then
you would need java.logging to be linked in - so if you do want
a test to spit out logging traces then you should require java.logging
in @modules - whether you use logging.properties or programmatic
interface to configure logging.

So it all depends on how useful said traces are when investigating
a test failure. If a test is known to fail intermittently and
test failure would be very difficult to analyze without the logging
traces then the test should probably require and configure java.logging
upfront.

Otherwise I agree you might want to remove the useless code, unless
you do want to validate that no NPE or else happen while logging...

best regards,

-- daniel




$.02, Roger



On 11/1/2016 1:53 PM, Daniel Fuchs wrote:

Hi Roger,

On 01/11/16 17:21, Roger Riggs wrote:

Hi Sergei,

I think it would be preferable to convert the tests to use
System.getLogger.
Is that possible?


Some of the tests want to configure the logging, rather
than simply produce traces - so they will need java.logging
to do that:

 670 Logger logger = Logger.getLogger("com.sun.net.httpserver");
 671 ConsoleHandler ch = new ConsoleHandler();
 672 logger.setLevel(Level.ALL);
 673 ch.setLevel(Level.ALL);
 674 logger.addHandler(ch);

It's recommended to use System.Logger to log messages,
but you will have to use java.util.logging if you want to configure
the logging framework. Of course a library shouldn't do that,
but a test is well in its right to configure logging to make
sure the traces will appear in the log.
Unless you do want to run the test in a VM that does not have
java.logging linked in.

cheers,

-- daniel




Thanks, Roger


On 11/1/2016 1:15 PM, Sergei Kovalev wrote:

Hello all,

Please review a small fix for tests.

BugID: https://bugs.openjdk.java.net/browse/JDK-8169002
WebRev: http://cr.openjdk.java.net/~skovalev/8169002/webrev.00/

Issue: Several tests from java/net/httpclient folder have undeclared
dependency on java.logging module. This issue leads the test to fail
in case module limitation.
Solution: added module declaration into jtreg header and organized
imports.

Re: RFR(s): 8169002: [TESTBUG] Several java/net/httpclient have undeclared dependency on java.logging module

2016-11-01 Thread Roger Riggs 

Hi,

Ok, leave the logging in.  (There are currently 3 issues marked as 
intermittent that refer to httpclient).


Roger

p.s.  I'm also a fan of using the TEST.properties for test directives 
that apply to multiple tests

in a directory.  In this case, "modules = java.logging".


On 11/1/2016 2:34 PM, Daniel Fuchs wrote:

Hi Roger,

I think we agree :-)

On 01/11/16 18:01, Roger Riggs wrote:

Hi Daniel,

It seemed useful to be able to run the test in as many environments as
possible
though realistically java.util.logging may be there too.

I don't see that setting the logging levels is intrinsic to the tests
and would be used
for debugging so perhaps that function can be dropped or configured 
via the

java.util.logging.config.file system property if/when needed.


For the java.util.logging.config.file system property to work then
you would need java.logging to be linked in - so if you do want
a test to spit out logging traces then you should require java.logging
in @modules - whether you use logging.properties or programmatic
interface to configure logging.

So it all depends on how useful said traces are when investigating
a test failure. If a test is known to fail intermittently and
test failure would be very difficult to analyze without the logging
traces then the test should probably require and configure java.logging
upfront.

Otherwise I agree you might want to remove the useless code, unless
you do want to validate that no NPE or else happen while logging...

best regards,

-- daniel




$.02, Roger



On 11/1/2016 1:53 PM, Daniel Fuchs wrote:

Hi Roger,

On 01/11/16 17:21, Roger Riggs wrote:

Hi Sergei,

I think it would be preferable to convert the tests to use
System.getLogger.
Is that possible?


Some of the tests want to configure the logging, rather
than simply produce traces - so they will need java.logging
to do that:

 670 Logger logger = 
Logger.getLogger("com.sun.net.httpserver");

 671 ConsoleHandler ch = new ConsoleHandler();
 672 logger.setLevel(Level.ALL);
 673 ch.setLevel(Level.ALL);
 674 logger.addHandler(ch);

It's recommended to use System.Logger to log messages,
but you will have to use java.util.logging if you want to configure
the logging framework. Of course a library shouldn't do that,
but a test is well in its right to configure logging to make
sure the traces will appear in the log.
Unless you do want to run the test in a VM that does not have
java.logging linked in.

cheers,

-- daniel




Thanks, Roger


On 11/1/2016 1:15 PM, Sergei Kovalev wrote:

Hello all,

Please review a small fix for tests.

BugID: https://bugs.openjdk.java.net/browse/JDK-8169002
WebRev: http://cr.openjdk.java.net/~skovalev/8169002/webrev.00/

Issue: Several tests from java/net/httpclient folder have undeclared
dependency on java.logging module. This issue leads the test to fail
in case module limitation.
Solution: added module declaration into jtreg header and organized
imports.

Re: RFR(s): 8169002: [TESTBUG] Several java/net/httpclient have undeclared dependency on java.logging module

2016-11-01 Thread Chris Hegarty 
On 1 Nov 2016, at 18:40, Roger Riggs  wrote:
> 
> Hi,
> 
> Ok, leave the logging in.  (There are currently 3 issues marked as 
> intermittent that refer to httpclient).
> 
> Roger
> 
> p.s.  I'm also a fan of using the TEST.properties for test directives that 
> apply to multiple tests
> in a directory.  In this case, "modules = java.logging”.

If I understand correctly, the list of ‘modules' in TEST.properties is only used
if the test does not explicitly have any @modules tags. So, if you want any 
specific exports, or modules, then you need to specify them all.

-Chris.

> 
> On 11/1/2016 2:34 PM, Daniel Fuchs wrote:
>> Hi Roger, 
>> 
>> I think we agree :-) 
>> 
>> On 01/11/16 18:01, Roger Riggs wrote: 
>>> Hi Daniel, 
>>> 
>>> It seemed useful to be able to run the test in as many environments as 
>>> possible 
>>> though realistically java.util.logging may be there too. 
>>> 
>>> I don't see that setting the logging levels is intrinsic to the tests 
>>> and would be used 
>>> for debugging so perhaps that function can be dropped or configured via the 
>>> java.util.logging.config.file system property if/when needed. 
>> 
>> For the java.util.logging.config.file system property to work then 
>> you would need java.logging to be linked in - so if you do want 
>> a test to spit out logging traces then you should require java.logging 
>> in @modules - whether you use logging.properties or programmatic 
>> interface to configure logging. 
>> 
>> So it all depends on how useful said traces are when investigating 
>> a test failure. If a test is known to fail intermittently and 
>> test failure would be very difficult to analyze without the logging 
>> traces then the test should probably require and configure java.logging 
>> upfront. 
>> 
>> Otherwise I agree you might want to remove the useless code, unless 
>> you do want to validate that no NPE or else happen while logging... 
>> 
>> best regards, 
>> 
>> -- daniel 
>> 
>> 
>>> 
>>> $.02, Roger 
>>> 
>>> 
>>> 
>>> On 11/1/2016 1:53 PM, Daniel Fuchs wrote: 
 Hi Roger, 
 
 On 01/11/16 17:21, Roger Riggs wrote: 
> Hi Sergei, 
> 
> I think it would be preferable to convert the tests to use 
> System.getLogger. 
> Is that possible? 
 
 Some of the tests want to configure the logging, rather 
 than simply produce traces - so they will need java.logging 
 to do that: 
 
  670 Logger logger = Logger.getLogger("com.sun.net.httpserver"); 
  671 ConsoleHandler ch = new ConsoleHandler(); 
  672 logger.setLevel(Level.ALL); 
  673 ch.setLevel(Level.ALL); 
  674 logger.addHandler(ch); 
 
 It's recommended to use System.Logger to log messages, 
 but you will have to use java.util.logging if you want to configure 
 the logging framework. Of course a library shouldn't do that, 
 but a test is well in its right to configure logging to make 
 sure the traces will appear in the log. 
 Unless you do want to run the test in a VM that does not have 
 java.logging linked in. 
 
 cheers, 
 
 -- daniel 
 
 
> 
> Thanks, Roger 
> 
> 
> On 11/1/2016 1:15 PM, Sergei Kovalev wrote: 
>> Hello all, 
>> 
>> Please review a small fix for tests. 
>> 
>> BugID: https://bugs.openjdk.java.net/browse/JDK-8169002 
>> WebRev: http://cr.openjdk.java.net/~skovalev/8169002/webrev.00/ 
>> 
>> Issue: Several tests from java/net/httpclient folder have undeclared 
>> dependency on java.logging module. This issue leads the test to fail 
>> in case module limitation. 
>> Solution: added module declaration into jtreg header and organized 
>> imports. 
>> 
> 
 
>>> 
>> 
>

RFR 8156504/9, java/net/URLPermission/nstest/lookup.sh fails intermittently

2016-11-01 Thread Felix Yang 

Hi there,

please review the following patch for an intermittent failing test. 
Converted it into plain java test and avoid free port anti-pattern.


Bug:

https://bugs.openjdk.java.net/browse/JDK-8156504

Webrev:

http://cr.openjdk.java.net/~xiaofeya/8156504/webrev.00/

Thanks,

Felix

Re: RFR 8156504/9, java/net/URLPermission/nstest/lookup.sh fails intermittently

2016-11-01 Thread Amy Lu 

Good to see one more script test be changed to java, thank you Felix.

I'm not official reviewer, but some minor comments.

  30  * @library /lib/testlibrary
  31  * @build jdk.testlibrary.*
  32  * @compile LookupTest.java
I noticed test requires testlibrary, but seems test do not actually 
depend on that lib, these lines could be removed.


  77 is.close();
Is it better to do the close in the finally block?

  128 if (serverSocket == null) {
  129 serverSocket.close();
  130 }
Typo here?

Even more minor...
-private static void addMappingToHostsFile (String host,
-   String addr,
-   String hostsFileName,
-   boolean append)
- throws Exception {
+private static void addMappingToHostsFile(String host, String addr,
+String hostsFileName, boolean append) throws Exception {
This might be reformatted automatically by IDE, but just feel previous 
one is more easy to read.


Thanks,
Amy


On 11/2/16 10:39 AM, Felix Yang wrote:

Hi there,

please review the following patch for an intermittent failing 
test. Converted it into plain java test and avoid free port anti-pattern.


Bug:

https://bugs.openjdk.java.net/browse/JDK-8156504

Webrev:

http://cr.openjdk.java.net/~xiaofeya/8156504/webrev.00/

Thanks,

Felix