Re: RFR(XXS) : 8209549 : remove VMPropsExt from TEST.ROOT

[David Holmes]

Looks fine.

Thanks,
David

On 16/08/2018 7:00 AM, Igor Ignatyev wrote:

http://cr.openjdk.java.net/~iignatyev//8209549/webrev.00/index.html

2 lines changed: 0 ins; 0 del; 2 mod;


Hi all,

could you please review this tiny patch which removes unused (and actually 
non-existed) file from requires.extraPropDefns and simplifies paths in 
requires.extraPropDefns and requires.extraPropDefns.bootlibs?

webrev: http://cr.openjdk.java.net/~iignatyev//8209549/webrev.00/index.html
testing: jdk/tier1

Thanks,
-- Igor

Re: RFR JDK-8209553: ExceptionInInitializerError can have a default detail message if the cause is given

[David Holmes]

Hi Mandy,

Not a review - sorry :)

On 16/08/2018 7:46 AM, mandy chung wrote:
ExceptionInInitializerError(Throwable cause) sets the detail message to 
null.  It'd be helpful to include a detail message rather than null

as in Throwable(Throwable cause) that constructs a new throwable
with a default message cause==null ? null : cause.toString().

This would help troubleshooting of writing new bootstrap methods
where VM currently asserts non-null linkage error message [1] where
there are other solutions such as constructing the message in
the VM or removing the assert.


I think that's just a bug in the VM code making inappropriate 
assumptions! There shouldn't have to be a detail message, especially in 
a wrapping exception like ExceptionInInitializerError! It only gets 
thrown because some other exception was thrown, there's really no more 
detail for the EIIE itself. Promoting the cause's detail message to be 
the detail message of the EIIE just seems wrong to me.


Did this come about through the new code that saves the original cause 
of the EIIE so that it can be reported when the subsequent 
NoClassDefFoundError is thrown due to the class being in the erroneous 
state? (I don't recall if that actually got pushed.)


David



This patch proposes to change ExceptionInInitializerError(Throwable cause)
to default the detail message to cause.toString() if cause is not null.
It's an incompatibility change; for example existing code that expects
a null message will now see a detail message.  But the compatibility
risk should be low as EIIE should be fatal.

This also proposes to add new LinkageError(Throwable cause) constructor
to retrofit the exception chaining mechanism and that allows to clean up
the current ExceptionInInitializerError implementation.

Webrev:
   http://cr.openjdk.java.net/~mchung/jdk12/webrevs/8209553/webrev.00

I will create a CSR.

Mandy
[1] https://bugs.openjdk.java.net/browse/JDK-8208172

Re: RFR JDK-8209553: ExceptionInInitializerError can have a default detail message if the cause is given

[mandy chung]

On 8/15/18 5:10 PM, mandy chung wrote:


I think we should remove the ExceptionInInitializerError::getCause
method and have getException to return getCause().  I think the
simplest is to keep the exception field and make sure it's set
with the cause. Existing version of EIIE always has null cause.
readObject will set the cause to be same as exception.  There may
be other options.


I prototype it:
   http://cr.openjdk.java.net/~mchung/jdk12/webrevs/8209553/webrev.01/

I manually verified it reading/writing with new and old version.
I will have to add the interop test case and possibly add a
package-private method to set Throwable::cause (rather than
making the field package-private).  Will update you.

Mandy

Re: RFR JDK-8209553: ExceptionInInitializerError can have a default detail message if the cause is given

[mandy chung]

On 8/15/18 3:20 PM, Peter Levart wrote:

Hi Mandy,

Just a question. Why does "private Throwable exception" field in 
ExceptionInInitializerError exist? Was it there before there was a 
"cause" in Throwable and later still remained there because of 
serialization format? Would it be possible to "simulate" its effect for 
serialization using "serialPersistentFields" and 
ObjectOutputStream.PutField?


Thanks for asking.  I meant to mention this and it'd be nice to
follow up this in a separate issue.

The private exception field exists since 1.1 and kept there for
serialization.  getException in existing releases returns the
exception field.  I can't think of any way to remove the exception
field in JDK n to deserialize it with older JDK x unless JDK x was
changed to write the exception field with the cause or getException
to return cause.

I think we should remove the ExceptionInInitializerError::getCause
method and have getException to return getCause().  I think the
simplest is to keep the exception field and make sure it's set
with the cause. Existing version of EIIE always has null cause.
readObject will set the cause to be same as exception.  There may
be other options.

The main challegne, I think, would be the deserialization of an old 
ExceptionInInitializerError.exception into new Throwable.cause in case 
the stream did not have Throwable.cause (which is the case for old 
ExceptionInInitializerError(s))


Deserializing from an older EIIE to a newer EIIE is feasible.  I was
thinking to separate it as a different issue.

Mandy

Re: RFR(XXS) : 8209386 : [error-prone] StreamResourceLeak in jdk.internal.ed module

[Igor Ignatyev]

ping?

-- Igor

> On Aug 10, 2018, at 1:44 PM, Igor Ignatyev  wrote:
> 
> http://cr.openjdk.java.net/~iignatyev//8209386/webrev.00/index.html
>> 4 lines changed: 1 ins; 0 del; 3 mod;
> 
> Hi all,
> 
> could you please review this small fix for the error detected in 
> jdk.internal.ed by error-prone?
> 
> webrev: http://cr.openjdk.java.net/~iignatyev//8209386/webrev.00/index.html
> JBS: https://bugs.openjdk.java.net/browse/JDK-8209386
> 
> Thanks,
> -- Igor

Re: RFR JDK-8209553: ExceptionInInitializerError can have a default detail message if the cause is given

[Peter Levart]

Hi Mandy,

Just a question. Why does "private Throwable exception" field in 
ExceptionInInitializerError exist? Was it there before there was a 
"cause" in Throwable and later still remained there because of 
serialization format? Would it be possible to "simulate" its effect for 
serialization using "serialPersistentFields" and 
ObjectOutputStream.PutField?


The main challegne, I think, would be the deserialization of an old 
ExceptionInInitializerError.exception into new Throwable.cause in case 
the stream did not have Throwable.cause (which is the case for old 
ExceptionInInitializerError(s))


Ragards, Peter

On 08/15/2018 11:46 PM, mandy chung wrote:
ExceptionInInitializerError(Throwable cause) sets the detail message 
to null.  It'd be helpful to include a detail message rather than null

as in Throwable(Throwable cause) that constructs a new throwable
with a default message cause==null ? null : cause.toString().

This would help troubleshooting of writing new bootstrap methods
where VM currently asserts non-null linkage error message [1] where
there are other solutions such as constructing the message in
the VM or removing the assert.

This patch proposes to change ExceptionInInitializerError(Throwable 
cause)

to default the detail message to cause.toString() if cause is not null.
It's an incompatibility change; for example existing code that expects
a null message will now see a detail message.  But the compatibility
risk should be low as EIIE should be fatal.

This also proposes to add new LinkageError(Throwable cause) constructor
to retrofit the exception chaining mechanism and that allows to clean up
the current ExceptionInInitializerError implementation.

Webrev:
http://cr.openjdk.java.net/~mchung/jdk12/webrevs/8209553/webrev.00

I will create a CSR.

Mandy
[1] https://bugs.openjdk.java.net/browse/JDK-8208172

RFR JDK-8209553: ExceptionInInitializerError can have a default detail message if the cause is given

[mandy chung]

ExceptionInInitializerError(Throwable cause) sets the detail message to 
null.  It'd be helpful to include a detail message rather than null

as in Throwable(Throwable cause) that constructs a new throwable
with a default message cause==null ? null : cause.toString().

This would help troubleshooting of writing new bootstrap methods
where VM currently asserts non-null linkage error message [1] where
there are other solutions such as constructing the message in
the VM or removing the assert.

This patch proposes to change ExceptionInInitializerError(Throwable cause)
to default the detail message to cause.toString() if cause is not null.
It's an incompatibility change; for example existing code that expects
a null message will now see a detail message.  But the compatibility
risk should be low as EIIE should be fatal.

This also proposes to add new LinkageError(Throwable cause) constructor
to retrofit the exception chaining mechanism and that allows to clean up
the current ExceptionInInitializerError implementation.

Webrev:
  http://cr.openjdk.java.net/~mchung/jdk12/webrevs/8209553/webrev.00

I will create a CSR.

Mandy
[1] https://bugs.openjdk.java.net/browse/JDK-8208172

RFR(XXS) : 8209549 : remove VMPropsExt from TEST.ROOT

[Igor Ignatyev]

http://cr.openjdk.java.net/~iignatyev//8209549/webrev.00/index.html
> 2 lines changed: 0 ins; 0 del; 2 mod;

Hi all,

could you please review this tiny patch which removes unused (and actually 
non-existed) file from requires.extraPropDefns and simplifies paths in 
requires.extraPropDefns and requires.extraPropDefns.bootlibs?

webrev: http://cr.openjdk.java.net/~iignatyev//8209549/webrev.00/index.html
testing: jdk/tier1

Thanks,
-- Igor

Re: Removing the Barrier

[Bernd Eckenfels]

You can remove all comments you like. If you do not distribute the resulting 
binaries you do not need to provide the changes/source you made (that’s a legal 
requirement from the GPL license). If you do need to provide source (because 
you distribute binaries) then you have to retain at least all copyright 
messages and license headers.

This assumes the comments have no technical function, this is true for normal 
code but not formsome parts like for example tests where execution parameters 
can be found in the comments. I am not sure about other locations where code 
generation references comments is used (but simply speaking if a comment looks 
unstructured it probably has no function)

Also keep in mind that comments are visible if you generate Javadoc or need to 
use src.zip (think IDEs)

Having said all that, that kind of shredded code does not get you very far, it 
will certainly not allow to contribute changes back to OpenJDK projects. 
Besides changing all line numbers (by removing comments from Java classes) also 
makes interpreting stacktraces harder.

Gruss
Bernd
--
http://bernd.eckenfels.net


Von: -1060516144m Auftrag von
Gesendet: Mittwoch, August 15, 2018 7:35 PM
An: core-libs-dev@openjdk.java.net
Betreff: Removing the Barrier

Can you explain how to remove the requirement for comments in the OpenJDK build 
in the Java source files? It's really killing my schedule to have to reinsert 
comments by hand.

Thanks.

Max R.

Software Developer

Re: Removing the Barrier

[Peter Levart]

The javadoc comments in public API are a requirement in OpenJDK and can 
not be removed. This is the OpenJDK project policy. If you want to 
extend OpenJDK you have to add the comments. And not only comments. New 
public API may be added to OpenJDK only after passing the special 
process called CSR:


https://wiki.openjdk.java.net/display/csr/Main

So if your schedule doesn't allow you to add comments, I would not 
venture into adding new public API to OpenJDK as it is much more work 
than just adding comments...


Regards, Peter

On 08/15/2018 05:20 PM, mr rupplin wrote:

Can you explain how to remove the requirement for comments in the OpenJDK build 
in the Java source files?  It's really killing my schedule to have to reinsert 
comments by hand.

Thanks.

Max R.

Software Developer

Re: JDK 12 RFR of 5075463 : (enum) Serialized Form javadoc for java.lang.Enum is misleading

[joe darcy]

Hi Roger,

Filed JDK-8209542: "Serialization discussion of enum types should 
mention java.lang.Enum."


Thanks for the review,

-Joe


On 8/15/2018 6:20 AM, Roger Riggs wrote:

Hi Joe,

This change looks fine as the spec for serializing Enum's is in OIS 
and OOS.


However, the spec for serializing Enum's in ObjectInputStream and 
ObjectOutputStream is incomplete.
It does not mention that the class of the Enum is written to the 
stream before the enum name.

Please file an issue for that.

Regards, Roger


On 8/15/18 7:27 AM, Lance Andersen wrote:

Hi Joe,

Added myself as a reviewer to the CSR and the change looks fine

On Aug 14, 2018, at 11:59 PM, joe darcy  wrote:


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




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: Core Type Compilation Issues

[Peter Levart]

On 08/15/2018 07:16 PM, Peter Levart wrote:



On 08/15/2018 05:49 PM, dalibor topic wrote:



On 06.08.2018 20:21, mr rupplin wrote:
Three problems that I run into when running the 'make jdk' after 
minor work on the System.java class for JNI and custom JDK:


java/lang/memory/GroupListener.java:111:

Can we get explanations for each of these? 


No.

There is no java.lang.memory in OpenJDK. So it looks as if you are 
not using OpenJDK, you're using something else.


Or Mr. Rupplin is adding class GroupListener to OpenJDK, thus creating 
new package java.lang.memory.


In that case I would suggest adding the class to some existent jdk 
internal package of java.base module below the jdk.internal. package 
hierarchy (java.lang. hierarchy is reserved). But as David Holmes 
already suggested, if this class depends on java.rmi classes, it may 
not be located in java.base module if it wants to access these classes 
directly from compiled code. OTOH it must be located in java.base 
module if it wants to be used from java.lang.System class (which I 
suspect Mr. Rupplin is doing from his message mentioning work on 
System.java class). It is possible to access java.rmi classes from 
java.base module using reflection though. Or better yet, using 
ServiceLoader mechanism.


So I would suggest Mr. Rupplin to:
- define a jdk internal service interface for the minimal 
functionality needed from java.rmi module in the java.base module.
- implement this interface by a concealed class in the java.rmi module 
and provide the service by adding "provides" directive to 
module-info.java of the java.rmi module
- declare that java.base module "uses" the service (in the 
module-info.java of the java.base module)
- code the logic in java.lang.System or helper class by loading the 
service using ServiceLoader API - the logic should be prepared to not 
find any such service since java.rmi module may not be present at runtime
- service interface and any additional utility classes such as 
GroupListener in java.base module should be put into existent (or new) 
jdk internal package(s) below the jdk.internal. package hierarchy.
- the package containintg the service interface should be exported to 
java.rmi module only that provides the implementation of the service


Hope this helps,

Regards, Peter



The big question for Mr. Rupplin is why does he want to put the logic 
that uses java.rmi module into java.lang.System class (or API). Wouldn't 
such logic (or API) belong to java.rmi module and shouldn't Mr. Rupplin 
extend its API instead? Some explanation about the intentions would help 
suggest the right approach.


Peter

Re: Core Type Compilation Issues

[Peter Levart]

On 08/15/2018 05:49 PM, dalibor topic wrote:



On 06.08.2018 20:21, mr rupplin wrote:
Three problems that I run into when running the 'make jdk' after 
minor work on the System.java class for JNI and custom JDK:


java/lang/memory/GroupListener.java:111:

Can we get explanations for each of these? 


No.

There is no java.lang.memory in OpenJDK. So it looks as if you are not 
using OpenJDK, you're using something else.


Or Mr. Rupplin is adding class GroupListener to OpenJDK, thus creating 
new package java.lang.memory.


In that case I would suggest adding the class to some existent jdk 
internal package of java.base module below the jdk.internal. package 
hierarchy (java.lang. hierarchy is reserved). But as David Holmes 
already suggested, if this class depends on java.rmi classes, it may not 
be located in java.base module if it wants to access these classes 
directly from compiled code. OTOH it must be located in java.base module 
if it wants to be used from java.lang.System class (which I suspect Mr. 
Rupplin is doing from his message mentioning work on System.java class). 
It is possible to access java.rmi classes from java.base module using 
reflection though. Or better yet, using ServiceLoader mechanism.


So I would suggest Mr. Rupplin to:
- define a jdk internal service interface for the minimal functionality 
needed from java.rmi module in the java.base module.
- implement this interface by a concealed class in the java.rmi module 
and provide the service by adding "provides" directive to 
module-info.java of the java.rmi module
- declare that java.base module "uses" the service (in the 
module-info.java of the java.base module)
- code the logic in java.lang.System or helper class by loading the 
service using ServiceLoader API - the logic should be prepared to not 
find any such service since java.rmi module may not be present at runtime
- service interface and any additional utility classes such as 
GroupListener in java.base module should be put into existent (or new) 
jdk internal package(s) below the jdk.internal. package hierarchy.
- the package containintg the service interface should be exported to 
java.rmi module only that provides the implementation of the service


Hope this helps,

Regards, Peter

Re: RFR 8209184: JDK8 ResourceBundle vulnerable to GC (fix included)

[mandy chung]

Hi Adam,

This fix is JDK 8u only and so you will need to request for 8u approval.

The proposed empty static method is fine with me.  Please fix the format 
and indentation before you post the review.


Since this patch is small, you can inline the diff in the RFR mail.
I can review it when you send the review request out.

Mandy
[1] http://openjdk.java.net/projects/jdk8u/approval-template.html

On 8/15/18 8:45 AM, Adam Farley8 wrote:

Hi Mandy, Hans,

Tried out the below (after removing my previous fix, the volatile 
compare), and it appears to solve the problem just fine.


If I generate a webrev and get it attached to the bug, would one of you 
mind approving the change?


--...at the end of getBundleImpl in jdk8  
     keepAlive(loader);
     return bundle;
}

//To keep the argument ClassLoader alive.
private static void keepAlive(ClassLoader loaderone){
         //Do nothing.
}
--

Best Regards

Adam Farley

Hans Boehm  wrote on 15/08/2018 06:44:08:


From: Hans Boehm 
To: Mandy Chung 
Cc: adam.far...@uk.ibm.com,  core-libs-dev , i18n-...@openjdk.java.net
Date: 15/08/2018 06:44
Subject: Re: RFR 8209184: JDK8  ResourceBundle vulnerable to GC (fix included)

FWIW, since there was agreement that empty static methods should 
work in this context, that seems like the best option to me.


On Tue, Aug 14, 2018 at 4:54 PM mandy chung   wrote:
Thanks for the information. Not sure what's the best option we can do
in 8u.  I think it's acceptable to have a fix that works in the
current context (like empty static method).

Thoughts?

Mandy

On 8/14/18 4:22 PM, Hans Boehm wrote:
> I haven't looked at the details here, but comparing against a  volatile
> is not in general a portable way to keep an object live. Like  empty
> static methods, it may be fine in the current (OpenJDK) context.
> 
> Volatile loads have roach motel semantics and can be advanced  past other

> operations. The comparison can be done early, too, keeping only  the
> condition code. There is no guarantee the reference will still  be around
> when you expect it.
> 
> I haven't come up with a great compiler-independent replacement  for

> reachabilityFence.
> 
> On Tue, Aug 14, 2018 at 8:25 AM mandy chung 
> > wrote:
> 
>     Hi Adam,
> 
>     Have you tried Peter's suggestion if an empty  static method taking an

>     Object parameter?  Does it work for you?
> 
>     Your proposed approach seems fine and I would  suggest to put the

>     check in a static keepAlive method that will  make it explicitly.
> 
>     Mandy
> 
>     On 8/10/18 8:42 AM, Adam Farley8 wrote:

>      > Hi All,
>      >
>      > This bug could be fixed by comparing  the Class Loader with a blank
>      > static volatile Object (defined outside  the method scope) at the
>      > end of the getBundleImpl class.
>      >
>      > E.g.
>      >
>      > -
>      > +1322
>      >      /**
>      >       * volatile  reference object to guard the ClassLoader object
>      >       * being garbage  collected before getBundleImpl() method
>     completes
>      >       * the caching  and retrieving of requested Resourcebundle object
>      >       */
>      >      private static volatile  Object vo = new Object();
>      >
>      >
>      > +1400
>      > //Should never be true. Using the loader  here prevents it being GC'd.
>      >  if (loader == vo) throw new Error("Unexpected error.");
>      > -
>      >
>      > Will upload a webrev after debate.
>      >
>      > - Adam
>      > Unless stated otherwise above:
>      > IBM United Kingdom Limited - Registered  in England and Wales with
>     number
>      > 741598.
>      > Registered office: PO Box 41, North  Harbour, Portsmouth,
>     Hampshire PO6 3AU
>      >
> 

Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 
741598.

Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

Re: Core Type Compilation Issues

[dalibor topic]

On 06.08.2018 20:21, mr rupplin wrote:

Three problems that I run into when running the 'make jdk' after minor work on 
the System.java class for JNI and custom JDK:

java/lang/memory/GroupListener.java:111:

Can we get explanations for each of these? 


No.

There is no java.lang.memory in OpenJDK. So it looks as if you are not 
using OpenJDK, you're using something else.


Ask whoever gave you what you're using for support. We can't help you 
fix it.


cheers,
dalibor topic
--
 Dalibor Topic | Principal Product Manager
Phone: +494089091214  | Mobile: +491737185961


ORACLE Deutschland B.V. & Co. KG | Kühnehöfe 5 | 22761 Hamburg

ORACLE Deutschland B.V. & Co. KG
Hauptverwaltung: Riesstr. 25, D-80992 München
Registergericht: Amtsgericht München, HRA 95603

Komplementärin: ORACLE Deutschland Verwaltung B.V.
Hertogswetering 163/167, 3543 AS Utrecht, Niederlande
Handelsregister der Handelskammer Midden-Niederlande, Nr. 30143697
Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher

 Oracle is committed to developing
practices and products that help protect the environment

Re: RFR 8209184: JDK8 ResourceBundle vulnerable to GC (fix included)

[Adam Farley8]

Hi Mandy, Hans,

Tried out the below (after removing my previous fix, the volatile 
compare), and it appears to solve the problem just fine. 

If I generate a webrev and get it attached to the bug, would one of you 
mind approving the change?

--...at the end of getBundleImpl in jdk8  
keepAlive(loader);
return bundle;
}

//To keep the argument ClassLoader alive.
private static void keepAlive(ClassLoader loaderone){
//Do nothing.
}
--

Best Regards

Adam Farley 

Hans Boehm  wrote on 15/08/2018 06:44:08:

> From: Hans Boehm 
> To: Mandy Chung 
> Cc: adam.far...@uk.ibm.com, core-libs-dev  d...@openjdk.java.net>, i18n-...@openjdk.java.net
> Date: 15/08/2018 06:44
> Subject: Re: RFR 8209184: JDK8 ResourceBundle vulnerable to GC (fix 
included)
> 
> FWIW, since there was agreement that empty static methods should 
> work in this context, that seems like the best option to me.
> 
> On Tue, Aug 14, 2018 at 4:54 PM mandy chung  
wrote:
> Thanks for the information.  Not sure what's the best option we can do
> in 8u.  I think it's acceptable to have a fix that works in the
> current context (like empty static method).
> 
> Thoughts?
> 
> Mandy
> 
> On 8/14/18 4:22 PM, Hans Boehm wrote:
> > I haven't looked at the details here, but comparing against a volatile 

> > is not in general a portable way to keep an object live. Like empty 
> > static methods, it may be fine in the current (OpenJDK) context.
> > 
> > Volatile loads have roach motel semantics and can be advanced past 
other 
> > operations. The comparison can be done early, too, keeping only the 
> > condition code. There is no guarantee the reference will still be 
around 
> > when you expect it.
> > 
> > I haven't come up with a great compiler-independent replacement for 
> > reachabilityFence.
> > 
> > On Tue, Aug 14, 2018 at 8:25 AM mandy chung  > > wrote:
> > 
> > Hi Adam,
> > 
> > Have you tried Peter's suggestion if an empty static method taking 
an
> > Object parameter?  Does it work for you?
> > 
> > Your proposed approach seems fine and I would suggest to put the
> > check in a static keepAlive method that will make it explicitly.
> > 
> > Mandy
> > 
> > On 8/10/18 8:42 AM, Adam Farley8 wrote:
> >  > Hi All,
> >  >
> >  > This bug could be fixed by comparing the Class Loader with a 
blank
> >  > static volatile Object (defined outside the method scope) at 
the
> >  > end of the getBundleImpl class.
> >  >
> >  > E.g.
> >  >
> >  > -
> >  > +1322
> >  >  /**
> >  >   * volatile reference object to guard the ClassLoader 
object
> >  >   * being garbage collected before getBundleImpl() method
> > completes
> >  >   * the caching and retrieving of requested Resourcebundle 
object
> >  >   */
> >  >  private static volatile Object vo = new Object();
> >  >
> >  >
> >  > +1400
> >  > //Should never be true. Using the loader here prevents it being 
GC'd.
> >  >  if (loader == vo) throw new Error("Unexpected 
error.");
> >  > -
> >  >
> >  > Will upload a webrev after debate.
> >  >
> >  > - Adam
> >  > Unless stated otherwise above:
> >  > IBM United Kingdom Limited - Registered in England and Wales 
with
> > number
> >  > 741598.
> >  > Registered office: PO Box 41, North Harbour, Portsmouth,
> > Hampshire PO6 3AU
> >  >
> > 
Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 
741598. 
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

Re: RFR 8209184: JDK8 ResourceBundle vulnerable to GC (fix included)

[Adam Farley8]

Hi Mandy, Hans,

Tried out the below (after removing my previous fix, the volatile 
compare), and it appears to solve the problem just fine. 

If I generate a webrev and get it attached to the bug, would one of you 
mind approving the change?

--...at the end of getBundleImpl in jdk8  
keepAlive(loader);
return bundle;
}

//To keep the argument ClassLoader alive.
private static void keepAlive(ClassLoader loaderone){
//Do nothing.
}
--

Best Regards

Adam Farley 

Hans Boehm  wrote on 15/08/2018 06:44:08:

> From: Hans Boehm 
> To: Mandy Chung 
> Cc: adam.far...@uk.ibm.com, core-libs-dev  d...@openjdk.java.net>, i18n-...@openjdk.java.net
> Date: 15/08/2018 06:44
> Subject: Re: RFR 8209184: JDK8 ResourceBundle vulnerable to GC (fix 
included)
> 
> FWIW, since there was agreement that empty static methods should 
> work in this context, that seems like the best option to me.
> 
> On Tue, Aug 14, 2018 at 4:54 PM mandy chung  
wrote:
> Thanks for the information.  Not sure what's the best option we can do
> in 8u.  I think it's acceptable to have a fix that works in the
> current context (like empty static method).
> 
> Thoughts?
> 
> Mandy
> 
> On 8/14/18 4:22 PM, Hans Boehm wrote:
> > I haven't looked at the details here, but comparing against a volatile 

> > is not in general a portable way to keep an object live. Like empty 
> > static methods, it may be fine in the current (OpenJDK) context.
> > 
> > Volatile loads have roach motel semantics and can be advanced past 
other 
> > operations. The comparison can be done early, too, keeping only the 
> > condition code. There is no guarantee the reference will still be 
around 
> > when you expect it.
> > 
> > I haven't come up with a great compiler-independent replacement for 
> > reachabilityFence.
> > 
> > On Tue, Aug 14, 2018 at 8:25 AM mandy chung  > > wrote:
> > 
> > Hi Adam,
> > 
> > Have you tried Peter's suggestion if an empty static method taking 
an
> > Object parameter?  Does it work for you?
> > 
> > Your proposed approach seems fine and I would suggest to put the
> > check in a static keepAlive method that will make it explicitly.
> > 
> > Mandy
> > 
> > On 8/10/18 8:42 AM, Adam Farley8 wrote:
> >  > Hi All,
> >  >
> >  > This bug could be fixed by comparing the Class Loader with a 
blank
> >  > static volatile Object (defined outside the method scope) at 
the
> >  > end of the getBundleImpl class.
> >  >
> >  > E.g.
> >  >
> >  > -
> >  > +1322
> >  >  /**
> >  >   * volatile reference object to guard the ClassLoader 
object
> >  >   * being garbage collected before getBundleImpl() method
> > completes
> >  >   * the caching and retrieving of requested Resourcebundle 
object
> >  >   */
> >  >  private static volatile Object vo = new Object();
> >  >
> >  >
> >  > +1400
> >  > //Should never be true. Using the loader here prevents it being 
GC'd.
> >  >  if (loader == vo) throw new Error("Unexpected 
error.");
> >  > -
> >  >
> >  > Will upload a webrev after debate.
> >  >
> >  > - Adam
> >  > Unless stated otherwise above:
> >  > IBM United Kingdom Limited - Registered in England and Wales 
with
> > number
> >  > 741598.
> >  > Registered office: PO Box 41, North Harbour, Portsmouth,
> > Hampshire PO6 3AU
> >  >
> > 
Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 
741598. 
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

Removing the Barrier

[mr rupplin]

Can you explain how to remove the requirement for comments in the OpenJDK build 
in the Java source files?  It's really killing my schedule to have to reinsert 
comments by hand.

Thanks.

Max R.

Software Developer

Re: RFR 8208715: Conversion of milliseconds to nanoseconds in UNIXProcess contains bug

[Martin Buchholz]

Your change is still approved, but ...

All tests are a little white-box in the sense that they are guided by where
bugs are likely to be found, which comes from experience with
implementations.  There is a collection of timed waiting APIs in the JDK
that throw InterruptedException.  It would be great if the entire test
matrix was covered: wait time 0, small positive wait times out, maximally
negative wait time, maximally positive wait time, interrupted before
waiting, interrupted while blocked in wait (Thread state not RUNNING), and
probably more.  We don't achieve full coverage anywhere, but it can be a
goal.

On Wed, Aug 15, 2018 at 7:06 AM, Roger Riggs  wrote:

> Hi Martin,
>
> ok, as a reliable pattern I see that to avoid knowing details of the
> implementation.
> But the self interrupt seems redundant if the other is needed.
>
> Updated:
>   http://cr.openjdk.java.net/~rriggs/webrev-timeout-8208715/
>
> Thanks, Roger
>
>
>
> On 8/14/2018 6:14 PM, Martin Buchholz wrote:
>
> Looks good to me ... but ...
>
> my intent was that the self-interrupt was added in __addition__ to
> interrupt by other thread, because there is often different code to handle
> pre-existing interrupt.  As with BlockingQueueTest.testTimedPollWithOffer.
>
> public void run() {
> Thread.currentThread().interrupt();
> try {
> boolean result = p.waitFor(Long.MAX_VALUE,
> TimeUnit.MILLISECONDS);
> fail("Should have thrown InterruptedException");
> } catch (InterruptedException success) {
> } catch (Throwable t) { unexpected(t); }
>
> try {
> aboutToWaitFor.countDown();
> boolean result = p.waitFor(Long.MAX_VALUE,
> TimeUnit.MILLISECONDS);
> fail("Should have thrown InterruptedException");
> } catch (InterruptedException success) {
> } catch (Throwable t) { unexpected(t); }
>
>
>
>
> On Tue, Aug 14, 2018 at 12:59 PM, Roger Riggs 
> wrote:
>
>> Hi Martin,
>>
>> Updated with suggestions:
>>  http://cr.openjdk.java.net/~rriggs/webrev-timeout-8208715/index.html
>>
>> On 8/14/2018 1:22 PM, Martin Buchholz wrote:
>>
>> Thanks, Roger.  I approve this version, although as always I have
>> comments.
>>
>>  520 private static native void waitForTimeoutInterruptibly(
>>  521 long handle, long timeout);
>>
>> The units being used should be obvious from the signature - rename
>> timeout to timeoutMillis.
>>
>> Done
>>
>> But even better is to push the support for nanos into the utility method,
>> so change this native method to accept a timeoutNanos.
>>
>> A bit far from the original issue and it might take a bit more time.
>>
>>
>> 2465 Thread.sleep(1000);
>>
>> I hate sleeps, and I would just delete this one - I don't think the test
>> relies on it (and if it did, one second is not long enough!).
>>
>> Done.  (And in the test case used for the copy/paste of the new test).
>>
>>
>> 2454 try {
>> 2455 aboutToWaitFor.countDown();
>> 2456 boolean result = p.waitFor(Long.MAX_VALUE,
>> TimeUnit.MILLISECONDS);
>> 2457 fail("waitFor() wasn't interrupted, its
>> return value was: " + result);
>> 2458 } catch (InterruptedException success) {
>> 2459 } catch (Throwable t) { unexpected(t); }
>>
>> It's easy to add a self-interrupt variant inside the run method
>>
>> Thread.currentThread().interrupt(); ...
>>
>> ok, and will run all the tests again.
>>
>> Thanks, Roger
>>
>>
>>
>> (TODO: Basic.java is in need of a re-write - mea culpa ...)
>>
>>
>>
>>
>>
>> On Tue, Aug 14, 2018 at 9:48 AM, Roger Riggs 
>> wrote:
>>
>>> Hi Martin,
>>>
>>> I updated the webrev with the suggestions.
>>>
>>> On 8/14/2018 10:47 AM, Martin Buchholz wrote:
>>>
>>> hi Roger,
>>>
>>>  509 if (deadline <= 0) { 510 deadline = 
>>> Long.MAX_VALUE; 511 }
>>>
>>>
>>> This must be wrong.  Nanotime wraparound is normal in this sort of code.
>>>
>>> ok, this reader didn't make that assumption
>>>
>>>
>>> ---
>>>
>>> We ought to be able to delegate the fiddling with nanos to
>>> TimeUnit.timedWait.
>>>
>>> Does it work to simply call NANOSECONDS.timedWait(remainingNanos) ?
>>> If not, is there a bug in TimeUnit.timedWait?
>>>
>>> That works except on Windows, that does not use wait().
>>>
>>>
>>> It's good to add a test for this.  I've tried hard in similar tests to
>>> avoid sleep and to add variants where the target thread is interrupted
>>> before and after starting to wait.  Testing pre-interrupt is easy - the
>>> thread can interrupt itself.  BlockingQueueTest.testTimedPollWithOffer
>>> is an example.
>>>
>>> I added a test, using the same logic as the existing tests for the
>>> Long.MAX_VALUE case
>>>
>>> Thanks, Roger
>>>
>>>
>>>
>>>
>>

Re: RFR 8208715: Conversion of milliseconds to nanoseconds in UNIXProcess contains bug

[Roger Riggs]

Hi Martin,

ok, as a reliable pattern I see that to avoid knowing details of the 
implementation.

But the self interrupt seems redundant if the other is needed.

Updated:
  http://cr.openjdk.java.net/~rriggs/webrev-timeout-8208715/

Thanks, Roger


On 8/14/2018 6:14 PM, Martin Buchholz wrote:

Looks good to me ... but ...

my intent was that the self-interrupt was added in __addition__ to 
interrupt by other thread, because there is often different code to 
handle pre-existing interrupt.  As with 
BlockingQueueTest.testTimedPollWithOffer.


                public void run() {
                    Thread.currentThread().interrupt();
                    try {
                        boolean result = p.waitFor(Long.MAX_VALUE, 
TimeUnit.MILLISECONDS);

                        fail("Should have thrown InterruptedException");
                    } catch (InterruptedException success) {
                    } catch (Throwable t) { unexpected(t); }

                    try {
                        aboutToWaitFor.countDown();
                        boolean result = p.waitFor(Long.MAX_VALUE, 
TimeUnit.MILLISECONDS);

                        fail("Should have thrown InterruptedException");
                    } catch (InterruptedException success) {
                    } catch (Throwable t) { unexpected(t); }




On Tue, Aug 14, 2018 at 12:59 PM, Roger Riggs > wrote:


Hi Martin,

Updated with suggestions:
http://cr.openjdk.java.net/~rriggs/webrev-timeout-8208715/index.html


On 8/14/2018 1:22 PM, Martin Buchholz wrote:

Thanks, Roger.  I approve this version, although as always I have
comments.

 520     private static native void waitForTimeoutInterruptibly(
 521         long handle, long timeout);

The units being used should be obvious from the signature -
rename timeout to timeoutMillis.

Done

But even better is to push the support for nanos into the utility
method, so change this native method to accept a timeoutNanos.

A bit far from the original issue and it might take a bit more time.


2465             Thread.sleep(1000);

I hate sleeps, and I would just delete this one - I don't think
the test relies on it (and if it did, one second is not long
enough!).

Done.  (And in the test case used for the copy/paste of the new test).


2454                     try {
2455  aboutToWaitFor.countDown();
2456                         boolean result =
p.waitFor(Long.MAX_VALUE, TimeUnit.MILLISECONDS);
2457                         fail("waitFor() wasn't interrupted,
its return value was: " + result);
2458                     } catch (InterruptedException success) {
2459                     } catch (Throwable t) { unexpected(t); }

It's easy to add a self-interrupt variant inside the run method

Thread.currentThread().interrupt(); ...

ok, and will run all the tests again.

Thanks, Roger




(TODO: Basic.java is in need of a re-write - mea culpa ...)





On Tue, Aug 14, 2018 at 9:48 AM, Roger Riggs
mailto:roger.ri...@oracle.com>> wrote:

Hi Martin,

I updated the webrev with the suggestions.

On 8/14/2018 10:47 AM, Martin Buchholz wrote:

hi Roger,

509 if (deadline <= 0) {
510 deadline = Long.MAX_VALUE;
511 }

This must be wrong.  Nanotime wraparound is normal in this
sort of code.

ok, this reader didn't make that assumption


---

We ought to be able to delegate the fiddling with nanos to
TimeUnit.timedWait.

Does it work to simply call
NANOSECONDS.timedWait(remainingNanos) ?
If not, is there a bug in TimeUnit.timedWait?

That works except on Windows, that does not use wait().



It's good to add a test for this.  I've tried hard in
similar tests to avoid sleep and to add variants where the
target thread is interrupted before and after starting to
wait.  Testing pre-interrupt is easy - the thread can
interrupt itself. BlockingQueueTest.testTimedPollWithOffer
is an example.

I added a test, using the same logic as the existing tests
for the Long.MAX_VALUE case

Thanks, Roger






On Tue, Aug 14, 2018 at 7:00 AM, Roger Riggs
mailto:roger.ri...@oracle.com>> wrote:

Please review a fix for
Process.waitFor(Long.MAX_VALUE,MILLIS).
Catch wrap around in very large wait times and saturate
at Long.MAX_VALUE.

Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-timeout-8208715/


Issue:
https://bugs.openjdk.java.net/browse/JDK-8208715


   

Re: JDK 12 RFR of 5075463 : (enum) Serialized Form javadoc for java.lang.Enum is misleading

[Roger Riggs]

Hi Joe,

This change looks fine as the spec for serializing Enum's is in OIS and OOS.

However, the spec for serializing Enum's in ObjectInputStream and 
ObjectOutputStream is incomplete.
It does not mention that the class of the Enum is written to the stream 
before the enum name.

Please file an issue for that.

Regards, Roger


On 8/15/18 7:27 AM, Lance Andersen wrote:

Hi Joe,

Added myself as a reviewer to the CSR and the change looks fine

On Aug 14, 2018, at 11:59 PM, joe darcy  wrote:


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




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: JDK 12 RFR of 8176425: Add radix indication in NumberFormatException message for Integer.decode

[Lance Andersen]

Looks good Joe
On Aug 14, 2018, at 11:20 PM, joe darcy  wrote:

> http://cr.openjdk.java.net/~darcy/8176425.0/




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: JDK 12 RFR of 5075463 : (enum) Serialized Form javadoc for java.lang.Enum is misleading

[Lance Andersen]

Hi Joe,

Added myself as a reviewer to the CSR and the change looks fine

On Aug 14, 2018, at 11:59 PM, joe darcy  wrote:

> https://bugs.openjdk.java.net/browse/JDK-8209524




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: 8207851 JEP Draft: Support ByteBuffer mapped over non-volatile memory

[Andrew Dinn]

Hi Alan,

Just a quick follow-up regarding the BufferPoolMXBean.

On 14/08/18 12:34, Andrew Dinn wrote:
> On 06/08/18 10:29, Alan Bateman wrote:
> 
>> Are you familiar with BufferPoolMXBean which can be used by management
>> tools to monitor pool of buffers? I'm wondering if we should introduce a
>> new pool for NVM so that it doesn't show up in the "mapped" pool.

Ok, so I have worked through the code and can see that MappedByteBuffers
for file device-based mappings (as opposed to direct memory mappings)
are currently counted by class Unmapper local to  FileChannelImpl and
the counts are made visible via a BufferPoolMXBean with name "mapped"
which is present in the list exposed by method getBufferPoolMXBeans() of
class ManagementFactoryHelper.

So, it seems what you are asking for is to keep separate tallies for
persistent MappedByteBuffers and expose them in a new BufferPoolMXBean
also inserted in the list exposed by getBufferPoolMXBeans. That sounds
like quite a good idea. It will leave any current code that wants to see
file mappings counting the 'same' thing yet still makes it possible to
count persistent mappings on their own and also tally all mappings by
iterating over all BufferPoolMXBeans in the list. I suggest giving the
bean the name "mapped_persistent".

I would happily update the JEP to include this as an 'API change' of
sorts but I'm not quite sure how to document it. Is this ok

 the JEP requires exposing a new java.lang.management.BufferPoolMXBean
with name "mapped_persistent" as a platform managed bean which can be
accessed using the existing ManagementFactory.getPlatformMXBeans() API

  the "mapped_persistent" bean records the buffer count, total bytes and
total allocation count stats for persistent MappedByteBuffers (i.e.
those mapped using MapMode READ_ONLY_PERSISTENT or READ_WRITE_PERSISTENT)

  these stats are collected separately from those currently collected in
the "mapped" BufferPoolMXBean which records the equivalent counts for
non-persistent MappedByteBuffers (i.e. those mapped using MapMode
READ_ONLY, READ_WRITE or PRIVATE)

It seems straightforward to implement this behaviour. If you like I can
add it to the next webrev?

regards,


Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: Arrays.asList returned List - Its modifying/write-through semantics

[Jaikiran Pai]

Hi Ivan,


On 15/08/18 1:20 PM, Ivan Gerasimov wrote:
> Hi Jaikiran!
>
> The first part (the documentation clarification) was requested some
> time ago [1], so it may be eventually fixed.
>
> [1] https://bugs.openjdk.java.net/browse/JDK-7033681
Thank you for pointing to that one. I hadn't found it during my search
before sending this mail.

>
> With respect to the second part (throwing UOE from remove(Object)), I
> agree with you that it would be more consistent to always throw it,
> regardless of the presence of the searched item.
> However, it would introduce a change of behavior, so I think it's
> unlikely to happen.
I understand.
>
> It may make sense to update the documentation for all the
> structurally-modifying methods of Collection/List (remove(Object),
> retainAll, removeAll, clear) in the same way it was done for
> List.removeIf() here [2].
>
> [2] https://bugs.openjdk.java.net/browse/JDK-8023339

I looked at that commit and yes, a similar @throws explaining the
UnsupportedOperationException will be helpful.

-Jai
>
> With kind regards,
> Ivan
>
>
> On 8/14/18 10:58 PM, Jaikiran Pai wrote:
>> Consider the following code:
>>
>> import java.util.Arrays;
>> import java.util.List;
>>
>> public class ArraysAsListTest {
>>
>>  public static void main(final String[] args) throws Exception {
>>  final List someList = Arrays.asList(new String[]
>> {"a"});
>>  System.out.println("Removed? " + someList.remove("a"));
>>  }
>> }
>>
>> It uses Arrays.asList to create a java.util.List and then calls a
>> list.remove(...) on it. This currently runs intothe following exception:
>>
>> Exception in thread "main" java.lang.UnsupportedOperationException:
>> remove
>>  at java.base/java.util.Iterator.remove(Iterator.java:102)
>>  at
>> java.base/java.util.AbstractCollection.remove(AbstractCollection.java:299)
>>
>>  at ArraysAsListTest.main(ArraysAsListTest.java:8)
>>
>> The javadoc of Arrays.asList[1] states the following:
>>
>> "Returns a fixed-size list backed by the specified array. (Changes to
>> the returned list "write through" to the array.)..."
>>
>> and has no other mention of how it's supposed to behave with "write"
>> operations. Given that the javadoc states that the returned list is
>> "write through", would that mean a "add/removal" is allowed? Probably
>> not, because it does say it's a fixed size list.So the size altering,
>> write operations (like add(), remove()) aren't allowed, but that isn't
>> clear from the javadoc. So should the javadoc be updated to clarify the
>> semantics of what changes are "write through" and what changes are not
>> supported?
>>
>> Furthermore, the UnsupportedOperationException isn't always thrown from
>> such a returned list. Consider this minor modification to the above
>> code:
>>
>>  final List someList = Arrays.asList(new String[]
>> {"a"});
>>  System.out.println("Removed? " + someList.remove("b"));
>>
>> I add "a" and remove "b". This now runs fine without exceptions and
>> prints "Removed? false". Should the implementation just throw the
>> UnsupportedOperationException irrespective of whether or not the element
>> is contained insuch a returned list? If not, should that be made clear
>> in the javadoc (maybe same for addoperations)?
>>
>> [1]
>> https://docs.oracle.com/javase/10/docs/api/java/util/Arrays.html#asList(T...)
>>
>>
>> -Jaikiran
>>
>>
>>
>

Re: Arrays.asList returned List - Its modifying/write-through semantics

[Ivan Gerasimov]

Hi Jaikiran!

The first part (the documentation clarification) was requested some time 
ago [1], so it may be eventually fixed.


[1] https://bugs.openjdk.java.net/browse/JDK-7033681

With respect to the second part (throwing UOE from remove(Object)), I 
agree with you that it would be more consistent to always throw it, 
regardless of the presence of the searched item.
However, it would introduce a change of behavior, so I think it's 
unlikely to happen.


It may make sense to update the documentation for all the 
structurally-modifying methods of Collection/List (remove(Object), 
retainAll, removeAll, clear) in the same way it was done for 
List.removeIf() here [2].


[2] https://bugs.openjdk.java.net/browse/JDK-8023339

With kind regards,
Ivan


On 8/14/18 10:58 PM, Jaikiran Pai wrote:

Consider the following code:

import java.util.Arrays;
import java.util.List;

public class ArraysAsListTest {

 public static void main(final String[] args) throws Exception {
 final List someList = Arrays.asList(new String[] {"a"});
 System.out.println("Removed? " + someList.remove("a"));
 }
}

It uses Arrays.asList to create a java.util.List and then calls a
list.remove(...) on it. This currently runs intothe following exception:

Exception in thread "main" java.lang.UnsupportedOperationException: remove
 at java.base/java.util.Iterator.remove(Iterator.java:102)
 at
java.base/java.util.AbstractCollection.remove(AbstractCollection.java:299)
 at ArraysAsListTest.main(ArraysAsListTest.java:8)

The javadoc of Arrays.asList[1] states the following:

"Returns a fixed-size list backed by the specified array. (Changes to
the returned list "write through" to the array.)..."

and has no other mention of how it's supposed to behave with "write"
operations. Given that the javadoc states that the returned list is
"write through", would that mean a "add/removal" is allowed? Probably
not, because it does say it's a fixed size list.So the size altering,
write operations (like add(), remove()) aren't allowed, but that isn't
clear from the javadoc. So should the javadoc be updated to clarify the
semantics of what changes are "write through" and what changes are not
supported?

Furthermore, the UnsupportedOperationException isn't always thrown from
such a returned list. Consider this minor modification to the above code:

 final List someList = Arrays.asList(new String[] {"a"});
 System.out.println("Removed? " + someList.remove("b"));

I add "a" and remove "b". This now runs fine without exceptions and
prints "Removed? false". Should the implementation just throw the
UnsupportedOperationException irrespective of whether or not the element
is contained insuch a returned list? If not, should that be made clear
in the javadoc (maybe same for addoperations)?

[1]
https://docs.oracle.com/javase/10/docs/api/java/util/Arrays.html#asList(T...)

-Jaikiran





--
With kind regards,
Ivan Gerasimov