RFR: 8160954: (spec) Runtime.Version regex and $PRE/$OPT issues

2016-10-20 Thread Iris Clark 
Hi.

Please review changes to address the following closely related bugs:

8160954: (spec) Runtime.Version regex and $PRE/$OPT issues
https://bugs.openjdk.java.net/browse/JDK-8160954

8148822: (spec) Regex in Runtime.Version and JEP 223 should match
https://bugs.openjdk.java.net/browse/JDK-8148822

8148877: (spec) Specify when and empty '+' is required in a version
string
https://bugs.openjdk.java.net/browse/JDK-8148877

webrev:

http://cr.openjdk.java.net/~iris/verona/8160954/webrev/

Changes to the specification of the $VNUM and $OPT regexp:

For line 963, we remove the leading '^' and trailing '$'.  These characters
are not strictly required for the regex to be accurate.  They're not included
in the implementation.  The presence of the trailing '$' precludes direct
substitution of this regex into the definition of $VSTR at line 1032 (see
bug 8160954).

Also for line 963, we remove the outermost qualifier in the portion of the
regex describing elements after $MAJOR.  This qualifier is redundant and a
source of catastrophic backtracking as described in this message [0, item 1]
(see bug 8148822).  It's not in the present implementation.

For line 1050, we remove the '\' as it is unnecessary in a character class
[0, item 4] (see bug 8148822).


Changes to the spec for $PRE/$OPT:

For lines 1056-1057, we clearly specify when an empty '+' is required (see
bugs 8148877, 8160954).  This spec corresponds to the code for additional
constraints for the "empty '+'" beginning at new line 1150.


The required CCC is in progress.

Thanks,
iris

[0] 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-January/038036.html
[1] http://download.java.net/java/jdk9/docs/api/java/lang/Runtime.Version.html

Re: RFR 9: 8164908: ReflectionFactory support for IIOP and custom serialization

2016-10-20 Thread Roger Riggs 

Hi,

Thanks for the comments..

Webrev's updated in place with comments so far.  (Including David's and 
Amy's)


http://cr.openjdk.java.net/~rriggs/webrev-reflection-factory-8164908/

http://cr.openjdk.java.net/~rriggs/webrev-reflection-factory-iiop-8164908/

On 10/20/2016 8:21 AM, Alan Bateman wrote:

On 19/10/2016 20:59, Roger Riggs wrote:

The support in sun.reflect.ReflectionFactory for custom 
serialization, such as IIOP input
and output streams, is being expanded beyond the necessary 
constructor of a serializable
class to include access to the private methods readObject, 
writeObject, readResolve,

writeReplace, etc.

The IIOP implementation is updated to use a combination of 
ReflectionFactory and
Unsafe to serialize and deserialize objects and no longer rely on 
setAccessible.

Tests are included for ReflectionFactory and the affected IIOP classes.

Please review and comment,

jdk repo webrev:
http://cr.openjdk.java.net/~rriggs/webrev-reflection-factory-8164908/
corba repo webrev :
http://cr.openjdk.java.net/~rriggs/webrev-reflection-factory-iiop-8164908/ 




I skimmed through the changes.

I assume findReadWriteObjectForSerialization should throw 
InternalError, rather than return null, if IllegalAccessException is 
thrown (as IAE is not possible here).

fixed


You've added @since 9 to sun.reflect.ReflectionFactory but that class 
is not new.
It was new in 9, but did not previously have @since. Its internal so it 
may not be important.

Added by 8137058: Clear out all non-Critical APIs from sun.reflect



The javadoc for 
sun.reflect.ReflectionFactory.newConstructorForSerialization doesn't 
say that it returns null when the Class is not Serializable.

The current implementation does not check that the argument is Serializable
and there are tests for that.  I will update the documentation to not 
specify a Serializable class.
I would need to track down all the uses to understand that adding the 
check would not break something.


It also does not check that the requested constructor is for a supertype.
I think the semantics of newConstructorForSerialization should include 
the search for

the super-type's noarg constructor instead of IIOP doing that search.



For the MH returning methods then I assume the javadoc should say that 
it returns a direct method handle.

ok


The synchronization in the IIOP ObjectStreamClass isn't very clear. 
Are the invoke*, read*, write* methods all invoked by the same thread 
that creates the ObjectStreamClass with the lookup method?

ObjectStreamClass instances are cached and re-used across all threads.
ObjectStreamClass.init() handles the synchronization of the initialization.
Any thread using the ObjectStreamClass will use the same method handle 
regardless of the

thread calling the method.

Thanks, Roger

Re: RFR 9: 8164908: ReflectionFactory support for IIOP and custom serialization

2016-10-20 Thread Roger Riggs 

Hi Amy,

Thanks for the suggestions.  I will fix them in the next update.
I've had some issues with port in use errors and am looking for best 
practices

in starting the servers and their cleanup.

Roger


On 10/20/2016 11:54 AM, Amy Lu wrote:

On 10/20/16 11:32 PM, Amy Lu wrote:

On 10/20/16 3:59 AM, Roger Riggs wrote:
The support in sun.reflect.ReflectionFactory for custom 
serialization, such as IIOP input
and output streams, is being expanded beyond the necessary 
constructor of a serializable
class to include access to the private methods readObject, 
writeObject, readResolve,

writeReplace, etc.

The IIOP implementation is updated to use a combination of 
ReflectionFactory and
Unsafe to serialize and deserialize objects and no longer rely on 
setAccessible.

Tests are included for ReflectionFactory and the affected IIOP classes.

Please review and comment,

jdk repo webrev:
http://cr.openjdk.java.net/~rriggs/webrev-reflection-factory-8164908/
corba repo webrev :
http://cr.openjdk.java.net/~rriggs/webrev-reflection-factory-iiop-8164908/ 



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

Thanks, Roger



test/com/sun/corba/serialization/ObjectStreamTest.java

371 startOrbd_orig();
Should test wait for several seconds after this to make sure it started?


413 orbdProcess.destroy();
414 if (!orbdProcess.waitFor(10, TimeUnit.SECONDS)) {

And to try best to cleanup process and release port, how about use:
orbdProcess.destroyForcibly();
orbdProcess.waitFor();

Thanks,
Amy


(I'm not an official reviewer.)

Thanks,
Amy

Re: RFR 9: 8164908: ReflectionFactory support for IIOP and custom serialization

2016-10-20 Thread Roger Riggs 

Hi David,

Thanks for the reminder, I'll include readObjectNoData in the next update.

(fyi, readObject, readObjectNoData, and writeObject must be private 
methods.)


Roger


On 10/20/2016 9:12 AM, David M. Lloyd wrote:

On 10/19/2016 02:59 PM, Roger Riggs wrote:

The support in sun.reflect.ReflectionFactory for custom serialization,
such as IIOP input
and output streams, is being expanded beyond the necessary constructor
of a serializable
class to include access to the private methods readObject, writeObject,
readResolve,
writeReplace, etc.

The IIOP implementation is updated to use a combination of
ReflectionFactory and
Unsafe to serialize and deserialize objects and no longer rely on
setAccessible.
Tests are included for ReflectionFactory and the affected IIOP classes.

Please review and comment,

jdk repo webrev:
http://cr.openjdk.java.net/~rriggs/webrev-reflection-factory-8164908/
corba repo webrev :
http://cr.openjdk.java.net/~rriggs/webrev-reflection-factory-iiop-8164908/ 



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


For the custom serialization case, it is also necessary to access the 
(usually private) readObjectNoData method (if any).

Re: RFR: jsr166 jdk9 integration wave 12

2016-10-20 Thread Paul Sandoz 

> On 19 Oct 2016, at 19:33, Martin Buchholz  wrote:
> 
> 
> 
> On Wed, Oct 19, 2016 at 3:44 PM, Paul Sandoz  > wrote:
> 
>> On 18 Oct 2016, at 13:58, Martin Buchholz > > wrote:
>> 
>> Latest webrev defers potential new methods:
>>/* TODO: public */ private void trimToSize()
> 
> Sorry to push back, i know it’s a hassle, but i think such features should be 
> retained in a separate proposed patch.
> 
> OK.  These methods can still be found in jsr166 CVS, but are now filtered out 
> during upstreaming.


Thanks for being flexible. Looks good to me (i did not look in detail at the 
impl improvements on ArrayDeque, i think Stuart has that one amply covered).

Paul.

Re: RFR: JDK-8146750:java.time.Month.getDisplayName() return incorrect narrow names with JRE provider on locale de,de_DE,en_US.

2016-10-20 Thread Naoto Sato 

Looks good, Rachna.

Naoto

On 10/20/16 1:27 AM, Rachna Goel wrote:

Hi,

Please review fix for JDK-8146750.

Bug : https://bugs.openjdk.java.net/browse/JDK-8146750

webrev : http://cr.openjdk.java.net/~rgoel/JDK-8146750/webrev.09/

Fix is to retrieve Narrow and Narrow_Standalone Month names and Day
names one by one, as they can be duplicate.

Thanks,

Rachna

Re: RFR 9: 8164908: ReflectionFactory support for IIOP and custom serialization

2016-10-20 Thread Amy Lu 

On 10/20/16 11:32 PM, Amy Lu wrote:

On 10/20/16 3:59 AM, Roger Riggs wrote:
The support in sun.reflect.ReflectionFactory for custom 
serialization, such as IIOP input
and output streams, is being expanded beyond the necessary 
constructor of a serializable
class to include access to the private methods readObject, 
writeObject, readResolve,

writeReplace, etc.

The IIOP implementation is updated to use a combination of 
ReflectionFactory and
Unsafe to serialize and deserialize objects and no longer rely on 
setAccessible.

Tests are included for ReflectionFactory and the affected IIOP classes.

Please review and comment,

jdk repo webrev:
http://cr.openjdk.java.net/~rriggs/webrev-reflection-factory-8164908/
corba repo webrev :
http://cr.openjdk.java.net/~rriggs/webrev-reflection-factory-iiop-8164908/ 



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

Thanks, Roger



test/com/sun/corba/serialization/ObjectStreamTest.java

371 startOrbd_orig();
Should test wait for several seconds after this to make sure it started?


413 orbdProcess.destroy();
414 if (!orbdProcess.waitFor(10, TimeUnit.SECONDS)) {

And to try best to cleanup process and release port, how about use:
orbdProcess.destroyForcibly();
orbdProcess.waitFor();

Thanks,
Amy


(I'm not an official reviewer.)

Thanks,
Amy

Re: RFR 9: 8164908: ReflectionFactory support for IIOP and custom serialization

2016-10-20 Thread Amy Lu 

On 10/20/16 3:59 AM, Roger Riggs wrote:
The support in sun.reflect.ReflectionFactory for custom serialization, 
such as IIOP input
and output streams, is being expanded beyond the necessary constructor 
of a serializable
class to include access to the private methods readObject, 
writeObject, readResolve,

writeReplace, etc.

The IIOP implementation is updated to use a combination of 
ReflectionFactory and
Unsafe to serialize and deserialize objects and no longer rely on 
setAccessible.

Tests are included for ReflectionFactory and the affected IIOP classes.

Please review and comment,

jdk repo webrev:
http://cr.openjdk.java.net/~rriggs/webrev-reflection-factory-8164908/
corba repo webrev :
http://cr.openjdk.java.net/~rriggs/webrev-reflection-factory-iiop-8164908/ 



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

Thanks, Roger



test/com/sun/corba/serialization/ObjectStreamTest.java

371 startOrbd_orig();
Should test wait for several seconds after this to make sure it started?

(I'm not an official reviewer.)

Thanks,
Amy

Re: [9] RfR: 8165271: Fix use of reflection to gain access to private fields

2016-10-20 Thread David DeHaven 

>> Please review the new patch version:
>> http://cr.openjdk.java.net/~ddehaven/8165271/jdk.1/
>> 
> 
> Looks okay but I think the new interface should be named 
> “JavaNetURLClassLoaderAccess” (matching “URLClassLoader")

I originally had that but changed it to look more consistent with the other 
JavaXXAccess interfaces... I can change it back before pushing.

-DrD-

Re: RFR: JDK-8146750:java.time.Month.getDisplayName() return incorrect narrow names with JRE provider on locale de,de_DE,en_US.

2016-10-20 Thread Roger Riggs 

+1

Looks fine

On 10/20/2016 5:06 AM, Masayoshi Okutsu wrote:

Looks good to me.

Masayoshi


On 10/20/2016 5:27 PM, Rachna Goel wrote:

Hi,

Please review fix for JDK-8146750.

Bug : https://bugs.openjdk.java.net/browse/JDK-8146750

webrev : http://cr.openjdk.java.net/~rgoel/JDK-8146750/webrev.09/

Fix is to retrieve Narrow and Narrow_Standalone Month names and Day 
names one by one, as they can be duplicate.


Thanks,

Rachna

Re: RFR 9: 8164908: ReflectionFactory support for IIOP and custom serialization

2016-10-20 Thread David M. Lloyd 

On 10/19/2016 02:59 PM, Roger Riggs wrote:

The support in sun.reflect.ReflectionFactory for custom serialization,
such as IIOP input
and output streams, is being expanded beyond the necessary constructor
of a serializable
class to include access to the private methods readObject, writeObject,
readResolve,
writeReplace, etc.

The IIOP implementation is updated to use a combination of
ReflectionFactory and
Unsafe to serialize and deserialize objects and no longer rely on
setAccessible.
Tests are included for ReflectionFactory and the affected IIOP classes.

Please review and comment,

jdk repo webrev:
http://cr.openjdk.java.net/~rriggs/webrev-reflection-factory-8164908/
corba repo webrev :
http://cr.openjdk.java.net/~rriggs/webrev-reflection-factory-iiop-8164908/

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


For the custom serialization case, it is also necessary to access the 
(usually private) readObjectNoData method (if any).


--
- DML

Re: RFR: jsr166 jdk9 integration wave 12

2016-10-20 Thread Jonathan Bluett-Duncan 
Ah, I see. Thanks for answering my questions Martin.

Sounds like there's no real benefit then to using guava-testlib in this
changeset instead of CollectionTest. And even if the benefit of using it
was considered to be high enough in the future, it would probably need its
own JSR, since I presume it would be applicable to a number of places in
the jdk, making it a humongous change.

Overall, it seems sensible to wait until e.g. JUnit 5 develops a worthwhile
solution to the dynamic test generation problem, at which point we could
then consider it for writing exhaustive tests for the jdk collections.

Thanks again,
Jonathan


On 20 October 2016 at 04:45, Martin Buchholz  wrote:

>
>
> On Wed, Oct 19, 2016 at 4:40 PM, Jonathan Bluett-Duncan <
> jbluettdun...@gmail.com> wrote:
>
>> Hi Martin,
>>
>> By collections infrastructure, do you mean something like the collection
>> testers in guava-testlib?
>>
>> If so, I agree that JUnit 5's dynamic tests look promising for
>> implementing such an infrastructure. I admit I don't have all the context
>> here, but would using guava-testlib in the meantime be a viable medium- or
>> short-term solution? Or would its dependence on JUnit 3/4 make switching
>> impractical? Or, perhaps, has development into CollectionTest gone so far
>> that, for that reason instead, it would be impractical until switch, at
>> least until something superior using e.g. JUnit 5's dynamic tests is made?
>>
>
> I'm embarrassed to say I'm not familiar enough with guava's testlib.
> Guava does have generic collection oriented tests, and even runs them on
> jdk classes. (Someone on the jdk quality team should be running the guava
> tests using development jdks!).  I'm not familiar enough with the tools to
> work on this myself, but I encourage someone who is to do that.
>
> I see that guava testlib does have:
>
> TestsForQueuesInJavaUtil.java
> TestsForSetsInJavaUtil.java
> TestsForListsInJavaUtil.java
> TestsForMapsInJavaUtil.java
>
> and that the infrastructure there is vaguely similar to what I ended up
> doing.  JDK version skew is a problem, because openjdk development is
> focused on jdk9, while guava is still trying to digest jdk8.
>

Re: RFR 9: 8164908: ReflectionFactory support for IIOP and custom serialization

2016-10-20 Thread Alan Bateman 

On 19/10/2016 20:59, Roger Riggs wrote:

The support in sun.reflect.ReflectionFactory for custom serialization, 
such as IIOP input
and output streams, is being expanded beyond the necessary constructor 
of a serializable
class to include access to the private methods readObject, 
writeObject, readResolve,

writeReplace, etc.

The IIOP implementation is updated to use a combination of 
ReflectionFactory and
Unsafe to serialize and deserialize objects and no longer rely on 
setAccessible.

Tests are included for ReflectionFactory and the affected IIOP classes.

Please review and comment,

jdk repo webrev:
http://cr.openjdk.java.net/~rriggs/webrev-reflection-factory-8164908/
corba repo webrev :
http://cr.openjdk.java.net/~rriggs/webrev-reflection-factory-iiop-8164908/ 




I skimmed through the changes.

I assume findReadWriteObjectForSerialization should throw InternalError, 
rather than return null, if IllegalAccessException is thrown (as IAE is 
not possible here).


You've added @since 9 to sun.reflect.ReflectionFactory but that class is 
not new.


The javadoc for 
sun.reflect.ReflectionFactory.newConstructorForSerialization doesn't say 
that it returns null when the Class is not Serializable.


For the MH returning methods then I assume the javadoc should say that 
it returns a direct method handle.


The synchronization in the IIOP ObjectStreamClass isn't very clear. Are 
the invoke*, read*, write* methods all invoked by the same thread that 
creates the ObjectStreamClass with the lookup method?


-Alan

Re: RFR: JDK-8146750:java.time.Month.getDisplayName() return incorrect narrow names with JRE provider on locale de,de_DE,en_US.

2016-10-20 Thread Masayoshi Okutsu 

Looks good to me.

Masayoshi


On 10/20/2016 5:27 PM, Rachna Goel wrote:

Hi,

Please review fix for JDK-8146750.

Bug : https://bugs.openjdk.java.net/browse/JDK-8146750

webrev : http://cr.openjdk.java.net/~rgoel/JDK-8146750/webrev.09/

Fix is to retrieve Narrow and Narrow_Standalone Month names and Day 
names one by one, as they can be duplicate.


Thanks,

Rachna

RFR 8167646: Better invalid FilePermission

2016-10-20 Thread Wang Weijun 
Please review the code change at

   http://cr.openjdk.java.net/~weijun/8167646/webrev.00/

A new flag invalid is added so invalid FilePermissions (invalid Path) do not 
equal or imply or is implied by anything else except for itself.

Thanks
Max

RFR: JDK-8146750:java.time.Month.getDisplayName() return incorrect narrow names with JRE provider on locale de,de_DE,en_US.

2016-10-20 Thread Rachna Goel 

Hi,

Please review fix for JDK-8146750.

Bug : https://bugs.openjdk.java.net/browse/JDK-8146750

webrev : http://cr.openjdk.java.net/~rgoel/JDK-8146750/webrev.09/

Fix is to retrieve Narrow and Narrow_Standalone Month names and Day 
names one by one, as they can be duplicate.


Thanks,

Rachna