Re: Race Condition in initializeEncoding() function in jni_util.c

[David Holmes]

Hi Ron,

On 11/11/2016 1:01 AM, Ronald Servant wrote:



Hi,

While building the J9 JVM for the IBM SDK for Java(*), we've run across
what we believe is a race condition in initializeEncoding(JNIEnv*) in
jni_util.c.


I suspect there is an assumption of single-threaded initialization of 
the system classes, which would hit this code and so safely initialize 
the statics. There may be many such assumptions.


Reordering the statements may address your failure but does not make the 
code thread-safe, and you may encounter additional, more obscure race 
conditions.


David
-



  (*)  The IBM SDK for Java is comprised of the J9 JVM and OpenJDK
class library.
We've encountered a situation where (*env)->CallObjectMethod() in
JNU_GetStringPlatformChars() is invoked with an uninitialized method ID
(the static variable "String_getBytes_ID").

The function initializeEnconding() initializes 4 static variables, but only
one of them ("fastEncoding") is checked to see if initializeEnconding()
should be called in JNU_NewStringPlatform() and JNU_GetStringPlatformChars
().  Further, "fastEncoding" is the first of the 4 static variables set by
initializeEncoding() leaving a race condition where another thread sees
"fastEncoding" set but "String_getBytes_ID" is not yet initialized.

I believe that moving the initialization of "String_getBytes_ID" and
"String_init_ID" to the top of the function would eliminate the impact of
the race condition.

I'd also recommend moving the initialization of "jnuEncoding" up one line,
but this may lead to leaking a global ref.

We are able to reproduce the failure in JNU_GetStringPlatformChars() due to
using an null String_getBytes_ID variable for a method id race condition in
our environment 1 in 20.  With the following patch applied, the race
condition can no longer be reproduced.

Do you agree there is a race condition?  If so, would the patch be an
appropriate fix?  We could introduce locking, but that feels like
overkill.

Ron.

diff -r efa71dc820eb src/java.base/share/native/libjava/jni_util.c
--- a/src/java.base/share/native/libjava/jni_util.cMon Nov 07 13:10:42
2016 -0400
+++ b/src/java.base/share/native/libjava/jni_util.cWed Nov 09 17:19:16
2016 -0500
@@ -688,6 +688,15 @@

 strClazz = JNU_ClassString(env);
 CHECK_NULL(strClazz);
+
+/* Initialize method-id cache */
+String_getBytes_ID = (*env)->GetMethodID(env, strClazz,
+ "getBytes",
"(Ljava/lang/String;)[B");
+CHECK_NULL(String_getBytes_ID);
+String_init_ID = (*env)->GetMethodID(env, strClazz,
+ "",
"([BLjava/lang/String;)V");
+CHECK_NULL(String_init_ID);
+

 propname = (*env)->NewStringUTF(env, "sun.jnu.encoding");
 if (propname) {
@@ -727,8 +736,8 @@
  strcmp(encname, "utf-16le") == 0)
 fastEncoding = FAST_CP1252;
 else {
+jnuEncoding = (jstring)(*env)->NewGlobalRef(env,
enc);
 fastEncoding = NO_FAST_ENCODING;
-jnuEncoding = (jstring)(*env)->NewGlobalRef(env,
enc);
 }
 (*env)->ReleaseStringUTFChars(env, enc, encname);
 }
@@ -741,13 +750,6 @@
 }
 (*env)->DeleteLocalRef(env, propname);
 (*env)->DeleteLocalRef(env, enc);
-
-/* Initialize method-id cache */
-String_getBytes_ID = (*env)->GetMethodID(env, strClazz,
- "getBytes",
"(Ljava/lang/String;)[B");
-CHECK_NULL(String_getBytes_ID);
-String_init_ID = (*env)->GetMethodID(env, strClazz,
- "",
"([BLjava/lang/String;)V");
 }

RE: RFR 8158880: java/time/tck/java/time/format/TCKDateTimeFormatterBuilder.java fail with zh_CN locale

[Bhanu Gopularam]

Hi Roger,

 

Thanks for the review. I have updated the test to set default locale (and 
restore it) in only those methods which were causing problem in non English 
locales.

 

Please review the updated webrev below:

Webrev: http://cr.openjdk.java.net/~bgopularam/JDK-8158880/webrev.01/

 

Thanks,

Bhanu

 

From: Roger Riggs 
Sent: Tuesday, June 21, 2016 8:36 PM
To: Bhanu Gopularam; core-libs-dev@openjdk.java.net
Cc: Stephen Colebourne
Subject: Re: RFR 8158880: 
java/time/tck/java/time/format/TCKDateTimeFormatterBuilder.java fail with zh_CN 
locale

 

Hi Bhanu,

It can be problematic to change the default Locale for the entire process, 
especially
since many tests are run in the same process.  It would be preferable to set 
the locale
in the DateTimeFormatter builder instead of changing the process wide Locale.

Please identify the specific test case to apply the fix only where needed.

Is it only 
tck.java.time.format.TCKDateTimeFormatterBuilder.test_appendZoneText_parseGenericTimeZonePatterns
where the failure is seen?

This would be an issue testing any pattern letter with locale dependent 
behavior.
The locale should be set explicitly in the test.

BTW,  There is a predefined Locale.US that can be used, no need to construct a 
new Locale.

Roger

 

 

On 6/21/2016 6:31 AM, Bhanu Gopularam wrote:

Hi all,
 
May I request you to review fix for following issue
Bug id: https://bugs.openjdk.java.net/browse/JDK-8158880 
 
Solution: To avoid test failure in non english locales, set the default locale 
while initial test setup
 
Webrev: http://cr.openjdk.java.net/~bgopularam/JDK-8158880/webrev.00/ 
 
Thanks,
Bhanu

 

Re: RFR: jdk8u-dev Backport of 8169191: (tz) Support tzdata2016i

[Masayoshi Okutsu]

+1


On 11/11/2016 1:48 AM, Martin Buchholz wrote:

Looks good!

On Thu, Nov 10, 2016 at 1:48 AM, Ramanand Patil 
wrote:


Hi all,
Please review the latest TZDATA integration (tzdata2016i) to JDK8U.
Since tzdata is cumulative, this bug fix backports both the tzdata
versions(tzdata2016h+tzdata2016i) into jdk8u.

Bug: https://bugs.openjdk.java.net/browse/JDK-8169191
Webrev: http://cr.openjdk.java.net/~rpatil/8169537/webrev.00/

Jdk9 changeset: http://hg.openjdk.java.net/jdk9/dev/jdk/rev/7f7091c1dd33

For reference:
Jdk9 changeset for tzdata2016h integration(JDK-8168512):
http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/3192d7aa428d

All the TimeZone related tests are passed after integration.


Regards,
Ramanand.

Re: RFR 8169435 : ClassLoader.isParallelCapable is final and conflicting method may get VerifyError

[David Holmes]

On 11/11/2016 8:46 AM, Mandy Chung wrote:



On Nov 10, 2016, at 2:28 PM, Peter Levart  wrote:

On 11/10/2016 05:59 PM, Alan Bateman wrote:



On 10/11/2016 17:42, David M. Lloyd wrote:

My original suggestion for the method was to make it static, and possibly even 
caller-sensitive, for just this reason.

Changing it to be non-final looks reasonable here, the main reason being that 
it's a no-arg is method and so unlikely that there are custom class loaders 
that have a method with this name that returns something other than boolean. 
However the modifier might be a concern and so time will tell if there are 
custom class loaders that defining a non-public no-arg method with this name.

-Alan


It would be nice for this method to be final.


That’d be the ideal case.


This way it could be relied on to return the "correct" answer regardless of the 
implementation subclass. Who knows, maybe some internal logic might need this method in the future 
and at that time another package-protected method would have to be added and exposed via 
SharedSecrets or similar. If "isParallelCapable" is already taken, then what about 
choosing another name?


This is alternative but it’s hard to predict the probability of a name clash 
with existing subclass implementation.


Since there is already a @CallerSensitive protected static method called 
"registerAsParallelCapable" for subclasses to call from their  blocks, 
the query could be called:

isRegisteredAsParallelCapable() ?

I doubt this name is already taken by any subclass out there…


isRegisteredAsParallelCapable may be okay.


I'd vote for that and keeping it final.

Thanks,
David


Mandy

Re: RFR 8169435 : ClassLoader.isParallelCapable is final and conflicting method may get VerifyError

[Mandy Chung]

> On Nov 10, 2016, at 2:28 PM, Peter Levart  wrote:
> 
> On 11/10/2016 05:59 PM, Alan Bateman wrote:
>> 
>> 
>> On 10/11/2016 17:42, David M. Lloyd wrote:
>>> My original suggestion for the method was to make it static, and possibly 
>>> even caller-sensitive, for just this reason.
>> Changing it to be non-final looks reasonable here, the main reason being 
>> that it's a no-arg is method and so unlikely that there are custom class 
>> loaders that have a method with this name that returns something other than 
>> boolean. However the modifier might be a concern and so time will tell if 
>> there are custom class loaders that defining a non-public no-arg method with 
>> this name.
>> 
>> -Alan
> 
> It would be nice for this method to be final.

That’d be the ideal case.

> This way it could be relied on to return the "correct" answer regardless of 
> the implementation subclass. Who knows, maybe some internal logic might need 
> this method in the future and at that time another package-protected method 
> would have to be added and exposed via SharedSecrets or similar. If 
> "isParallelCapable" is already taken, then what about choosing another name?

This is alternative but it’s hard to predict the probability of a name clash 
with existing subclass implementation.

> Since there is already a @CallerSensitive protected static method called 
> "registerAsParallelCapable" for subclasses to call from their  
> blocks, the query could be called:
> 
> isRegisteredAsParallelCapable() ?
> 
> I doubt this name is already taken by any subclass out there…

isRegisteredAsParallelCapable may be okay.

Mandy

Re: RFR 8169435 : ClassLoader.isParallelCapable is final and conflicting method may get VerifyError

[Peter Levart]

On 11/10/2016 05:59 PM, Alan Bateman wrote:



On 10/11/2016 17:42, David M. Lloyd wrote:
My original suggestion for the method was to make it static, and 
possibly even caller-sensitive, for just this reason.
Changing it to be non-final looks reasonable here, the main reason 
being that it's a no-arg is method and so unlikely that there are 
custom class loaders that have a method with this name that returns 
something other than boolean. However the modifier might be a concern 
and so time will tell if there are custom class loaders that defining 
a non-public no-arg method with this name.


-Alan


It would be nice for this method to be final. This way it could be 
relied on to return the "correct" answer regardless of the 
implementation subclass. Who knows, maybe some internal logic might need 
this method in the future and at that time another package-protected 
method would have to be added and exposed via SharedSecrets or similar. 
If "isParallelCapable" is already taken, then what about choosing 
another name? Since there is already a @CallerSensitive protected static 
method called "registerAsParallelCapable" for subclasses to call from 
their  blocks, the query could be called:


isRegisteredAsParallelCapable() ?

I doubt this name is already taken by any subclass out there...

Regards, Peter

Re: 8169001: Remove launcher's built-in ergonomics

[David Holmes]

Looks good!

Thanks,
David

On 11/11/2016 7:10 AM, Kumar Srinivasan wrote:

Hi David,

Here is the updated revision
* added back in FreeUnknowVMS
* add -client KNOWN as you suggested

delta webrev
http://cr.openjdk.java.net/~ksrini/8169001/webrev.01/webrev.delta/

full webrev (for reference):
http://cr.openjdk.java.net/~ksrini/8169001/webrev.01/

Thanks
Kumar




On 11/7/2016 3:24 PM, David Holmes wrote:

Hi Kumar,

On 8/11/2016 4:47 AM, Kumar Srinivasan wrote:


Hello,

Please review the fix for:
https://bugs.openjdk.java.net/browse/JDK-8169001

Webrev at:
http://cr.openjdk.java.net/~ksrini/8169001/webrev.00/


Overall this looks like a complete eradication of the launcher
ergonomics. A few specific comments:

src/java.base/share/native/launcher/main.c

!const_cpwildcard, const_javaw, 0);

Can you clarify this change with

!const_cpwildcard, const_javaw, 0 /* unused */);

Thanks.

---

src/java.base/share/native/libjli/java.c

 220 jint ergo   /* unused */

I assume JLI_Launch is an exported API otherwise I'd expect this to be
deleted. The fact it is unused should also be documented in
src/java.base/share/native/libjli/java.h. But I'm also wondering where
this API change is being documented for external users?

- FreeKnownVMs();  /* after last possible PrintUsage() */

Not clear why you no longer need to free here. But if you don't then
FreeKnownVMs() seems to be dead code now.

---

src/java.base/unix/conf/i586/jvm.cfg

I think you still need an entry for "-client KNOWN" as anyone can
build the client VM if they choose. Ditto for
src/java.base/unix/conf/sparc/jvm.cfg (though 32-bit Solaris builds
are obsolete now).

---

You have updated a number of copyright notices but not all of them.

---

I see no changes to tests, but I'm pretty sure we have a test for
server-class-machine somewhere (or we did - I recall it breaking at
some point).

Thanks,
David


Background:

Launcher ergonomics was introduced last decade to help determine
if the execution system is "Server Class", this was necessary to
choose server VM on platforms that supported both client and server
VMs (primarily for Solaris and Linux 32-bit).

The algorithm involves computing and detecting the number of CPUs
and the amount of memory on the target system. All modern computers
systems with hyper-threading cause the ergonomics to choose server.

JDK9 Platforms that have only server vm.

./linux-x64/lib/amd64/server/libjvm.so
./linux-arm64-vfp-hflt/lib/aarch64/server/libjvm.so
./solaris-sparcv9/lib/sparcv9/server/libjvm.so
./solaris-x64/lib/amd64/server/libjvm.so
./windows-x86/bin/server/jvm.dll
./windows-x64/bin/server/jvm.dll

JDK9 Platforms that have more than one vm variant:
./linux-arm32-vfp-hflt/lib/arm/client/libjvm.so (default)
./linux-arm32-vfp-hflt/lib/arm/minimal/libjvm.so

./linux-x86/lib/i386/server/libjvm.so (default)
./linux-x86/lib/i386/minimal/libjvm.so


In the cases where multiple VMs are supported the ergnomics
has no effect, and the default platforms are chosen by the
jvm.cfg. Thus the launcher ergonomics is obsolete and redundant.


Thanks
Kumar

Re: 8169001: Remove launcher's built-in ergonomics

[Kumar Srinivasan]

Hi David,

Here is the updated revision
* added back in FreeUnknowVMS
* add -client KNOWN as you suggested

delta webrev
http://cr.openjdk.java.net/~ksrini/8169001/webrev.01/webrev.delta/

full webrev (for reference):
http://cr.openjdk.java.net/~ksrini/8169001/webrev.01/

Thanks
Kumar




On 11/7/2016 3:24 PM, David Holmes wrote:

Hi Kumar,

On 8/11/2016 4:47 AM, Kumar Srinivasan wrote:


Hello,

Please review the fix for:
https://bugs.openjdk.java.net/browse/JDK-8169001

Webrev at:
http://cr.openjdk.java.net/~ksrini/8169001/webrev.00/


Overall this looks like a complete eradication of the launcher 
ergonomics. A few specific comments:


src/java.base/share/native/launcher/main.c

!const_cpwildcard, const_javaw, 0);

Can you clarify this change with

!const_cpwildcard, const_javaw, 0 /* unused */);

Thanks.

---

src/java.base/share/native/libjli/java.c

 220 jint ergo   /* unused */

I assume JLI_Launch is an exported API otherwise I'd expect this to be 
deleted. The fact it is unused should also be documented in 
src/java.base/share/native/libjli/java.h. But I'm also wondering where 
this API change is being documented for external users?


- FreeKnownVMs();  /* after last possible PrintUsage() */

Not clear why you no longer need to free here. But if you don't then 
FreeKnownVMs() seems to be dead code now.


---

src/java.base/unix/conf/i586/jvm.cfg

I think you still need an entry for "-client KNOWN" as anyone can 
build the client VM if they choose. Ditto for 
src/java.base/unix/conf/sparc/jvm.cfg (though 32-bit Solaris builds 
are obsolete now).


---

You have updated a number of copyright notices but not all of them.

---

I see no changes to tests, but I'm pretty sure we have a test for 
server-class-machine somewhere (or we did - I recall it breaking at 
some point).


Thanks,
David


Background:

Launcher ergonomics was introduced last decade to help determine
if the execution system is "Server Class", this was necessary to
choose server VM on platforms that supported both client and server
VMs (primarily for Solaris and Linux 32-bit).

The algorithm involves computing and detecting the number of CPUs
and the amount of memory on the target system. All modern computers
systems with hyper-threading cause the ergonomics to choose server.

JDK9 Platforms that have only server vm.

./linux-x64/lib/amd64/server/libjvm.so
./linux-arm64-vfp-hflt/lib/aarch64/server/libjvm.so
./solaris-sparcv9/lib/sparcv9/server/libjvm.so
./solaris-x64/lib/amd64/server/libjvm.so
./windows-x86/bin/server/jvm.dll
./windows-x64/bin/server/jvm.dll

JDK9 Platforms that have more than one vm variant:
./linux-arm32-vfp-hflt/lib/arm/client/libjvm.so (default)
./linux-arm32-vfp-hflt/lib/arm/minimal/libjvm.so

./linux-x86/lib/i386/server/libjvm.so (default)
./linux-x86/lib/i386/minimal/libjvm.so


In the cases where multiple VMs are supported the ergnomics
has no effect, and the default platforms are chosen by the
jvm.cfg. Thus the launcher ergonomics is obsolete and redundant.


Thanks
Kumar

Re: 8169425: Values computed by a ClassValue should not strongly reference the ClassValue

[Paul Sandoz]

Hi Jochen,

Can you confirm if my analysis of Groovy using ClassValue was correct:

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


AFAICT in this case the issue was not with ClassValue itself but the storing of 
computed values in a global set.

If i understand correctly you are you raising a wider issue with the function 
of ClassValue itself, it may be insufficient for your use-case, and not 
specifically with computed values strongly referring the associated ClassValue?

Specifically, I am struggling to unpack this bit:

> But if you will need at the same time a ClassValue to not to prevent a class 
> we computed the value for from unloading and have the computed value alive 
> for min(lifespan class, lifespan runtime), then you get a real big problem in 
> realizing this.


Computed values can strongly refer to the associated class, it becomes 
problematic when computed values strongly refer to the associated ClassValue. 
Is that something you require?

Paul.


> On 10 Nov 2016, at 00:51, Jochen Theodorou  wrote:
> 
> 
> 
> On 09.11.2016 17:47, Paul Sandoz wrote:
>> Hi Peter,
>> 
>> Good point about if such support was added it would break the API and also 
>> (with Remi) about Ephemerons.
>> 
>> You are correct in stating the constraints in more detail regarding classes 
>> in the same loader. However, i think that is going into more detail than I 
>> would prefer for what i think is an edge case.
> 
> I would not regard that an edge case. For a dynamic language using ClassValue 
> to store information, it is very easy to produce a memory leak with 
> ClassValue.
> 
>> So I want in general to warn developers away from strongly referencing this 
>> ClassValue in the computed value for any associated class.
>> 
>> If we do get strong feedback that this is a real problem we could consider 
>> adding a clever little static method like you suggest, with caveats that the 
>> computing Function might go away.
> 
> for us it is such a strong problem, that we are unable to transfer the old 
> structure to use ClassValue without a major rewrite of large parts.
> Just imagine a runtime that uses ClassValue and that your application, let it 
> be a web application, will spawn many runtimes over time. You do want the 
> runtimes and all computed values be able to be garbage collected. But if you 
> will need at the same time a ClassValue to not to prevent a class we computed 
> the value for from unloading and have the computed value alive for 
> min(lifespan class, lifespan runtime), then you get a real big problem in 
> realizing this.
> 
> As it stands for me ClassValue is only usable as a class associated cache 
> with values you can recreate at any moment. That is not good enough for us in 
> the general case.
> 
> If that is an edge case I still miss a major part in the documentation... and 
> that is what a ClassValue should be used for instead of a cryptic and 
> incomplete description of what a ClassValue is. And then we could talk about 
> if the intended use and the actual usability fit together
> 
> bye Jcohen

Re: RFR 9: 8169556 : Wrap FileInputStream's native skip and available methods

[Brian Burkhalter]

+1

Thanks,

Brian

On Nov 10, 2016, at 11:17 AM, Chris Hegarty  wrote:

> On 10 Nov 2016, at 19:09, Roger Riggs  wrote:
>> 
>> Hi,
>> 
>> Please review this request to enable instrumentation on the FileInputStream 
>> skip and available methods
>> by wrapping native methods with java methods as proposed and contributed by 
>> Sunny Chan.
>> 
>> webrev:
>> http://cr.openjdk.java.net/~rriggs/webrev-fis-native-wrap-8169556/
> 
> Reviewed. Thanks Roger.

Re: RFR 9: 8169556 : Wrap FileInputStream's native skip and available methods

[Chris Hegarty]

On 10 Nov 2016, at 19:09, Roger Riggs  wrote:
> 
> Hi,
> 
> Please review this request to enable instrumentation on the FileInputStream 
> skip and available methods
> by wrapping native methods with java methods as proposed and contributed by 
> Sunny Chan.
> 
> webrev:
>  http://cr.openjdk.java.net/~rriggs/webrev-fis-native-wrap-8169556/

Reviewed. Thanks Roger.

-Chris.

> Issue:
>  https://bugs.openjdk.java.net/browse/JDK-8169556
> 
> Regards, Roger
> 
> p.s.  Sorry for the long lapse Sonny
> 
> 
> On 11/9/2016 10:02 PM, Chan, Sunny wrote:
>> Hello all,
>> 
>> I proposed a patch to provide wrapping for some native methods in 
>> FileInputStream a while ago in May, and the patch has been reviewed but it 
>> has not been integrated. I have checked the patch again with the latest JDK9 
>> dev builds and it still works correctly passing java.io regression tests so 
>> could I have someone to sponsor this and integrate this patch?
>> 
>> Here is the previous threads from email archive: 
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-May/040901.html I 
>> have attached the patch again in this email for reference.
>> 
>> Sunny Chan
>> Executive Director
>> Technology
>> 
>> Goldman Sachs (Asia) L.L.C. | 39th Floor | The Center | 99 Queens Road 
>> Central | Hong Kong
>> Email:  sunny.c...@gs.com | Tel: +852 2978 6481 | Fax: +852 2978 0633
>> 
>> Learn more about Goldman Sachs
>> GS.com | 
>> Blog | 
>> LinkedIn | 
>> YouTube | 
>> Twitter
>> 
>> This message may contain information that is confidential or privileged.  If 
>> you are not the intended recipient, please advise the sender immediately and 
>> delete this message.  See http://www.gs.com/disclaimer/email for further 
>> information on confidentiality and the risks inherent in electronic 
>> communication.
>> 
> 

RFR 9: 8169556 : Wrap FileInputStream's native skip and available methods

[Roger Riggs]

Hi,

Please review this request to enable instrumentation on the 
FileInputStream skip and available methods
by wrapping native methods with java methods as proposed and contributed 
by Sunny Chan.


webrev:
  http://cr.openjdk.java.net/~rriggs/webrev-fis-native-wrap-8169556/

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

Regards, Roger

p.s.  Sorry for the long lapse Sonny


On 11/9/2016 10:02 PM, Chan, Sunny wrote:

Hello all,

I proposed a patch to provide wrapping for some native methods in 
FileInputStream a while ago in May, and the patch has been reviewed but it has 
not been integrated. I have checked the patch again with the latest JDK9 dev 
builds and it still works correctly passing java.io regression tests so could I 
have someone to sponsor this and integrate this patch?

Here is the previous threads from email archive: 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-May/040901.html I 
have attached the patch again in this email for reference.

Sunny Chan
Executive Director
Technology

Goldman Sachs (Asia) L.L.C. | 39th Floor | The Center | 99 Queens Road Central 
| Hong Kong
Email:  sunny.c...@gs.com | Tel: +852 2978 6481 | Fax: +852 2978 0633

Learn more about Goldman Sachs
GS.com | Blog 
| LinkedIn | 
YouTube | Twitter

This message may contain information that is confidential or privileged.  If 
you are not the intended recipient, please advise the sender immediately and 
delete this message.  See http://www.gs.com/disclaimer/email for further 
information on confidentiality and the risks inherent in electronic 
communication.

Re: Review request: JDK-8168386: Fix jdeps verbose options

[Lance Andersen]

Hi Mandy,

This looks OK. 

Best
Lance
> On Nov 9, 2016, at 10:01 PM, Mandy Chung  wrote:
> 
> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8168386/webrev.00
> 
> This patch tightens the option validation to jdeps and the new test shows 
> some example of conflicting options.
> 
> Mandy

 
  

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

[9] RfR: 8169289: JavaFX application in named module fails to launch if no main method

[David DeHaven]

Please review the (fairly straightforward) JDK changes needed to support 
launching JavaFX applications in a named module.

JBS:
https://bugs.openjdk.java.net/browse/JDK-8169289

Webrev:
http://cr.openjdk.java.net/~ddehaven/8169289/jdk.0/



For reference, here are the openjfx changes needed:
http://cr.openjdk.java.net/~ddehaven/8169289/openjfx-rt.0/

The changes can be pushed independently, so no fancy juggling will be needed. 
Only the LM_MODULE mode fails in any mixed combination, which fails as things 
stand now. I will file a followup issue to add unit tests, but those tests will 
go into openjfx and will wait until openjfx and openjdk promoted builds are in 
sync. I don't think the non-FX modes are affected substantially enough to 
warrant any test changes on the openjdk side.

-DrD-

Re: RFR 8169435 : ClassLoader.isParallelCapable is final and conflicting method may get VerifyError

[Alan Bateman]

On 10/11/2016 17:42, David M. Lloyd wrote:
My original suggestion for the method was to make it static, and 
possibly even caller-sensitive, for just this reason.
Changing it to be non-final looks reasonable here, the main reason being 
that it's a no-arg is method and so unlikely that there are custom 
class loaders that have a method with this name that returns something 
other than boolean. However the modifier might be a concern and so time 
will tell if there are custom class loaders that defining a non-public 
no-arg method with this name.


-Alan

Re: RFR: jdk8u-dev Backport of 8169191: (tz) Support tzdata2016i

[Martin Buchholz]

Looks good!

On Thu, Nov 10, 2016 at 1:48 AM, Ramanand Patil 
wrote:

> Hi all,
> Please review the latest TZDATA integration (tzdata2016i) to JDK8U.
> Since tzdata is cumulative, this bug fix backports both the tzdata
> versions(tzdata2016h+tzdata2016i) into jdk8u.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8169191
> Webrev: http://cr.openjdk.java.net/~rpatil/8169537/webrev.00/
>
> Jdk9 changeset: http://hg.openjdk.java.net/jdk9/dev/jdk/rev/7f7091c1dd33
>
> For reference:
> Jdk9 changeset for tzdata2016h integration(JDK-8168512):
> http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/3192d7aa428d
>
> All the TimeZone related tests are passed after integration.
>
>
> Regards,
> Ramanand.
>

Re: RFR 8169435 : ClassLoader.isParallelCapable is final and conflicting method may get VerifyError

[David M. Lloyd]

My original suggestion for the method was to make it static, and 
possibly even caller-sensitive, for just this reason.


On 11/09/2016 04:53 PM, Brent Christian wrote:

Hi,

It seems that the method name used in 8165793[1], "isParallelCapable",
was a little *too* intuitive.  An existing use of that method name has
been uncovered (via NetBeans, in Eclipse Equinox).

Because the newly added method was marked final, the pre-existing method
results in a VerifyError under JDK 9 (for overriding a final method).

Please review my fix, which makes ClassLoader.isParallelCapable()
non-final.  The semantics of such a boolean-returning is method are
clear - it's unlikely that existing methods would use different semantics.

I've included an @apiNote (ala ClassLoader.getName()) explaining that
the non-final-ness here is strictly for compatibility.

Bug:
https://bugs.openjdk.java.net/browse/JDK-8169435
Webrev:
http://cr.openjdk.java.net/~bchristi/8169435/webrev.0/

Thanks,
-Brent

1. https://bugs.openjdk.java.net/browse/JDK-8165793



--
- DML

RFR: 8169495: Add a method to set an Authenticator on a HttpURLConnection.

[Daniel Fuchs]

Hi,

Please find below a patch for:

https://bugs.openjdk.java.net/browse/JDK-8169495
8169495: Add a method to set an Authenticator on a HttpURLConnection.

webrev:
http://cr.openjdk.java.net/~dfuchs/webrev_8169495/webrev.00

The public API changes are in java.net.HttpURLConnection and
java.net.Authenticator. For backward compatibility reason the
new HttpURLConnection::setAuthenticator method is not abstract,
but is instead implemented to throw UOE unless overridden.

Again for compatibility reasons, if no authenticator is explicitly
supplied to the connection then the behavior is unchanged: the
default authenticator will be invoked, if needed, at the time
credentials are requested through the HTTP protocol.

Here is the description of the new HttpURLConnection::setAuthenticator 
method:


 /**
+ * Supplies an {@link java.net.Authenticator Authenticator} to be used
+ * when authentication is requested through the HTTP protocol for
+ * this {@code HttpURLConnection}.
+ * If no authenticator is supplied, the
+ * {@linkplain Authenticator#setDefault(java.net.Authenticator) default
+ * authenticator} will be used.
+ *
+ * @implNote The default behavior of this method is to throw
+ *   {@link UnsupportedOperationException}. Concrete
+ *   implementations of {@code HttpURLConnection}
+ *   which support supplying an {@code Authenticator} for a
+ *   specific {@code HttpURLConnection} instance should
+ *   override this method to implement a different behavior.
+ *
+ * @implSpec Depending on authentication schemes, an implementation
+ *   may or may not need to use the provided authenticator
+ *   to obtain a password. For instance, an implementation that
+ *   relies on third-party security libraries may still 
invoke the

+ *   default authenticator if these libraries are configured
+ *   to do so.
+ *   Likewise, an implementation that supports transparent
+ *   NTLM authentication may let the system attempt
+ *   to connect using the system user credentials first,
+ *   before invoking the provided authenticator.
+ *   
+ *   However, if an authenticator is specifically provided,
+ *   then the underlying connection may only be reused for
+ *   {@code HttpURLConnection} instances which share the same
+ *   {@code Authenticator} instance, and authentication 
information,
+ *   if cached, may only be reused for an {@code 
HttpURLConnection}

+ *   sharing that same {@code Authenticator}.
+ *
+ * @param auth The {@code Authenticator} that should be used by this
+ *   {@code HttpURLConnection}.
+ *
+ * @throws  UnsupportedOperationException if setting an 
Authenticator is

+ *  not supported by the underlying implementation.
+ * @throws  IllegalStateException if URLConnection is already 
connected.
+ * @throws  NullPointerException if the supplied {@code auth} is 
{@code null}.

+ * @since 9
+ */
+public void setAuthenticator(Authenticator auth) {
+throw new UnsupportedOperationException("Supplying an 
authenticator"

++ " is not supported by " + this.getClass());
+}


best regards,

-- daniel

Race Condition in initializeEncoding() function in jni_util.c

[Ronald Servant]

Hi,

While building the J9 JVM for the IBM SDK for Java(*), we've run across
what we believe is a race condition in initializeEncoding(JNIEnv*) in
jni_util.c.

      (*)  The IBM SDK for Java is comprised of the J9 JVM and OpenJDK
class library.
We've encountered a situation where (*env)->CallObjectMethod() in
JNU_GetStringPlatformChars() is invoked with an uninitialized method ID
(the static variable "String_getBytes_ID").

The function initializeEnconding() initializes 4 static variables, but only
one of them ("fastEncoding") is checked to see if initializeEnconding()
should be called in JNU_NewStringPlatform() and JNU_GetStringPlatformChars
().  Further, "fastEncoding" is the first of the 4 static variables set by
initializeEncoding() leaving a race condition where another thread sees
"fastEncoding" set but "String_getBytes_ID" is not yet initialized.

I believe that moving the initialization of "String_getBytes_ID" and
"String_init_ID" to the top of the function would eliminate the impact of
the race condition.

I'd also recommend moving the initialization of "jnuEncoding" up one line,
but this may lead to leaking a global ref.

We are able to reproduce the failure in JNU_GetStringPlatformChars() due to
using an null String_getBytes_ID variable for a method id race condition in
our environment 1 in 20.  With the following patch applied, the race
condition can no longer be reproduced.

Do you agree there is a race condition?  If so, would the patch be an
appropriate fix?  We could introduce locking, but that feels like
overkill.

Ron.

diff -r efa71dc820eb src/java.base/share/native/libjava/jni_util.c
--- a/src/java.base/share/native/libjava/jni_util.c    Mon Nov 07 13:10:42
2016 -0400
+++ b/src/java.base/share/native/libjava/jni_util.c    Wed Nov 09 17:19:16
2016 -0500
@@ -688,6 +688,15 @@

     strClazz = JNU_ClassString(env);
     CHECK_NULL(strClazz);
+
+    /* Initialize method-id cache */
+    String_getBytes_ID = (*env)->GetMethodID(env, strClazz,
+                                             "getBytes",
"(Ljava/lang/String;)[B");
+    CHECK_NULL(String_getBytes_ID);
+    String_init_ID = (*env)->GetMethodID(env, strClazz,
+                                         "",
"([BLjava/lang/String;)V");
+    CHECK_NULL(String_init_ID);
+

     propname = (*env)->NewStringUTF(env, "sun.jnu.encoding");
     if (propname) {
@@ -727,8 +736,8 @@
                              strcmp(encname, "utf-16le") == 0)
                         fastEncoding = FAST_CP1252;
                     else {
+                        jnuEncoding = (jstring)(*env)->NewGlobalRef(env,
enc);
                         fastEncoding = NO_FAST_ENCODING;
-                        jnuEncoding = (jstring)(*env)->NewGlobalRef(env,
enc);
                     }
                     (*env)->ReleaseStringUTFChars(env, enc, encname);
                 }
@@ -741,13 +750,6 @@
     }
     (*env)->DeleteLocalRef(env, propname);
     (*env)->DeleteLocalRef(env, enc);
-
-    /* Initialize method-id cache */
-    String_getBytes_ID = (*env)->GetMethodID(env, strClazz,
-                                             "getBytes",
"(Ljava/lang/String;)[B");
-    CHECK_NULL(String_getBytes_ID);
-    String_init_ID = (*env)->GetMethodID(env, strClazz,
-                                         "",
"([BLjava/lang/String;)V");
 }

Re: JDK 9 RFR of JDK-8169041: com/sun/corba/cachedSocket should be added to exclusiveAccess.dirs

[Chris Hegarty]

Looks ok Amy.

-Chris.

> On 10 Nov 2016, at 03:31, Amy Lu  wrote:
> 
> Please review the patch to add com/sun/corba/cachedSocket to 
> exclusiveAccess.dirs so as tests won't run concurrently.
> 
> This is to address one potential issue that corba tests (start and use orbd 
> service) may run into port conflict issue if run in concurrency. Most of such 
> tests are under javax/rmi and already in exclusiveAccess.dirs. Test 
> com/sun/corba/cachedSocket/7056731.sh also runs orbd and should not be run 
> concurrently.
> 
> bug: https://bugs.openjdk.java.net/browse/JDK-8169041
> webrev: http://cr.openjdk.java.net/~amlu/8169041/webrev.00
> 
> Thanks,
> Amy
> 
> --- old/test/TEST.ROOT2016-11-10 11:05:37.0 +0800
> +++ new/test/TEST.ROOT2016-11-10 11:05:36.0 +0800
> @@ -18,7 +18,7 @@
> othervm.dirs=java/awt java/beans javax/accessibility javax/imageio 
> javax/sound javax/print javax/management com/sun/awt sun/awt sun/java2d 
> sun/pisces javax/xml/jaxp/testng/validation java/lang/ProcessHandle
>  # Tests that cannot run concurrently
> -exclusiveAccess.dirs=java/rmi/Naming java/util/prefs 
> sun/management/jmxremote sun/tools/jstatd sun/security/mscapi 
> java/util/stream javax/rmi
> +exclusiveAccess.dirs=java/rmi/Naming java/util/prefs 
> sun/management/jmxremote sun/tools/jstatd sun/security/mscapi 
> java/util/stream javax/rmi com/sun/corba/cachedSocket
>  # Group definitions
> groups=TEST.groups [closed/TEST.groups]
> 
> 

Re: [9] RFR: 8167240: Write new tests to cover functionality of existing 'jimage' options

[Denis Kononenko]

Hi Andrey,

No, it isn't. I submitted a new CR: 
https://bugs.openjdk.java.net/browse/JDK-8167384.

Thank you,
Denis.

> Hi,
> 
> Looks OK.  Is it 100% pass rate?
> 
> —Andrey
> > On 9 Nov 2016, at 20:36, Denis Kononenko
>  wrote:
> > 
> > 
> > 
> > Hi,
> > 
> > After discussion with Andrey we decided to add more tests for corner
> cases.
> > The new changes are available by the link below.
> > 
> > WEBREV: http://cr.openjdk.java.net/~dkononenko/8167240/webrev.03/
> > BUGURL: https://bugs.openjdk.java.net/browse/JDK-8167240
> > 
> > 
> > Thank you,
> > Denis.
> > 
> >> Hi,
> >> 
> >> Looks OK to me.
> >> I can suggest two more cases. A directory and file symlink can be
> >> passed in options where tool requires a file path. 
> >> 
> >> —Andrey
> >> 
> >>> On 8 Nov 2016, at 16:17, Denis Kononenko
> >>  wrote:
> >>> 
> >>> 
> >>> Hi,
> >>> 
> >>> The new version of changes.
> >>> 
> >>> - Switched back to jdk/test/testlibrary to avoid unwanted
> >> dependencies (JImageToolTest.java);
> >>> - Verified tests on smallest possible JDK build.
> >>> 
> >>> WEBREV: http://cr.openjdk.java.net/~dkononenko/8167240/webrev.02/
> >>> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8167240
> >>> 
> >>> Thank you,
> >>> Denis.
> >>> 
>  Denis,
>  
>  I can see that you have switched to the top level test library
> >> with
>  this change. With that you are getting more module dependencies
> >> than 
>  just java.base. First of all, it would probably make sense to
> >> build
>  only the classes you needed (which would be 
>  jdk.test.lib.process.ProcessTools, I assume), but even if you
> only
>  build that, jdk.test.lib.process.ProcessTools has dependencies
> >> outside
>  java.base module.
>  
>  You either have to declare @modules in your test or go back to
> the
>  jdk/test/lib/testlibrary. Then, of course, unneeded module
>  dependencies are questionable.
>  
>  Shura
>  
>  
> > On Nov 3, 2016, at 6:29 AM, Denis Kononenko
>   wrote:
> > 
> > Hi,
> > 
> > I've done some rework accordingly to Alan's and Shura's
> comments:
> > 
> > 1) removed overlapped tests from JImageToolTest.java;
> > 
> > 2) added new tests JImageVerifyTest.java for jimage verify;
> > 
> > 3) reorganized jtreg's tags;
> > 
> > The new WEBREV can be found here:
>  http://cr.openjdk.java.net/~dkononenko/8167240/webrev.01/
> > 
> > Thank you,
> > Denis.
> > 
> > On 06.10.2016 19:37, Denis Kononenko wrote:
> >> Hi,
> >> 
> >> Could someone please review these new tests for jimage
> utility.
> >> 
> >> There're 5 new files containing tests to cover use cases for
>  'info', 'list', 'extract' and other options. No new tests for
>  'verify'.
> >> 
> >> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8167240
> >> WEBREV:
> >> http://cr.openjdk.java.net/~dkononenko/8167240/webrev.00/
> >> 
> >> 
> >> Thank you,
> >> Denis.
> >

RFR: jdk8u-dev Backport of 8169191: (tz) Support tzdata2016i

[Ramanand Patil]

Hi all,
Please review the latest TZDATA integration (tzdata2016i) to JDK8U.
Since tzdata is cumulative, this bug fix backports both the tzdata 
versions(tzdata2016h+tzdata2016i) into jdk8u.

Bug: https://bugs.openjdk.java.net/browse/JDK-8169191
Webrev: http://cr.openjdk.java.net/~rpatil/8169537/webrev.00/

Jdk9 changeset: http://hg.openjdk.java.net/jdk9/dev/jdk/rev/7f7091c1dd33

For reference:
Jdk9 changeset for tzdata2016h integration(JDK-8168512): 
http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/3192d7aa428d

All the TimeZone related tests are passed after integration.


Regards,
Ramanand.

Re: 8169425: Values computed by a ClassValue should not strongly reference the ClassValue

[Jochen Theodorou]

On 09.11.2016 19:18, Doug Lea wrote:
[...]

And: Similar cases can indeed occur with ThreadLocal, but users seem
to mostly avoid them, sometimes after painful experience.


ThreadLocal is already a bad choice once you subclass it.

bye Jochen

Re: 8169425: Values computed by a ClassValue should not strongly reference the ClassValue

[Jochen Theodorou]

On 09.11.2016 17:47, Paul Sandoz wrote:

Hi Peter,

Good point about if such support was added it would break the API and also 
(with Remi) about Ephemerons.

You are correct in stating the constraints in more detail regarding classes in 
the same loader. However, i think that is going into more detail than I would 
prefer for what i think is an edge case.


I would not regard that an edge case. For a dynamic language using 
ClassValue to store information, it is very easy to produce a memory 
leak with ClassValue.



So I want in general to warn developers away from strongly referencing this 
ClassValue in the computed value for any associated class.

If we do get strong feedback that this is a real problem we could consider 
adding a clever little static method like you suggest, with caveats that the 
computing Function might go away.


for us it is such a strong problem, that we are unable to transfer the 
old structure to use ClassValue without a major rewrite of large parts.
Just imagine a runtime that uses ClassValue and that your application, 
let it be a web application, will spawn many runtimes over time. You do 
want the runtimes and all computed values be able to be garbage 
collected. But if you will need at the same time a ClassValue to not to 
prevent a class we computed the value for from unloading and have the 
computed value alive for min(lifespan class, lifespan runtime), then you 
get a real big problem in realizing this.


As it stands for me ClassValue is only usable as a class associated 
cache with values you can recreate at any moment. That is not good 
enough for us in the general case.


If that is an edge case I still miss a major part in the 
documentation... and that is what a ClassValue should be used for 
instead of a cryptic and incomplete description of what a ClassValue is. 
And then we could talk about if the intended use and the actual 
usability fit together


bye Jcohen