Exceptions in Iterator.forEachRemaining()

[Zheka Kozlov]

Hi everyone.

I noticed that different Iterator implementations behave differently when
an Exception is thrown inside a Consumer passed to forEachRemaining():

private static void test(List list) {
Iterator iterator = list.iterator();
try {
iterator.forEachRemaining(i -> {
if (i == 3) {
throw new RuntimeException();
} else {
System.out.print(i);
}
});
} catch (RuntimeException ex) {
}
iterator.forEachRemaining(System.out::print);
}

public static void main(String... args) throws Throwable {
test(List.of(1, 2, 3, 4, 5)); // Prints 1245
System.out.println();
test(Arrays.asList(1, 2, 3, 4, 5)); // Prints 1245
System.out.println();
test(new LinkedList<>(List.of(1, 2, 3, 4, 5))); // Prints 12345 (BAD)
System.out.println();
test(new ArrayList<>(List.of(1, 2, 3, 4, 5))); // Prints 1212345 (BAD)
}

Is this a bug? I think yes because an overridden forEachRemaining() should
behave the same as the default implementation:

while (hasNext()) {
action.accept(next());
}

So, in this case, List.of() and Arrays.asList() are correct while
LinkedList and ArrayList are not.

Re: JDK 12 RFR of JDK-8212081: AnnotatedType.toString implementation don't print annotations on embedded types

[joe darcy]

PS Re-refined implementation at

    http://cr.openjdk.java.net/~darcy/8212081.1/

The implementation now elides "extends java.lang.Object" in "? extends 
java.lang.Object" if Object is not annotated and there are no other bounds.


The tests were updated to cover this situation too.

Thanks,

-Joe


On 10/16/2018 7:26 PM, Werner Dietl wrote:

Hi Joe,

thanks for fixing this! I like the improved testing approach.
Changes look good to me, but I'm not a reviewer.

Best,
cu, WMD.
On Mon, Oct 15, 2018 at 2:11 AM joe darcy  wrote:

Follow-up fix developed; details below.

On 10/11/2018 12:12 PM, joe darcy wrote:

Hi Werner,

On 10/10/2018 1:23 PM, Werner Dietl wrote:

Hi Joe, all,

the logic looks good to me.

In the tests I'm wondering whether to include an annotated wildcard
bound. There is:

307 public @AnnotType(11) Set<@AnnotType(13) ? extends Number>
fooNumberSet2() {return null;}

but nothing like

Set fooNumberSet2() {return null;}

I wouldn't expect problems for this, but a test would exercise some of
the code that gets added.

You were correct to probe at the bounds on the wildcard components;
the code that was reviewed and pushed does not print annotations on
the bounds.

I'll work on an update to handle this and similar cases where there
are embedded types that could have annotations.


Please review the fix for

  JDK-8212081 : AnnotatedType.toString implementation don't print
annotations on embedded types
  http://cr.openjdk.java.net/~darcy/8212081.1/

A few notes on the test, the main structural change is that information
about the expected properties of the toString form of the AnnotatedType
of a method's return type is stored in a *declaration annotation* on the
method. The presence of the expected number of type annotations on the
full string of the AnnotatedType can thus be directly tested.

Thanks,

-Joe

Re: JDK 12 RFR of JDK-6304578: (reflect) toGenericString fails to print bounds of type variables on generic methods

[joe darcy]

Hi Peter,

Coming back to this review after my Code One activities this year have 
run their course...



On 10/17/2018 3:07 PM, Peter Levart wrote:

Hi Joe,

On 10/17/2018 09:16 PM, joe darcy wrote:
PS In response to some off-list feedback, an updated webrev uses a 
stream-ier implementation:


http://cr.openjdk.java.net/~darcy/6304578.1/


I suggest further niceing:

- if you make typeVarBounds() static, you could use it in Stream.map() 
as a method reference.
- in Class.java line 285, you could also use Type::getTypeName method 
reference
- in Class.java line 269, wouldn't the output be nicer if the 
delimiter in joining collector was ", " instead of "," (with a space 
after comma)?


The same for Executable.



Revised webrev generally taking those suggestions up at:

    http://cr.openjdk.java.net/~darcy/6304578.2/

Diff of relevant parts of prior version below (screening out unrelated 
changes to Class made in the interim).


The behavior with respect to using a comma as a separator is part of the 
spec of these methods and I don't want to alter that as part of this change.


Thanks,

-Joe

diff -r 6304578.1/raw_files/new/  6304578.2/raw_files/new/
diff -r 
6304578.1/raw_files/new/src/java.base/share/classes/java/lang/Class.java 
6304578.2/raw_files/new/src/java.base/share/classes/java/lang/Class.java

268c268
< sb.append(Stream.of(typeparms).map(t -> typeVarBounds(t)).
---
> sb.append(Stream.of(typeparms).map(Class::typeVarBounds).
279c279
< String typeVarBounds(TypeVariable typeVar) {
---
> static String typeVarBounds(TypeVariable typeVar) {
285c285
< Stream.of(bounds).map(e -> e.getTypeName()).
---
> Stream.of(bounds).map(Type::getTypeName).
3413c3413,3414
< StringJoiner sj = new StringJoiner(", ", getName() + "." + 
name + "(", ")");

---
> StringBuilder sb = new StringBuilder();
> sb.append(getName() + "." + name + "(");
3415,3420c3416,3420
< for (int i = 0; i < argTypes.length; i++) {
< Class c = argTypes[i];
< sj.add((c == null) ? "null" : c.getName());
< }
< }
< return sj.toString();
---
> Stream.of(argTypes).map(c -> {return (c == null) ? "null" : 
c.getName();}).

> collect(Collectors.joining(","));
> }
> sb.append(")");
> return sb.toString();
diff -r 
6304578.1/raw_files/new/src/java.base/share/classes/java/lang/reflect/Executable.java 
6304578.2/raw_files/new/src/java.base/share/classes/java/lang/reflect/Executable.java

116c116
< sb.append(Stream.of(parameterTypes).map(p -> p.getTypeName()).
---
> sb.append(Stream.of(parameterTypes).map(Type::getTypeName).
122c122
< sb.append(Stream.of(exceptionTypes).map(e -> e.getTypeName()).
---
> sb.append(Stream.of(exceptionTypes).map(Type::getTypeName).
137c137
< String typeVarBounds(TypeVariable typeVar) {
---
> static String typeVarBounds(TypeVariable typeVar) {
143c143
< Stream.of(bounds).map(e -> e.getTypeName()).
---
> Stream.of(bounds).map(Type::getTypeName).
156c156
< sb.append(Stream.of(typeparms).map(t -> typeVarBounds(t)).
---
> sb.append(Stream.of(typeparms).map(Executable::typeVarBounds).
176,180c176,177
< StringJoiner joiner = new StringJoiner(",", " throws 
", "");

< for (Type exceptionType : exceptionTypes) {
< joiner.add(exceptionType.getTypeName());
< }
< sb.append(joiner.toString());
---
> sb.append(Stream.of(exceptionTypes).map(Type::getTypeName).
>   collect(Collectors.joining(",", " throws ", "")));

Re: RFR: 8139965 - Hang seen when using com.sun.jndi.ldap.search.replyQueueSize

[Rob McKenna]

Thanks Daniel:

http://cr.openjdk.java.net/~robm/8139965/webrev.03/

I'm planning to follow up on the test side of things with a separate
bug. I think the technique used in some of the recent SQE LDAP tests
might be applicable.

-Rob

On 05/09/18 09:53, Daniel Fuchs wrote:
> Hi Rob,
> 
> That looks better but I believe that:
> 
> 1. closed should be volatile since it's read from outside
>synchronized block
> 
> 2. it seems there might be a race where the last response
>received could be dropped, if the connection is closed
>just after it's been pulled from the queue.
> 
> So I would suggest exchanging:
> 
>  115 if (isClosed()) {
>  116 return null;
>  117 }
>  118
>  119 return result;
> 
> with:
> 
>  return result == EOF ? null : result;
> 
> best regards,
> 
> -- daniel
> 
> On 05/09/2018 02:05, Rob McKenna wrote:
> > Thanks for the reviews folks.
> > 
> > I believe the following captures your recommended changes:
> > 
> > http://cr.openjdk.java.net/~robm/8139965/webrev.02/
> > 
> > W.r.t. testing I think this area has been difficult to test
> > traditionally. I'll have a dig through the existing testbase (and I'll
> > get back to you) to see if there's anything similar but afaik most tests
> > simply mimic a bindable dummy ldap server.
> > 
> > Vyom, are you aware of any more rigorous tests / ldap test frameworks?
> > 
> >  -Rob
> > 
> > On 04/09/18 10:22, Daniel Fuchs wrote:
> > > Hi Rob,
> > > 
> > > I concur with Chris.
> > > completed needs to be volatile and close() needs to
> > > set a flag and use offer like cancel().
> > > 
> > > The condition for testing for closed then becomes
> > > that the flag is set and the queue is empty or has EOF
> > > as its head.
> > > 
> > > Is there any way this could be tested by a regression
> > > test?
> > > 
> > > best regards,
> > > 
> > > -- daniel
> > > 
> > > On 04/09/2018 10:00, Chris Hegarty wrote:
> > > > Rob,
> > > > 
> > > > > On 3 Sep 2018, at 22:48, Rob McKenna  wrote:
> > > > > 
> > > > > Hi folks,
> > > > > 
> > > > > I'd like to get a re-review of this change:
> > > > > 
> > > > > https://bugs.openjdk.java.net/browse/JDK-8139965 
> > > > > 
> > > > 
> > > > This issue is closed as `will not fix`. I presume you will re-open it 
> > > > before pushing.
> > > > 
> > > > > http://cr.openjdk.java.net/~robm/8139965/webrev/ 
> > > > > 
> > > > 
> > > > 
> > > > 43 private boolean completed;
> > > > Won’t `completed` need to be volatile now? ( since the removal of 
> > > > synchronized from hasSearchCompleted )
> > > > 
> > > > LdapRequest.close puts EOF into the queue, but that is a potentially 
> > > > blocking operation ( while holding the lock ). If the queue is at 
> > > > capacity, then it will block forever. This model will only work if 
> > > > `getReplyBer` is always guaranteed to be running concurrently. Is it?
> > > > 
> > > > Please check the indentation of LdapRequest.java L103 ( in
> > > > the new file ). It appears, in the webrev, that the trailing `}` is
> > > > not lined up.
> > > > 
> > > > The indentation doesn’t look right here either.
> > > >624 if (nparent) {
> > > >625 LdapRequest ldr = pendingRequests;
> > > >626 while (ldr != null) {
> > > >627 ldr.close();
> > > >628 ldr = ldr.next;
> > > >629 }
> > > >630 }
> > > >631 }
> > > > 
> > > > -Chris
> > > > 
> > > 
> 

Re: RFR: JDK-8212828 Allow POSIX_SPAWN to be used for ProcessImpl on Linux

[Roger Riggs]

Hi Thomas,

In an abundance of caution, I was thinking that it would be a change right
at the beginning of a new release so it gets the most exercise and
users in early access, etc.

And before that it needs to be put into more regular usage and
some more unusual environments. The default is currently selected
by virtual of being first in the argument list; that doesn't lend itself
to being configurable with either configure or make arguments.

Roger


On 10/25/2018 06:33 AM, Thomas Stüfe wrote:

Hi all,

the more I mull over this, the more I would prefer to do the jump for
real and attempt switch the default to posix_spawn() for Linux.

We have theoretically established that both glibc down to 2.4 and
muslc since always did "the right thing".

We still have time in the 12 time line to test this thoroughly.

What do you think?

Thanks, Thomas

On Wed, Oct 24, 2018 at 10:59 PM Thomas Stüfe  wrote:

Hi Roger,

On Wed 24. Oct 2018 at 21:39, Roger Riggs  wrote:

Hi Thomas,

Sorry, I had the incantation for multiple tests wrong.
Separate test configurations need to be in separate /* */ comment blocks.
BTW, it creates separate .jtr files for each block.

diff --git a/test/jdk/java/lang/ProcessBuilder/Basic.java 
b/test/jdk/java/lang/ProcessBuilder/Basic.java
--- a/test/jdk/java/lang/ProcessBuilder/Basic.java
+++ b/test/jdk/java/lang/ProcessBuilder/Basic.java
@@ -33,8 +33,10 @@
   * @modules java.base/java.lang:open
   * @run main/othervm/timeout=300 Basic
   * @run main/othervm/timeout=300 -Djdk.lang.Process.launchMechanism=fork Basic
- *
+ */
+/*
   * @test
+ * @modules java.base/java.lang:open
   * @requires (os.family == "linux")
   * @run main/othervm/timeout=300 
-Djdk.lang.Process.launchMechanism=posix_spawn Basic
   * @author Martin Buchholz


As for the build configuration being out of scope...

I don't want to see part of the work done and then the rest forgotten or
worse yet someone comes along in the future and incorrectly believes that
posix_spawn is ready for production and starts filing bugs or getting bit by 
something.
There's a lot more testing to do before it could be come the default
and perhaps a significant warning should be attached to the code so it does not 
get forgotten.

I agree, we ideally should not roll out half tested changes. But where does 
that leave us wrt this patch?

We could just not do it, requiring anyone willing to do the extensive testing 
necessary to switch the default to posix-spawn to apply this change first 
locally. This is not an insurmountable amount of work, especially since the 
base is quite static, so the patch won’t bit rot easily.

We could push the patch in its current form, plus a large source comment? But 
then. Users do not read comments. A warning on stderr? Would play havoc with 
many tests parsing stderr.

Do you have an idea how to proceed?

Thanks, Thomas


Regards, Roger


On 10/24/18 1:53 PM, Thomas Stüfe wrote:

Hi Roger,

On Wed, Oct 24, 2018 at 5:23 PM Roger Riggs  wrote:

Hi Thomas,

Thanks for adding the test.

There's a feature of jtreg that was exposed a couple of month ago that
can be used to deal with running the test too many times.

There can be multiple @test blocks with different @requires.

* @run main/othervm/timeout=300 Basic * @run main/othervm/timeout=300
-Djdk.lang.Process.launchMechanism=fork Basic * * @test * @requires
(os.family == "linux") * @run main/othervm/timeout=300
-Djdk.lang.Process.launchMechanism=posix_spawn Basic


Does not seem to work, sorry:

   24 /*
   25  * @test
   26  * @bug 4199068 4738465 4937983 4930681 4926230 4931433 4932663 4986689
   27  *  5026830 5023243 5070673 4052517 4811767 6192449 6397034 6413313
   28  *  6464154 6523983 6206031 4960438 6631352 6631966 6850957 6850958
   29  *  4947220 7018606 7034570 4244896 5049299 8003488 8054494 8058464
   30  *  8067796
   31  * @key intermittent
   32  * @summary Basic tests for Process and Environment Variable code
   33  * @modules java.base/java.lang:open
   34  * @run main/othervm/timeout=300 Basic
   35  * @run main/othervm/timeout=300
-Djdk.lang.Process.launchMechanism=fork Basic
   36  * @test
   37  * @requires (os.family == "liinux")
   38  * @run main/othervm/timeout=300
-Djdk.lang.Process.launchMechanism=posix_spawn Basic
   39  * @author Martin Buchholz
   40  */

If I have @requires (os.family == "linux") all three variants are executed - ok.

Then I wanted to have a negative test, so I misnamed linux as "liinux"
and would have expected the first @test block to fire, the second to
be ignored. But in fact:

thomas@mainframe /shared/projects/openjdk/jtreg $
../jtreg-prebuilt/jtreg/bin/jtreg -jdk
../jdk-jdk/output-slowdebug/images/jdk/
../jdk-jdk/source/test/jdk/java/lang/ProcessBuilder/Basic.java
Test results: no tests selected

So to me it looks like as if the @requires tag is valid for both @test
blocks, not only the one it appears in.

I'll run this through our build system too.

To fully test out using posix_spawn in man

Re: RFR 8212129: Remove finalize methods from java.util.zip.ZipFIle/Inflator/Deflator

[Xueming Shen]

+1

A potential "enhancement" for future consideration is whether or not worth
to combine DeflaterZFStreamRef/InflaterZFStream back to one class, maybe
with a "instanceof" before calling Deflater/Inflater.end(addr).

-Sherman

On 10/24/18, 10:19 AM, Lance Andersen wrote:

Hi all,

This change removes the finalize methods from 
java.util.zip.ZipFIle/Inflator/Deflator.  These methods were deprecated and  
marked for removal in JDK 9

The webrev can be found at: 
http://cr.openjdk.java.net/~lancea/8212129/webrev.00/

The relevant JCK tests for zip and jar continue to run clean as do the JDK 
tier1 - tier3 tests

The CSR has also been finalized

Best,
Lance




  
    

  Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com

Re: RFR: JDK-8212828 Allow POSIX_SPAWN to be used for ProcessImpl on Linux

[David Lloyd]

I'm in favor, for whatever that's worth.
On Thu, Oct 25, 2018 at 5:33 AM Thomas Stüfe  wrote:
>
> Hi all,
>
> the more I mull over this, the more I would prefer to do the jump for
> real and attempt switch the default to posix_spawn() for Linux.
>
> We have theoretically established that both glibc down to 2.4 and
> muslc since always did "the right thing".
>
> We still have time in the 12 time line to test this thoroughly.
>
> What do you think?
>
> Thanks, Thomas
>
> On Wed, Oct 24, 2018 at 10:59 PM Thomas Stüfe  wrote:
> >
> > Hi Roger,
> >
> > On Wed 24. Oct 2018 at 21:39, Roger Riggs  wrote:
> >>
> >> Hi Thomas,
> >>
> >> Sorry, I had the incantation for multiple tests wrong.
> >> Separate test configurations need to be in separate /* */ comment blocks.
> >> BTW, it creates separate .jtr files for each block.
> >>
> >> diff --git a/test/jdk/java/lang/ProcessBuilder/Basic.java 
> >> b/test/jdk/java/lang/ProcessBuilder/Basic.java
> >> --- a/test/jdk/java/lang/ProcessBuilder/Basic.java
> >> +++ b/test/jdk/java/lang/ProcessBuilder/Basic.java
> >> @@ -33,8 +33,10 @@
> >>   * @modules java.base/java.lang:open
> >>   * @run main/othervm/timeout=300 Basic
> >>   * @run main/othervm/timeout=300 -Djdk.lang.Process.launchMechanism=fork 
> >> Basic
> >> - *
> >> + */
> >> +/*
> >>   * @test
> >> + * @modules java.base/java.lang:open
> >>   * @requires (os.family == "linux")
> >>   * @run main/othervm/timeout=300 
> >> -Djdk.lang.Process.launchMechanism=posix_spawn Basic
> >>   * @author Martin Buchholz
> >>
> >>
> >> As for the build configuration being out of scope...
> >>
> >> I don't want to see part of the work done and then the rest forgotten or
> >> worse yet someone comes along in the future and incorrectly believes that
> >> posix_spawn is ready for production and starts filing bugs or getting bit 
> >> by something.
> >> There's a lot more testing to do before it could be come the default
> >> and perhaps a significant warning should be attached to the code so it 
> >> does not get forgotten.
> >
> >
> > I agree, we ideally should not roll out half tested changes. But where does 
> > that leave us wrt this patch?
> >
> > We could just not do it, requiring anyone willing to do the extensive 
> > testing necessary to switch the default to posix-spawn to apply this change 
> > first locally. This is not an insurmountable amount of work, especially 
> > since the base is quite static, so the patch won’t bit rot easily.
> >
> > We could push the patch in its current form, plus a large source comment? 
> > But then. Users do not read comments. A warning on stderr? Would play havoc 
> > with many tests parsing stderr.
> >
> > Do you have an idea how to proceed?
> >
> > Thanks, Thomas
> >
> >>
> >> Regards, Roger
> >>
> >>
> >> On 10/24/18 1:53 PM, Thomas Stüfe wrote:
> >>
> >> Hi Roger,
> >>
> >> On Wed, Oct 24, 2018 at 5:23 PM Roger Riggs  wrote:
> >>
> >> Hi Thomas,
> >>
> >> Thanks for adding the test.
> >>
> >> There's a feature of jtreg that was exposed a couple of month ago that
> >> can be used to deal with running the test too many times.
> >>
> >> There can be multiple @test blocks with different @requires.
> >>
> >> * @run main/othervm/timeout=300 Basic * @run main/othervm/timeout=300
> >> -Djdk.lang.Process.launchMechanism=fork Basic * * @test * @requires
> >> (os.family == "linux") * @run main/othervm/timeout=300
> >> -Djdk.lang.Process.launchMechanism=posix_spawn Basic
> >>
> >>
> >> Does not seem to work, sorry:
> >>
> >>   24 /*
> >>   25  * @test
> >>   26  * @bug 4199068 4738465 4937983 4930681 4926230 4931433 4932663 
> >> 4986689
> >>   27  *  5026830 5023243 5070673 4052517 4811767 6192449 6397034 
> >> 6413313
> >>   28  *  6464154 6523983 6206031 4960438 6631352 6631966 6850957 
> >> 6850958
> >>   29  *  4947220 7018606 7034570 4244896 5049299 8003488 8054494 
> >> 8058464
> >>   30  *  8067796
> >>   31  * @key intermittent
> >>   32  * @summary Basic tests for Process and Environment Variable code
> >>   33  * @modules java.base/java.lang:open
> >>   34  * @run main/othervm/timeout=300 Basic
> >>   35  * @run main/othervm/timeout=300
> >> -Djdk.lang.Process.launchMechanism=fork Basic
> >>   36  * @test
> >>   37  * @requires (os.family == "liinux")
> >>   38  * @run main/othervm/timeout=300
> >> -Djdk.lang.Process.launchMechanism=posix_spawn Basic
> >>   39  * @author Martin Buchholz
> >>   40  */
> >>
> >> If I have @requires (os.family == "linux") all three variants are executed 
> >> - ok.
> >>
> >> Then I wanted to have a negative test, so I misnamed linux as "liinux"
> >> and would have expected the first @test block to fire, the second to
> >> be ignored. But in fact:
> >>
> >> thomas@mainframe /shared/projects/openjdk/jtreg $
> >> ../jtreg-prebuilt/jtreg/bin/jtreg -jdk
> >> ../jdk-jdk/output-slowdebug/images/jdk/
> >> ../jdk-jdk/source/test/jdk/java/lang/ProcessBuilder/Basic.java
> >> Test results: no tests selected
> >>
> >> So to me it looks like as if the @requ

[RFR] 8160768: Add capability to custom resolve host/domain names within the default JDNI LDAP provider

[Rob McKenna]

This recently received CSR approval, so it seems like a good time to pick
the codereview up again:

http://cr.openjdk.java.net/~robm/8160768/webrev.08/

Referencing:

http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-December/050794.html

1) I'm copying the behaviour from
LdapCtxFactory.getInitialContext(Hashtable) where an empty String is
the default value used when the provider url is null.

I don't think HostnameChecker (by way of SNIHostname) will accept either
empty or null values when doing the comparison.

Somewhat oddly however, LdapCtx.extendedOperation(ExtendedRequest)
appears to pass the String "null" to
StartTlsResponseImpl.setConnection(Connection, String), which attempts
to substitute null values with the Connections host so there may be a bug
here.

I'm happy to allow null returns here if necessary. Sean, can you
comment on the distinction between null & "" as far as hostname
verification is concerned?

2) In the latest iteration lookupEndpoints() returns an
Optional. Currently neither getEndpoints() nor
getDomainName() can be null. (they can be an empty list and/or an empty
String however)

3) Corrected.

4) See https://www.oracle.com/technetwork/java/seccodeguide-139067.html#4-5

-Rob

Re: Review Request JDK-8212948: Remove unused jdk.internal.misc.VMNotification interface

[Mandy Chung]

On 10/24/18 11:12 PM, Alan Bateman wrote:

On 24/10/2018 23:17, Mandy Chung wrote:

Remove a leftover file from JDK-8159590 that removed
jdk.internal.misc.VM.registerVMNotification method taking
VMNotification parameter.

$ hg remove 
src/java.base/share/classes/jdk/internal/misc/VMNotification.java
Looks okay to me. I assume you've checked that there aren't any tests 
using it.


Yes, I checked and don't find any.

Mandy

Re: Time-zone database issues

[Naoto Sato]

Thanks, Stephen.

I filed an issue for your suggestion:

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

I will need to look into the issue, but so far as I understand, will it 
be fine to modify the offending transition date to the next day for 
2018f's immediate issue?


Naoto

On 10/22/18 3:25 PM, Stephen Colebourne wrote:

The IANA time-zone database [1] provides details of how time-zones
change over time. The latest release - 2018f - cannot be processed
successfully by the current JDK parser [2].  A workaround exists
however unlike previous cases of tzdb incompatibility, this time it
makes sense for the JDK parser and API to be changed.

Problem
---
The JDK parser and API make the assumption that a time-zone change can
occur at any LocalTime or midnight-end-of-day, ie. from 00:00 to
24:00. Unfortunately, the tzdb source files allow (and now include)
rules outside those valid values, in this case a value of 25:00.
Specifically, the rule that causes problems says that clocks change at
25:00 on the first Saturday on or after the 8th of September.

In the current problematic case, the rule can be rewritten to say that
clocks change at 01:00 on the first Sunday on or after the 9th of
September. However, there are cases where it is difficult to
impossible to rewrite the rule (such as 25:00 on the last Saturday in
a month, difficult because it goes into the next month).

Proposed solution


Fixing the parser to handle values like 25:00 would be relatively
easy. However, these rules are also exposed in the public API of
java.time.zone.ZoneOffsetTransitionRule [3]. Currently this class has
methods `getLocalTime()` and `isMidnightEndOfDay()`. These would need
to be deprecated and replaced by a new method `getLocalTimeDuration()`
(or some other name) that returns an instance of `Duration`.

A user of ThreeTen-Backport [4] has provided a branch to do this, so I
know the change to be possible. However, since I have looked at the
code I cannot implement the change in OpenJDK (compromised IP). It
needs a cleanroom implementation by someone else.

Is there agreement on the need for change? Is anyone (Oracle or
otherwise) willing to volunteer do the work?

thanks
Stephen


[1] https://www.iana.org/time-zones
[2] https://bugs.openjdk.java.net/browse/JDK-8212684
[3] 
https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/time/zone/ZoneOffsetTransitionRule.html
[4] https://www.threeten.org/threetenbp/

Re: 6516099: InputStream.skipFully(int k) to skip exactly k bytes

[Roger Riggs]

Hi,

The FIS skipping past of end of file is puzzling.
If the  'were beyond EOF' was considering the possibility that the file 
was being

extended concurrently with the skip operation then it would not be random,
just a normal writer/reader race.  The return value from skip would be 
accurate

and still usable for skipNBytes.

If the spec for skipNBytes describes its behavior in terms of the normal 
behaviors
of skip(n) and read(n) then it will not making promises it can't keep 
regardless of the subclass behavior.


FIS also says it can do negative seeks which seems in conflict with 
InputStream.
The FIS.skip(-n) behavior raises the question about whether 
InputStream.skipNBytes should

allow -n?

Roger

p.s.

There are 200+ subclasses of InputStream in the JDK itself so trying to 
figure out which ones should have performant versions of skipNBytes 
would be quite a bit of extra work.



On 10/24/2018 08:14 PM, Brian Burkhalter wrote:

Hi Sergey,

That is rather ugly. Thanks for pointing it out. The previous version which 
merely repeatedly read and discarded a buffer of bytes did not have this 
problem, but it also did not take advantage of any performance improvement to 
be obtained by subclass overriding of skip().

It does introduce some randomness into the situation. For example if some 
FileInputStream with a backing file of length L were used in two different 
scenarios the results could differ.

A) skip(L-4) then skipNBytes(8)
B) skip(L) then skipNBytes(4)

In case A, skipNBytes(8) could conceivably succeed while in case B, 
skipNBytes(4) could fail, which is inconsistent given that the final positions 
in stream terms are theoretically identical.

Unclear what to do here ...

Thanks,

Brian


On Oct 24, 2018, at 4:41 PM, Sergey Bylokhov  wrote:

It looks like the new version will not throw EOFException if the "skip(n)" 
method will skip more bytes than requested, it is possible for example in 
FileInputStream.skip(long n);

"This method may skip more bytes than what are remaining in the
 * backing file. This produces no exception and the number of bytes skipped
 * may include some number of bytes that were beyond the EOF of the
 * backing file. Attempting to read from the stream after skipping past
 * the end will result in -1 indicating the end of the file."

Re: RFR 8212129: Remove finalize methods from java.util.zip.ZipFIle/Inflator/Deflator

[Lance Andersen]

> On Oct 25, 2018, at 9:32 AM, Alan Bateman  wrote:
> 
> 
> 
> On 24/10/2018 18:19, Lance Andersen wrote:
>> Hi all,
>> 
>> This change removes the finalize methods from 
>> java.util.zip.ZipFIle/Inflator/Deflator.  These methods were deprecated and  
>> marked for removal in JDK 9
>> 
>> The webrev can be found at: 
>> http://cr.openjdk.java.net/~lancea/8212129/webrev.00/ 
>> 
>> 
>> The relevant JCK tests for zip and jar continue to run clean as do the JDK 
>> tier1 - tier3 tests
>> 
>> The CSR has also been finalized
>> 
> Looks good. A minor nit is that the odd formatting at L200 of Deflater can be 
> fixed as you are changing it.

Will fix, thank you
> 
> -Alan

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com 

Re: RFR 8212129: Remove finalize methods from java.util.zip.ZipFIle/Inflator/Deflator

[Alan Bateman]

On 24/10/2018 18:19, Lance Andersen wrote:

Hi all,

This change removes the finalize methods from 
java.util.zip.ZipFIle/Inflator/Deflator.  These methods were deprecated and  
marked for removal in JDK 9

The webrev can be found at: http://cr.openjdk.java.net/~lancea/8212129/webrev.00/ 


The relevant JCK tests for zip and jar continue to run clean as do the JDK 
tier1 - tier3 tests

The CSR has also been finalized

Looks good. A minor nit is that the odd formatting at L200 of Deflater 
can be fixed as you are changing it.


-Alan

Re: RFR 8212129: Remove finalize methods from java.util.zip.ZipFIle/Inflator/Deflator

[Roger Riggs]

Looks good Lance

Thanks

On 10/24/2018 01:19 PM, Lance Andersen wrote:

Hi all,

This change removes the finalize methods from 
java.util.zip.ZipFIle/Inflator/Deflator.  These methods were deprecated and  
marked for removal in JDK 9

The webrev can be found at: http://cr.openjdk.java.net/~lancea/8212129/webrev.00/ 


The relevant JCK tests for zip and jar continue to run clean as do the JDK 
tier1 - tier3 tests

The CSR has also been finalized

Best,
Lance




  
   

  Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com 

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

[Kevin Rushforth]

Andy added the a comment [1] to the JEP with the command line options. 
I'll format it and add it to the JEP itself soon, but until then you can 
see it in the comments to help you review it.


The tests will come shortly (Andy can comment on the state of this). 
They will be a mix of automated tests and (especially for the 
installers) manual tests.


The module with the API is meant to be a small runtime module that would 
be jlinked in with the application (whereas the tool itself typically 
wouldn't be). The working name is jdk.packager.services (inherited from 
javapacakger). An alternative name we had considered was 
jdk.packager.runtime? Or maybe jdk.packager.support? Do you have any 
suggestions?


The exported package is named jdk.packager.services.singleton, since the 
singleton app feature is the only one that currently needs runtime 
support. The intention would be to add other packages as new features 
that require runtime support are added in the future (e.g., daemon 
services or JRE argument support). We haven't given any thought to 
alternative names, other than if we change the module name to something 
like jdk.packager.runtime, we would likely want to use that to inform 
the name of the package. Do you have any recommendations?


Perhaps we can discuss the JNLPConverter demo in a separate thread, 
since I see Erik raised a similar question?


Andy can reply to anything I missed or to clarify further.

-- Kevin

[1] 
https://bugs.openjdk.java.net/browse/JDK-8200758?focusedCommentId=14217780&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14217780



On 10/24/2018 7:22 AM, Alan Bateman wrote:

On 23/10/2018 16:49, Andy Herrick wrote:
This patch implements the Java Packager Tool (jpackager) as described 
in JEP 343: Packaging Tool 



jpackager is a simple packaging tool, based on the JavaFX 
|javapackager| tool, that:


 * Supports native packaging formats to give the end user a natural
   installation experience. These formats include |msi| and |exe| on
   Windows, |pkg| and |dmg| on MacOS, and |deb| and |rpm| on Linux.
 * Allows launch-time parameters to be specified at packaging time.
 * Can be invoked directly, from the command line, or programmatically,
   via the |ToolProvider| API.

Webrev:

http://cr.openjdk.java.net/~herrick/8212780/webrev.01/


cc'ing build-dev as it's important to get it reviewed there.

What is the plan for tests to go with this tool? I see there is one 
test in the webrev to do some argument validation but I don't see 
anything else right now.


What is the status of the JNLPConverter tool? I see it is included as 
a "demo" but maybe it would be better to host somewhere else as this 
is for developers migrating Java Web Start applications.


Would it be possible to update the JEP with all the CLI options? That 
would be useful for review and also useful for those invoking it with 
the ToolProvider API.


If I read the webrev correctly then it adds two modules, one with the 
jpackager tool and the other with an API. It would be useful to get a 
bit more information on the split. Also I think the name of the API 
module and the package that it exports needs discussion to make sure 
that the right names are chosen.


-Alan

Re: RFR: JDK-8212828 Allow POSIX_SPAWN to be used for ProcessImpl on Linux

[Thomas Stüfe]

Hi all,

the more I mull over this, the more I would prefer to do the jump for
real and attempt switch the default to posix_spawn() for Linux.

We have theoretically established that both glibc down to 2.4 and
muslc since always did "the right thing".

We still have time in the 12 time line to test this thoroughly.

What do you think?

Thanks, Thomas

On Wed, Oct 24, 2018 at 10:59 PM Thomas Stüfe  wrote:
>
> Hi Roger,
>
> On Wed 24. Oct 2018 at 21:39, Roger Riggs  wrote:
>>
>> Hi Thomas,
>>
>> Sorry, I had the incantation for multiple tests wrong.
>> Separate test configurations need to be in separate /* */ comment blocks.
>> BTW, it creates separate .jtr files for each block.
>>
>> diff --git a/test/jdk/java/lang/ProcessBuilder/Basic.java 
>> b/test/jdk/java/lang/ProcessBuilder/Basic.java
>> --- a/test/jdk/java/lang/ProcessBuilder/Basic.java
>> +++ b/test/jdk/java/lang/ProcessBuilder/Basic.java
>> @@ -33,8 +33,10 @@
>>   * @modules java.base/java.lang:open
>>   * @run main/othervm/timeout=300 Basic
>>   * @run main/othervm/timeout=300 -Djdk.lang.Process.launchMechanism=fork 
>> Basic
>> - *
>> + */
>> +/*
>>   * @test
>> + * @modules java.base/java.lang:open
>>   * @requires (os.family == "linux")
>>   * @run main/othervm/timeout=300 
>> -Djdk.lang.Process.launchMechanism=posix_spawn Basic
>>   * @author Martin Buchholz
>>
>>
>> As for the build configuration being out of scope...
>>
>> I don't want to see part of the work done and then the rest forgotten or
>> worse yet someone comes along in the future and incorrectly believes that
>> posix_spawn is ready for production and starts filing bugs or getting bit by 
>> something.
>> There's a lot more testing to do before it could be come the default
>> and perhaps a significant warning should be attached to the code so it does 
>> not get forgotten.
>
>
> I agree, we ideally should not roll out half tested changes. But where does 
> that leave us wrt this patch?
>
> We could just not do it, requiring anyone willing to do the extensive testing 
> necessary to switch the default to posix-spawn to apply this change first 
> locally. This is not an insurmountable amount of work, especially since the 
> base is quite static, so the patch won’t bit rot easily.
>
> We could push the patch in its current form, plus a large source comment? But 
> then. Users do not read comments. A warning on stderr? Would play havoc with 
> many tests parsing stderr.
>
> Do you have an idea how to proceed?
>
> Thanks, Thomas
>
>>
>> Regards, Roger
>>
>>
>> On 10/24/18 1:53 PM, Thomas Stüfe wrote:
>>
>> Hi Roger,
>>
>> On Wed, Oct 24, 2018 at 5:23 PM Roger Riggs  wrote:
>>
>> Hi Thomas,
>>
>> Thanks for adding the test.
>>
>> There's a feature of jtreg that was exposed a couple of month ago that
>> can be used to deal with running the test too many times.
>>
>> There can be multiple @test blocks with different @requires.
>>
>> * @run main/othervm/timeout=300 Basic * @run main/othervm/timeout=300
>> -Djdk.lang.Process.launchMechanism=fork Basic * * @test * @requires
>> (os.family == "linux") * @run main/othervm/timeout=300
>> -Djdk.lang.Process.launchMechanism=posix_spawn Basic
>>
>>
>> Does not seem to work, sorry:
>>
>>   24 /*
>>   25  * @test
>>   26  * @bug 4199068 4738465 4937983 4930681 4926230 4931433 4932663 4986689
>>   27  *  5026830 5023243 5070673 4052517 4811767 6192449 6397034 6413313
>>   28  *  6464154 6523983 6206031 4960438 6631352 6631966 6850957 6850958
>>   29  *  4947220 7018606 7034570 4244896 5049299 8003488 8054494 8058464
>>   30  *  8067796
>>   31  * @key intermittent
>>   32  * @summary Basic tests for Process and Environment Variable code
>>   33  * @modules java.base/java.lang:open
>>   34  * @run main/othervm/timeout=300 Basic
>>   35  * @run main/othervm/timeout=300
>> -Djdk.lang.Process.launchMechanism=fork Basic
>>   36  * @test
>>   37  * @requires (os.family == "liinux")
>>   38  * @run main/othervm/timeout=300
>> -Djdk.lang.Process.launchMechanism=posix_spawn Basic
>>   39  * @author Martin Buchholz
>>   40  */
>>
>> If I have @requires (os.family == "linux") all three variants are executed - 
>> ok.
>>
>> Then I wanted to have a negative test, so I misnamed linux as "liinux"
>> and would have expected the first @test block to fire, the second to
>> be ignored. But in fact:
>>
>> thomas@mainframe /shared/projects/openjdk/jtreg $
>> ../jtreg-prebuilt/jtreg/bin/jtreg -jdk
>> ../jdk-jdk/output-slowdebug/images/jdk/
>> ../jdk-jdk/source/test/jdk/java/lang/ProcessBuilder/Basic.java
>> Test results: no tests selected
>>
>> So to me it looks like as if the @requires tag is valid for both @test
>> blocks, not only the one it appears in.
>>
>> I'll run this through our build system too.
>>
>> To fully test out using posix_spawn in many more different scenarios the
>> build system should be augmented to be able to use it as the default for
>> an entire build/CI/CD/test runs.
>>
>> Sure! But I think this is out of scope for this patch.
>>
>

Re: Time-zone database issues

[Stephen Colebourne]

On Thu, 25 Oct 2018 at 09:37, Andrew Haley  wrote:
>
> On 10/22/2018 11:25 PM, Stephen Colebourne wrote:
> > The IANA time-zone database [1] provides details of how time-zones
> > change over time. The latest release - 2018f - cannot be processed
> > successfully by the current JDK parser [2].  A workaround exists
> > however unlike previous cases of tzdb incompatibility, this time it
> > makes sense for the JDK parser and API to be changed.
>
> I don't think so. This patch is in the development version of tzdata:
>
> mm.icann.org/pipermail/tz/attachments/20181018/a1734c98/0001-Avoid-25-00-in-rearguard-format.patch

Perhaps I'm not making myself clear enough. We can cope with *this*
change either by using rearguard or changing the parser. However, the
tzdb author is determined to push ahead with whatever data changes he
sees fit (calls for more tzdb backwards compatibility are rejected).
At some point, he will add an out of bounds change here that can't be
represented in rearguard or in Java. We need to make a change here to
protect ourselves from that, and to fix what is in many ways a bug in
the Java API - caused by a misunderstanding of their file format.

Stephen

Re: 6516099: InputStream.skipFully(int k) to skip exactly k bytes

[Daniel Fuchs]

Hi Brian,

Hmmm... Just thinking aloud:

Should InputStream provide a safe but non-optimized version of
skipNBytes() that does not rely on skip(), and should subclasses
that override skip() for performance also be changed to override
skipNBytes too if performance improvement can be gained?

best regards,

-- daniel

On 25/10/2018 01:14, Brian Burkhalter wrote:

Hi Sergey,

That is rather ugly. Thanks for pointing it out. The previous version which 
merely repeatedly read and discarded a buffer of bytes did not have this 
problem, but it also did not take advantage of any performance improvement to 
be obtained by subclass overriding of skip().

It does introduce some randomness into the situation. For example if some 
FileInputStream with a backing file of length L were used in two different 
scenarios the results could differ.

A) skip(L-4) then skipNBytes(8)
B) skip(L) then skipNBytes(4)

In case A, skipNBytes(8) could conceivably succeed while in case B, 
skipNBytes(4) could fail, which is inconsistent given that the final positions 
in stream terms are theoretically identical.

Unclear what to do here ...

Thanks,

Brian


On Oct 24, 2018, at 4:41 PM, Sergey Bylokhov  wrote:

It looks like the new version will not throw EOFException if the "skip(n)" 
method will skip more bytes than requested, it is possible for example in 
FileInputStream.skip(long n);

"This method may skip more bytes than what are remaining in the
 * backing file. This produces no exception and the number of bytes skipped
 * may include some number of bytes that were beyond the EOF of the
 * backing file. Attempting to read from the stream after skipping past
 * the end will result in -1 indicating the end of the file."

Re: Time-zone database issues

[Andrew Haley]

On 10/22/2018 11:25 PM, Stephen Colebourne wrote:
> The IANA time-zone database [1] provides details of how time-zones
> change over time. The latest release - 2018f - cannot be processed
> successfully by the current JDK parser [2].  A workaround exists
> however unlike previous cases of tzdb incompatibility, this time it
> makes sense for the JDK parser and API to be changed.

I don't think so. This patch is in the development version of tzdata:

mm.icann.org/pipermail/tz/attachments/20181018/a1734c98/0001-Avoid-25-00-in-rearguard-format.patch

-- 
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. 
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

RE: RFR 8212129: Remove finalize methods from java.util.zip.ZipFIle/Inflator/Deflator

[Langer, Christoph]

Hi Lance,

looks fine, reviewed. Good to see this happening.

Best regards
Christoph

> -Original Message-
> From: core-libs-dev  On Behalf
> Of Lance Andersen
> Sent: Mittwoch, 24. Oktober 2018 19:20
> To: core-libs-dev 
> Subject: RFR 8212129: Remove finalize methods from
> java.util.zip.ZipFIle/Inflator/Deflator
> 
> Hi all,
> 
> This change removes the finalize methods from
> java.util.zip.ZipFIle/Inflator/Deflator.  These methods were deprecated and
> marked for removal in JDK 9
> 
> The webrev can be found at:
> http://cr.openjdk.java.net/~lancea/8212129/webrev.00/
> 
> 
> The relevant JCK tests for zip and jar continue to run clean as do the JDK 
> tier1
> - tier3 tests
> 
> The CSR has also been finalized
> 
> Best,
> Lance
> 
> 
> 
> 
>  
>  
> 
>  Lance Andersen|
> Principal Member of Technical Staff | +1.781.442.2037
> Oracle Java Engineering
> 1 Network Drive
> Burlington, MA 01803
> lance.ander...@oracle.com 
> 
>