Re: RFR 8159245: Loggers created by system classes are not initialized correctly when configured programmatically from application code.

[Hamlin Li]

Hi Daniel,

Got you point.

Thank you
-Hamlin

On 2016/6/22 13:24, Daniel Fuchs wrote:

Hi Hamlin,

On 22/06/16 02:40, Hamlin Li wrote:

Just another minor comment, I'm not sure if following line is necessary
in Logger.java, as it's already been checked in LogManager.java line 
577:


439 if (cfg == other) return cfg;


Yes, you're right - but it makes sense for the method to have it :-)

The reason it's already checked in LogManager is to avoid the
creation and invocation of a PrivilegedAction for nothing.
If we did not need to go through that - we would just call
mergeConfigWithSystemPeer and let the importConfig config
method do the check.

best regards,

-- daniel

Re: RFR: 8153955: java.util.logging.FileHandler can not create file synchronously over 101 access

[Daniel Fuchs]

Hi Ramanand,

This looks good to me.

best regards,

-- daniel

On 22/06/16 06:21, Ramanand Patil wrote:

Thank you Sean.

Updated Webrev: http://cr.openjdk.java.net/~rpatil/8153955/webrev.03/


Regards,
Ramanand.

-Original Message-
From: Seán Coffey
Sent: Friday, June 17, 2016 9:34 PM
To: Ramanand Patil; Daniel Fuchs; Bernd Eckenfels; 
core-libs-dev@openjdk.java.net
Subject: Re: RFR: 8153955: java.util.logging.FileHandler can not create file 
synchronously over 101 access

Looks fine Ramanand. Please ensure the CCC is approved before pushing.

2 comments on the testcase :
fileHendlers  --> fileHandlers.

Can you consider using the
jdk.testlibrary.FileUtils.deleteFileIfExistsWithRetry for removal of directory 
that you create. It should be more stable in the long run.

Regards,
Sean.

On 17/06/16 12:19, Ramanand Patil wrote:

Hi All,

Gentle reminder...

Regards,
Ramanand.

-Original Message-
From: Ramanand Patil
Sent: Tuesday, June 14, 2016 9:51 PM
To: Daniel Fuchs; Bernd Eckenfels; core-libs-dev@openjdk.java.net
Subject: RE: RFR: 8153955: java.util.logging.FileHandler can not
create file synchronously over 101 access

Hi all,

May I request one more review for this bug fix.?

Thank you very much Daniel for your review.


Regards,
Ramanand.

-Original Message-
From: Daniel Fuchs
Sent: Friday, June 10, 2016 7:47 PM
To: Ramanand Patil; Bernd Eckenfels; core-libs-dev@openjdk.java.net
Subject: Re: RFR: 8153955: java.util.logging.FileHandler can not
create file synchronously over 101 access

Hi Ramanand,

Looks good to me now.

best regards,

-- daniel

On 10/06/16 14:47, Ramanand Patil wrote:

Hi Daniel and Bernd,
Thank you for your reviews.
Here is the updated Webrev:
http://cr.openjdk.java.net/~rpatil/8153955/webrev.02/

Bernd,
Considering the information that "if the FileHandler tries to open the filename and 
finds the file is currently in use by another process it will increment the unique number 
field and try again" what you suggested looks correct. But using 
concurrent/synchronous too should not make it wrong, because at this time all the files 
are used simultaneously and any file which is not in use will be opened and used by 
FileHandler. I think this should not make a big difference, but as per your suggestion I 
have altered only that section of logging.properties.
Test is also updated with the suggested comment. Thank you.

Daniel,
1. FileHandler.java: Changed the wordings as you suggested.
2. FileHandlerMaxLocksTest::main
- Now using "user.dir" in createLoggerDir(). I saw some tests(like 
java/util/logging/FileHandlerPath.java) were already using user home or temp directory, 
so thought that should not be a problem.
- Removed the WeekReference and using ArrayList for all the FileHandler 
instances.
- Tested on Windows, Linux(Ubuntu14.04) and Solaris and test runs 
fine(able to delete all the log files and loggerDir). Hopefully, test will not 
have any issues on other platforms.(JPRT job is also passed).


Regards,
Ramanand.

-Original Message-
From: Bernd Eckenfels [mailto:e...@zusammenkunft.net]
Sent: Thursday, June 09, 2016 11:16 PM
To: Daniel Fuchs; core-libs-dev@openjdk.java.net
Subject: Re: RFR: 8153955: java.util.logging.FileHandler can not
create file synchronously over 101 access

Hello,

I find the "concurrent/synchronous" comment a bit confusing.

How about:

# Number of attempts to obtain lock file in FileHandler # Implemented
by incrementing the %u placeholder as documented # in FileHandler
Javadoc

In the test I would add a comment

"200 raises the default limit of 100, we try 102 times"



Gruss
Bernd


Am Thu, 9 Jun 2016 07:31:25 +0100
schrieb Daniel Fuchs :


Hi Ramanand,

Thanks for the updated. I still have some remarks:

FileHandler.java:

98  *specifies the maximum number of concurrent locks hold
by 99  *FileHandler (defaults 100). 

Is the verb form correct: hold => held ?

logging.properties:

42 # when the unique field %u is incremented as per the javadoc.

"... as per the javadoc" => "... as per FileHandler API documentation"

FileHandlerMaxLocksTest.java:

- FileHandlerMaxLocksTest::createLoggerDir():

75 String tmpDir = System.getProperty("java.io.tmpdir");
76 if (tmpDir == null) {
77 tmpDir = System.getProperty("user.home");
78 }
79 File tmpOrHomeDir = new File(tmpDir);
80 File loggerDir = new File(tmpOrHomeDir, LOGGER_DIR);

The preferred place for a test to create file is the scratch
directory that jtreg creates for the test. The scratch directory
location is available from the "user.dir" system property.

I suggest changing the code above to:

 String userDir =   System.getProperty("user.dir", ".");
 File loggerDir = new File(userDir, LOGGER_DIR);

- FileHandlerMaxLocksTest::main

I am not sure what you try to achieve by using WeakReference there.
I would suggest to store all created FileHandler insta

Re: RFR 8159245: Loggers created by system classes are not initialized correctly when configured programmatically from application code.

[Daniel Fuchs]

Hi Hamlin,

On 22/06/16 02:40, Hamlin Li wrote:

Just another minor comment, I'm not sure if following line is necessary
in Logger.java, as it's already been checked in LogManager.java line 577:

439 if (cfg == other) return cfg;


Yes, you're right - but it makes sense for the method to have it :-)

The reason it's already checked in LogManager is to avoid the
creation and invocation of a PrivilegedAction for nothing.
If we did not need to go through that - we would just call
mergeConfigWithSystemPeer and let the importConfig config
method do the check.

best regards,

-- daniel

RE: RFR: 8153955: java.util.logging.FileHandler can not create file synchronously over 101 access

[Ramanand Patil]

Thank you Sean.

Updated Webrev: http://cr.openjdk.java.net/~rpatil/8153955/webrev.03/


Regards,
Ramanand.

-Original Message-
From: Seán Coffey 
Sent: Friday, June 17, 2016 9:34 PM
To: Ramanand Patil; Daniel Fuchs; Bernd Eckenfels; 
core-libs-dev@openjdk.java.net
Subject: Re: RFR: 8153955: java.util.logging.FileHandler can not create file 
synchronously over 101 access

Looks fine Ramanand. Please ensure the CCC is approved before pushing.

2 comments on the testcase :
fileHendlers  --> fileHandlers.

Can you consider using the
jdk.testlibrary.FileUtils.deleteFileIfExistsWithRetry for removal of directory 
that you create. It should be more stable in the long run.

Regards,
Sean.

On 17/06/16 12:19, Ramanand Patil wrote:
> Hi All,
>
> Gentle reminder...
>
> Regards,
> Ramanand.
>
> -Original Message-
> From: Ramanand Patil
> Sent: Tuesday, June 14, 2016 9:51 PM
> To: Daniel Fuchs; Bernd Eckenfels; core-libs-dev@openjdk.java.net
> Subject: RE: RFR: 8153955: java.util.logging.FileHandler can not 
> create file synchronously over 101 access
>
> Hi all,
>
> May I request one more review for this bug fix.?
>
> Thank you very much Daniel for your review.
>
>
> Regards,
> Ramanand.
>
> -Original Message-
> From: Daniel Fuchs
> Sent: Friday, June 10, 2016 7:47 PM
> To: Ramanand Patil; Bernd Eckenfels; core-libs-dev@openjdk.java.net
> Subject: Re: RFR: 8153955: java.util.logging.FileHandler can not 
> create file synchronously over 101 access
>
> Hi Ramanand,
>
> Looks good to me now.
>
> best regards,
>
> -- daniel
>
> On 10/06/16 14:47, Ramanand Patil wrote:
>> Hi Daniel and Bernd,
>> Thank you for your reviews.
>> Here is the updated Webrev:
>> http://cr.openjdk.java.net/~rpatil/8153955/webrev.02/
>>
>> Bernd,
>> Considering the information that "if the FileHandler tries to open the 
>> filename and finds the file is currently in use by another process it will 
>> increment the unique number field and try again" what you suggested looks 
>> correct. But using concurrent/synchronous too should not make it wrong, 
>> because at this time all the files are used simultaneously and any file 
>> which is not in use will be opened and used by FileHandler. I think this 
>> should not make a big difference, but as per your suggestion I have altered 
>> only that section of logging.properties.
>> Test is also updated with the suggested comment. Thank you.
>>
>> Daniel,
>> 1. FileHandler.java: Changed the wordings as you suggested.
>> 2. FileHandlerMaxLocksTest::main
>>  - Now using "user.dir" in createLoggerDir(). I saw some tests(like 
>> java/util/logging/FileHandlerPath.java) were already using user home or temp 
>> directory, so thought that should not be a problem.
>>  - Removed the WeekReference and using ArrayList for all the FileHandler 
>> instances.
>>  - Tested on Windows, Linux(Ubuntu14.04) and Solaris and test runs 
>> fine(able to delete all the log files and loggerDir). Hopefully, test will 
>> not have any issues on other platforms.(JPRT job is also passed).
>>
>>
>> Regards,
>> Ramanand.
>>
>> -Original Message-
>> From: Bernd Eckenfels [mailto:e...@zusammenkunft.net]
>> Sent: Thursday, June 09, 2016 11:16 PM
>> To: Daniel Fuchs; core-libs-dev@openjdk.java.net
>> Subject: Re: RFR: 8153955: java.util.logging.FileHandler can not 
>> create file synchronously over 101 access
>>
>> Hello,
>>
>> I find the "concurrent/synchronous" comment a bit confusing.
>>
>> How about:
>>
>> # Number of attempts to obtain lock file in FileHandler # Implemented 
>> by incrementing the %u placeholder as documented # in FileHandler 
>> Javadoc
>>
>> In the test I would add a comment
>>
>> "200 raises the default limit of 100, we try 102 times"
>>
>>
>>
>> Gruss
>> Bernd
>>
>>
>> Am Thu, 9 Jun 2016 07:31:25 +0100
>> schrieb Daniel Fuchs :
>>
>>> Hi Ramanand,
>>>
>>> Thanks for the updated. I still have some remarks:
>>>
>>> FileHandler.java:
>>>
>>> 98  *specifies the maximum number of concurrent locks hold
>>> by 99  *FileHandler (defaults 100). 
>>>
>>> Is the verb form correct: hold => held ?
>>>
>>> logging.properties:
>>>
>>> 42 # when the unique field %u is incremented as per the javadoc.
>>>
>>> "... as per the javadoc" => "... as per FileHandler API documentation"
>>>
>>> FileHandlerMaxLocksTest.java:
>>>
>>> - FileHandlerMaxLocksTest::createLoggerDir():
>>>
>>> 75 String tmpDir = System.getProperty("java.io.tmpdir");
>>> 76 if (tmpDir == null) {
>>> 77 tmpDir = System.getProperty("user.home");
>>> 78 }
>>> 79 File tmpOrHomeDir = new File(tmpDir);
>>> 80 File loggerDir = new File(tmpOrHomeDir, LOGGER_DIR);
>>>
>>> The preferred place for a test to create file is the scratch 
>>> directory that jtreg creates for the test. The scratch directory 
>>> location is available from the "user.dir" system property.
>>>
>>> I suggest changing the code above to:
>>>
>>>  String userDi

Re: RFR 8159245: Loggers created by system classes are not initialized correctly when configured programmatically from application code.

[Hamlin Li]

Hi Daniel,

Thank you for detailed explanation, new version makes sense to me.
Just another minor comment, I'm not sure if following line is necessary 
in Logger.java, as it's already been checked in LogManager.java line 577:


439 if (cfg == other) return cfg;

Thank you
-Hamlin

On 2016/6/21 19:54, Daniel Fuchs wrote:

Hi Hamlin,

I was mistaken in my first assessment.

The case where the system handler's list is not empty
should only happen if by misfortune two different threads
happen to attempt to merge the same two configurations
concurrently. Though of no consequence for level, filter,
etc... (single values) that would be an issue for the
handlers list if not handled correctly.

With that in mind I have slightly revised the fix - and
added a more verbose comment explaining the reason for the
isEmpty() check.

http://cr.openjdk.java.net/~dfuchs/webrev_8159245/webrev.01/index.html

best regards,

-- daniel


On 21/06/16 07:57, Daniel Fuchs wrote:

Hi Hamlin,

There's no difference in behaviour - mergeConfigWithSystemPeer is called
unconditionally but the handlers from the application logger will be
copied in the system logger config only if the system's logger
list is empty - just like before.

This is something that will probably need to be revisited - maybe
the system handlers should be closed first and the application
handlers added unconditionally.
The issue here is that if the system logger had handlers
configured from the logging.properties file then the user handlers
are going to be garbage collected before being closed - which is
not ideal - even if it should rarely happen.

Let me think on it.

Re: [9] RFR: 8022291: Mac OS: Unexpected JavaLaunchHelper message displaying

[David DeHaven]

> JBS:
> https://bugs.openjdk.java.net/browse/JDK-8022291
> 
> Webrev:
> http://cr.openjdk.java.net/~ddehaven/8022291/jdk.0/index.html
> 
> This actually turned out to be pretty easy to fix, I eliminated the 
> JavaLaunchHelper class and in place of it stuffed the block it replaced into 
> a NSBlockOperation then changed the performSelectorOnMainThread call to 
> invoke the NSBlockOperation's start message.
> 
> I tested against the SWT snippet (Snippet297) that was attached to the 
> original Eclipse bug that triggered the original fix. The SWT tests I could 
> dig up all seemed to work ok.
> 
> Original Eclipse bug, used to verify the fix:
> https://bugs.eclipse.org/bugs/show_bug.cgi?id=389486
> 
> 
> This should be backported to 8u after it bakes in 9 for a bit.


Minor update:
http://cr.openjdk.java.net/~ddehaven/8022291/jdk.1/

I put the NSAutoreleasePool back, so it's directly portable to jdk8u.

-DrD-

Re: RFR 9 7131356 : (props) "No Java runtime present, requesting install" when creating VM from JNI [macosx]

[Naoto Sato]

Actually, j.u.Locale class' "country" code is defined as ISO-3166 
alpha-2 *or* UN M.49 numeric-3 area code, so if the OSX's underlying 
setting is "es-419" for the preferred language, "user.country" should be 
"419".


Naoto

On 6/21/16 3:17 PM, Brent Christian wrote:

Hi, Naoto

"Spanish (Latin America)" already works the same as it did before the
change - it maps to "es_XL".  Because "es-419" has more than 2
characters following the '-', no change is made and
getMacOSXLocale(LC_MESSAGES) returns "es-419".  I added a mapping for
"es-419" -> "es_XL" in locale_str.h.

System property values are (still) set to:
  user.language: es
  user.country: XL
  user.country.format: (2-char country code for the selected Region)

Thanks,
-Brent

On 21/06/16 14:48, Naoto Sato wrote:

Hi Brent,

I looked at the system preference setting panel and noticed there is
"Spanish (Latin America)", which I think uses UN M.49 code ("es-419") as
the designate region. Can you make changes to deal with it? (sorry I
should have noticed this earlier, and it's unfortunate not to be able to
upcall Locale.forLanguageTag()!)

Naoto

On 6/20/16 4:38 PM, Brent Christian wrote:

Hi,

I have an updated webrev at:
http://cr.openjdk.java.net/~bchristi/7131356/webrev.03/

The comments have been updated as Gerard suggested.

The code to process the languageString now accounts for 3-character
language codes, and 4-char script designations (line 86).

More extensive testing of languages with multiple scripts/regions
revealed that more changes were needed to maintain current behavior.
Without knowing the internal workings of how
JRSCopyCanonicalLanguageForPrimaryLanguage adjusts the language ID, I
believe the best options are to add a few more mappings as needed to
locale_aliases (locale_str.h), and to add a special case for Portuguese
(line 104).

OS X has 3 language options for Portuguese:
Portugues (Portugal)
Portugues (Brasil)
Portugues (Brasileiro)

CFLocaleCopyPreferredLanguages() gives the expected language code for
Portugues (Brasileiro) ("pt-BR"), but "Portugues (Brasil)" doesn't
include a region code (it's simply, "pt"), so gets mapped to the default
country, Portugal.  To maintain current behavior, I added a special case
to map "pt" to "pt_BR" when the Region system preference is set to
Brazil.


This change introduces one notable behavior change, which I argue is
actually a fix.  The bug report and test case do not list the "Spanish
(Mexico)" language, but it is present on MaxOS 10.9 (presumably it was
added to the OS since the original investigation).  The old code mapped
this to "es_XL", XL being one of the "user-assigned" ISO 3166 country
codes.  The new code maps to "es_MX", MX being the country code for
Mexico.


I've tested pretty thoroughly on MacOS 10.9; I'm pretty sure I tried
every language that includes multiple scripts/locales.  I also added a
couple updated tests to the bug report.

General testing has looked good so far.

Thanks,
-Brent

Re: RFR 9 7131356 : (props) "No Java runtime present, requesting install" when creating VM from JNI [macosx]

[Brent Christian]

Hi, Naoto

"Spanish (Latin America)" already works the same as it did before the 
change - it maps to "es_XL".  Because "es-419" has more than 2 
characters following the '-', no change is made and 
getMacOSXLocale(LC_MESSAGES) returns "es-419".  I added a mapping for 
"es-419" -> "es_XL" in locale_str.h.


System property values are (still) set to:
  user.language: es
  user.country: XL
  user.country.format: (2-char country code for the selected Region)

Thanks,
-Brent

On 21/06/16 14:48, Naoto Sato wrote:

Hi Brent,

I looked at the system preference setting panel and noticed there is
"Spanish (Latin America)", which I think uses UN M.49 code ("es-419") as
the designate region. Can you make changes to deal with it? (sorry I
should have noticed this earlier, and it's unfortunate not to be able to
upcall Locale.forLanguageTag()!)

Naoto

On 6/20/16 4:38 PM, Brent Christian wrote:

Hi,

I have an updated webrev at:
http://cr.openjdk.java.net/~bchristi/7131356/webrev.03/

The comments have been updated as Gerard suggested.

The code to process the languageString now accounts for 3-character
language codes, and 4-char script designations (line 86).

More extensive testing of languages with multiple scripts/regions
revealed that more changes were needed to maintain current behavior.
Without knowing the internal workings of how
JRSCopyCanonicalLanguageForPrimaryLanguage adjusts the language ID, I
believe the best options are to add a few more mappings as needed to
locale_aliases (locale_str.h), and to add a special case for Portuguese
(line 104).

OS X has 3 language options for Portuguese:
Portugues (Portugal)
Portugues (Brasil)
Portugues (Brasileiro)

CFLocaleCopyPreferredLanguages() gives the expected language code for
Portugues (Brasileiro) ("pt-BR"), but "Portugues (Brasil)" doesn't
include a region code (it's simply, "pt"), so gets mapped to the default
country, Portugal.  To maintain current behavior, I added a special case
to map "pt" to "pt_BR" when the Region system preference is set to
Brazil.


This change introduces one notable behavior change, which I argue is
actually a fix.  The bug report and test case do not list the "Spanish
(Mexico)" language, but it is present on MaxOS 10.9 (presumably it was
added to the OS since the original investigation).  The old code mapped
this to "es_XL", XL being one of the "user-assigned" ISO 3166 country
codes.  The new code maps to "es_MX", MX being the country code for
Mexico.


I've tested pretty thoroughly on MacOS 10.9; I'm pretty sure I tried
every language that includes multiple scripts/locales.  I also added a
couple updated tests to the bug report.

General testing has looked good so far.

Thanks,
-Brent

Re: RFR 9 7131356 : (props) "No Java runtime present, requesting install" when creating VM from JNI [macosx]

[Naoto Sato]

Hi Brent,

I looked at the system preference setting panel and noticed there is 
"Spanish (Latin America)", which I think uses UN M.49 code ("es-419") as 
the designate region. Can you make changes to deal with it? (sorry I 
should have noticed this earlier, and it's unfortunate not to be able to 
upcall Locale.forLanguageTag()!)


Naoto

On 6/20/16 4:38 PM, Brent Christian wrote:

Hi,

I have an updated webrev at:
http://cr.openjdk.java.net/~bchristi/7131356/webrev.03/

The comments have been updated as Gerard suggested.

The code to process the languageString now accounts for 3-character
language codes, and 4-char script designations (line 86).

More extensive testing of languages with multiple scripts/regions
revealed that more changes were needed to maintain current behavior.
Without knowing the internal workings of how
JRSCopyCanonicalLanguageForPrimaryLanguage adjusts the language ID, I
believe the best options are to add a few more mappings as needed to
locale_aliases (locale_str.h), and to add a special case for Portuguese
(line 104).

OS X has 3 language options for Portuguese:
Portugues (Portugal)
Portugues (Brasil)
Portugues (Brasileiro)

CFLocaleCopyPreferredLanguages() gives the expected language code for
Portugues (Brasileiro) ("pt-BR"), but "Portugues (Brasil)" doesn't
include a region code (it's simply, "pt"), so gets mapped to the default
country, Portugal.  To maintain current behavior, I added a special case
to map "pt" to "pt_BR" when the Region system preference is set to Brazil.


This change introduces one notable behavior change, which I argue is
actually a fix.  The bug report and test case do not list the "Spanish
(Mexico)" language, but it is present on MaxOS 10.9 (presumably it was
added to the OS since the original investigation).  The old code mapped
this to "es_XL", XL being one of the "user-assigned" ISO 3166 country
codes.  The new code maps to "es_MX", MX being the country code for Mexico.


I've tested pretty thoroughly on MacOS 10.9; I'm pretty sure I tried
every language that includes multiple scripts/locales.  I also added a
couple updated tests to the bug report.

General testing has looked good so far.

Thanks,
-Brent

Re: [9] RFR: 8159548: Formatter returns unexpected strings if locale is null.

[Xueming Shen]

+1

On 6/16/16, 2:48 PM, Naoto Sato wrote:

Hi,

Please review the fix to the following issue:

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

The proposed fix is located at:

http://cr.openjdk.java.net/~naoto/8159548/webrev.00/

The previous fix to 8146156 overly localized the formatted text for 
the "null" locale. The offending localization code were reverted to 
the original.


Naoto

Re: RFR: 8150173: JAXBContext.newInstance causes PrivilegedActionException when createContext's declared in absract class extended by discovered JAXB implementation

[Mandy Chung]

> On Jun 21, 2016, at 10:39 AM, Daniel Fuchs  wrote:
> 
> Hi,
> 
> Please find below a somewhat trivial patch for
> 
> 8150173: JAXBContext.newInstance causes PrivilegedActionException
> when createContext's declared in absract class extended
> by discovered JAXB implementation
> https://bugs.openjdk.java.net/browse/JDK-8150173
> 
> Patch:
> http://cr.openjdk.java.net/~dfuchs/webrev_8150173/webrev.00
> 
> This is an oversight that was introduced with JDK-8145104.
> 
> The issue is simply that newInstance() must be invoked on
> the concrete class, not on the class that defines the
> createContext method.

Thanks for taking this one on.

 234 if (JAXBContextFactory.class.isAssignableFrom(declaringClass)

The spec says that implementation class of JAXBContextFactory must also 
implement no-arg constructor.

So I think this line is not needed.  Instead instantiateProviderIfNecessary 
should simply take the implClass parameter (the Method parameter doesn’t seem 
to be needed).

 245 throw new 
JAXBException(Messages.format(Messages.COULD_NOT_INSTANTIATE, declaringClass, 
e), e);

Since you are on this file, it looks to me that it should use e.getCause() 
instead of e.

Mandy

[9] RFR: 8022291: Mac OS: Unexpected JavaLaunchHelper message displaying

[David DeHaven]

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

Webrev:
http://cr.openjdk.java.net/~ddehaven/8022291/jdk.0/index.html

This actually turned out to be pretty easy to fix, I eliminated the 
JavaLaunchHelper class and in place of it stuffed the block it replaced into a 
NSBlockOperation then changed the performSelectorOnMainThread call to invoke 
the NSBlockOperation's start message.

I tested against the SWT snippet (Snippet297) that was attached to the original 
Eclipse bug that triggered the original fix. The SWT tests I could dig up all 
seemed to work ok.

Original Eclipse bug, used to verify the fix:
https://bugs.eclipse.org/bugs/show_bug.cgi?id=389486


This should be backported to 8u after it bakes in 9 for a bit.

-DrD-

Re: RFR: 8150680 JarFile.Release enum needs reconsideration with respect to it's values

[Steve Drach]

Hi Paul,

> Hi. I would like to point out that it appears JarFile.Release enum is 
> specifically tailored to address multi-version jar specification.

I think you are looking at the current code, not the webrev.  What the webrev 
does is remove the JarFile.Release enum.

> However, I find nothing in that enum specific except for the documentation 
> and BASE_VERSION. Wouldn't it better to create a top-level Release enum that 
> can be used to identify anything in the JDK with release semantics -- apart 
> from Jar files? 
> 
> Cheers,
> Paul
> 
> On Tue, Jun 21, 2016 at 1:23 PM, Steve Drach  > wrote:
> Hi Paul,
> 
> I believe this webrev addresses your concerns:
> 
> http://cr.openjdk.java.net/~sdrach/8150680/webrev.03/index.html 
>  
>  >
> 
> 
> > On Jun 16, 2016, at 3:49 PM, Paul Sandoz  > > wrote:
> >
> >
> >> On 16 Jun 2016, at 14:44, Steve Drach  >> > wrote:
> >>
> >> This webrev uses methods instead of fields to return the base and runtime 
> >> values used internally by JarFile.  I’ve also optimized it a bit.
> >>
> >> http://cr.openjdk.java.net/~sdrach/8150680/webrev.02/ 
> >>  
> >>  >> >
> >>
> >
> > JarFIle
> > —
> >
> > 132 private final static int base_version;
> >
> > You are using lower case, here, this caught me out as i thought it was an 
> > non-static field. Call it something like BASE_VERSION_MAJOR.
> >
> >
> > 155 BASE_VERSION = 
> > Runtime.Version.parse(String.valueOf(base_version));
> >
> > 164 RUNTIME_VERSION = 
> > Runtime.Version.parse(String.valueOf(runtimeVersion));
> >
> > Use Integer.toString rather than String.valueOf (also update specification).
> >
> >
> > 337 public final Runtime.Version getVersion() {
> > 338 if (VERSION == null) {
> > 339 if (isMultiRelease()) {
> > 340 VERSION = 
> > Runtime.Version.parse(String.valueOf(version));
> > 341 } else {
> > 342 VERSION = BASE_VERSION;
> > 343 }
> > 344 }
> > 345 return VERSION;
> > 346 }
> > 347 private Runtime.Version VERSION;
> >
> > You are using the style for a static field.
> >
> > In the JarFile constructor why don’t you just store the version passed in 
> > unless MULTI_RELEASE_FORCED?
> >
> > Have final fields:
> >
> >  final Runtime.Version version;
> >  final int version_major;
> >
> > then do:
> >
> >  if (MULTI_RELEASE_FORCED || version.major() == RUNTIME_VERSION.major()) {
> >  // This also deals with the common case where the value from 
> > JarFile.runtimeVersion() is passed
> >  this.version = RUNTIME_VERSION;
> >  } else if (version.major() <= BASE_VERSION_MAJOR) {
> >  // This also deals with the common case where the value from 
> > JarFile.baseVersion() is passed
> >  this.version = BASE_VERSION;
> >  } else {
> > // Canonicalize
> > this.version = Runtime.Version.parse(Integer.toString(version.major()));
> >  }
> >  this.version_major = version.major();
> >
> > Paul.
> >
> >
> >
> >
> >>> On Jun 15, 2016, at 4:31 PM, Joseph D. Darcy  >>> > wrote:
> >>>
> >>> Steve,
> >>>
> >>> In JarFile, please use methods not fields to return the new information. 
> >>> The information in question is not constant across versions. Using 
> >>> methods instead of fields avoid over-committing on a particular 
> >>> implementation, etc.
> >>>
> >>> Cheers,
> >>>
> >>> -Joe
> >>>
> >>> On 6/15/2016 3:49 PM, Steve Drach wrote:
>  I’ve updated the webrev to address the issue of the constructor 
>  accepting values like Version.parse(“7.1”)
> 
>  http://cr.openjdk.java.net/~sdrach/8150680/webrev.01/ 
>   
>    >
> 
> > On Jun 15, 2016, at 8:56 AM, Steve Drach  > > wrote:
> >
> >>> Please review the following changeset:
> >>>
> >>> webrev: 
> >>> http://cr.openjdk.java.net/~sdrach/8150680/webrev.00/index.html 
> >>>  
> >>>  >>> >
> >>> issue: https://bugs.openjdk.java.net/browse/JDK-8150680 
> >>>  
> >>>  >>> 

Re: RFR: 8150680 JarFile.Release enum needs reconsideration with respect to it's values

[Paul Benedict]

Hi. I would like to point out that it appears JarFile.Release enum is
specifically tailored to address multi-version jar specification. However,
I find nothing in that enum specific except for the documentation and
BASE_VERSION. Wouldn't it better to create a top-level Release enum that
can be used to identify anything in the JDK with release semantics -- apart
from Jar files?

Cheers,
Paul

On Tue, Jun 21, 2016 at 1:23 PM, Steve Drach  wrote:

> Hi Paul,
>
> I believe this webrev addresses your concerns:
>
> http://cr.openjdk.java.net/~sdrach/8150680/webrev.03/index.html <
> http://cr.openjdk.java.net/~sdrach/8150680/webrev.03/index.html>
>
>
> > On Jun 16, 2016, at 3:49 PM, Paul Sandoz  wrote:
> >
> >
> >> On 16 Jun 2016, at 14:44, Steve Drach  wrote:
> >>
> >> This webrev uses methods instead of fields to return the base and
> runtime values used internally by JarFile.  I’ve also optimized it a bit.
> >>
> >> http://cr.openjdk.java.net/~sdrach/8150680/webrev.02/ <
> http://cr.openjdk.java.net/~sdrach/8150680/webrev.02/>
> >>
> >
> > JarFIle
> > —
> >
> > 132 private final static int base_version;
> >
> > You are using lower case, here, this caught me out as i thought it was
> an non-static field. Call it something like BASE_VERSION_MAJOR.
> >
> >
> > 155 BASE_VERSION =
> Runtime.Version.parse(String.valueOf(base_version));
> >
> > 164 RUNTIME_VERSION =
> Runtime.Version.parse(String.valueOf(runtimeVersion));
> >
> > Use Integer.toString rather than String.valueOf (also update
> specification).
> >
> >
> > 337 public final Runtime.Version getVersion() {
> > 338 if (VERSION == null) {
> > 339 if (isMultiRelease()) {
> > 340 VERSION =
> Runtime.Version.parse(String.valueOf(version));
> > 341 } else {
> > 342 VERSION = BASE_VERSION;
> > 343 }
> > 344 }
> > 345 return VERSION;
> > 346 }
> > 347 private Runtime.Version VERSION;
> >
> > You are using the style for a static field.
> >
> > In the JarFile constructor why don’t you just store the version passed
> in unless MULTI_RELEASE_FORCED?
> >
> > Have final fields:
> >
> >  final Runtime.Version version;
> >  final int version_major;
> >
> > then do:
> >
> >  if (MULTI_RELEASE_FORCED || version.major() == RUNTIME_VERSION.major())
> {
> >  // This also deals with the common case where the value from
> JarFile.runtimeVersion() is passed
> >  this.version = RUNTIME_VERSION;
> >  } else if (version.major() <= BASE_VERSION_MAJOR) {
> >  // This also deals with the common case where the value from
> JarFile.baseVersion() is passed
> >  this.version = BASE_VERSION;
> >  } else {
> > // Canonicalize
> > this.version =
> Runtime.Version.parse(Integer.toString(version.major()));
> >  }
> >  this.version_major = version.major();
> >
> > Paul.
> >
> >
> >
> >
> >>> On Jun 15, 2016, at 4:31 PM, Joseph D. Darcy 
> wrote:
> >>>
> >>> Steve,
> >>>
> >>> In JarFile, please use methods not fields to return the new
> information. The information in question is not constant across versions.
> Using methods instead of fields avoid over-committing on a particular
> implementation, etc.
> >>>
> >>> Cheers,
> >>>
> >>> -Joe
> >>>
> >>> On 6/15/2016 3:49 PM, Steve Drach wrote:
>  I’ve updated the webrev to address the issue of the constructor
> accepting values like Version.parse(“7.1”)
> 
>  http://cr.openjdk.java.net/~sdrach/8150680/webrev.01/ <
> http://cr.openjdk.java.net/~sdrach/8150680/webrev.01/>
> 
> > On Jun 15, 2016, at 8:56 AM, Steve Drach 
> wrote:
> >
> >>> Please review the following changeset:
> >>>
> >>> webrev:
> http://cr.openjdk.java.net/~sdrach/8150680/webrev.00/index.html <
> http://cr.openjdk.java.net/~sdrach/8150680/webrev.00/index.html>
> >>> issue: https://bugs.openjdk.java.net/browse/JDK-8150680 <
> https://bugs.openjdk.java.net/browse/JDK-8150680>
> >>>
> >>> The issue calls for reconsidering the JarFile.Release enum.  A
> comment in the bug report suggests replacing JarFile.Release with
> Runtime.Version, and that’s what I did.  Specifically I removed the enum,
> changed the constructor to accept a Runtime.Version object instead of a
> JarFile.Release object, updated all places in the JDK that invoked the
> constructor and updated all tests.
> >>>
> >> Moving to Runtime.Version seems right but doesn't the javadoc for
> the constructor need to be updated to make it clear how it behavior when
> invoking with something like Version.parse("7.1") ? If I read the code
> correctly then this will be accepted and getVersion() will return 7.1.
> > Yes, it needs to be updated and it needs to be fixed.  Thanks for
> finding that.
> >
> >> Fields or methods is another discussion point for the base and
> runtime versions.
> > My thinking is, in this case fields and methods are equivalent, the
> method not giving any more flexibility than a field.  For e

Re: RFR: 8150680 JarFile.Release enum needs reconsideration with respect to it's values

[Steve Drach]

Hi Paul,

I believe this webrev addresses your concerns:

http://cr.openjdk.java.net/~sdrach/8150680/webrev.03/index.html 



> On Jun 16, 2016, at 3:49 PM, Paul Sandoz  wrote:
> 
> 
>> On 16 Jun 2016, at 14:44, Steve Drach  wrote:
>> 
>> This webrev uses methods instead of fields to return the base and runtime 
>> values used internally by JarFile.  I’ve also optimized it a bit.
>> 
>> http://cr.openjdk.java.net/~sdrach/8150680/webrev.02/ 
>> 
>> 
> 
> JarFIle
> —
> 
> 132 private final static int base_version;
> 
> You are using lower case, here, this caught me out as i thought it was an 
> non-static field. Call it something like BASE_VERSION_MAJOR.
> 
> 
> 155 BASE_VERSION = 
> Runtime.Version.parse(String.valueOf(base_version));
> 
> 164 RUNTIME_VERSION = 
> Runtime.Version.parse(String.valueOf(runtimeVersion));
> 
> Use Integer.toString rather than String.valueOf (also update specification).
> 
> 
> 337 public final Runtime.Version getVersion() {
> 338 if (VERSION == null) {
> 339 if (isMultiRelease()) {
> 340 VERSION = Runtime.Version.parse(String.valueOf(version));
> 341 } else {
> 342 VERSION = BASE_VERSION;
> 343 }
> 344 }
> 345 return VERSION;
> 346 }
> 347 private Runtime.Version VERSION;
> 
> You are using the style for a static field.
> 
> In the JarFile constructor why don’t you just store the version passed in 
> unless MULTI_RELEASE_FORCED?
> 
> Have final fields:
> 
>  final Runtime.Version version;
>  final int version_major;
> 
> then do:
> 
>  if (MULTI_RELEASE_FORCED || version.major() == RUNTIME_VERSION.major()) {
>  // This also deals with the common case where the value from 
> JarFile.runtimeVersion() is passed
>  this.version = RUNTIME_VERSION;
>  } else if (version.major() <= BASE_VERSION_MAJOR) {
>  // This also deals with the common case where the value from 
> JarFile.baseVersion() is passed
>  this.version = BASE_VERSION;
>  } else {
> // Canonicalize
> this.version = Runtime.Version.parse(Integer.toString(version.major()));
>  }
>  this.version_major = version.major();
> 
> Paul.
> 
> 
> 
> 
>>> On Jun 15, 2016, at 4:31 PM, Joseph D. Darcy  wrote:
>>> 
>>> Steve,
>>> 
>>> In JarFile, please use methods not fields to return the new information. 
>>> The information in question is not constant across versions. Using methods 
>>> instead of fields avoid over-committing on a particular implementation, etc.
>>> 
>>> Cheers,
>>> 
>>> -Joe
>>> 
>>> On 6/15/2016 3:49 PM, Steve Drach wrote:
 I’ve updated the webrev to address the issue of the constructor accepting 
 values like Version.parse(“7.1”)
 
 http://cr.openjdk.java.net/~sdrach/8150680/webrev.01/ 
 
 
> On Jun 15, 2016, at 8:56 AM, Steve Drach  wrote:
> 
>>> Please review the following changeset:
>>> 
>>> webrev: http://cr.openjdk.java.net/~sdrach/8150680/webrev.00/index.html 
>>> 
>>> issue: https://bugs.openjdk.java.net/browse/JDK-8150680 
>>> 
>>> 
>>> The issue calls for reconsidering the JarFile.Release enum.  A comment 
>>> in the bug report suggests replacing JarFile.Release with 
>>> Runtime.Version, and that’s what I did.  Specifically I removed the 
>>> enum, changed the constructor to accept a Runtime.Version object 
>>> instead of a JarFile.Release object, updated all places in the JDK that 
>>> invoked the constructor and updated all tests.
>>> 
>> Moving to Runtime.Version seems right but doesn't the javadoc for the 
>> constructor need to be updated to make it clear how it behavior when 
>> invoking with something like Version.parse("7.1") ? If I read the code 
>> correctly then this will be accepted and getVersion() will return 7.1.
> Yes, it needs to be updated and it needs to be fixed.  Thanks for finding 
> that.
> 
>> Fields or methods is another discussion point for the base and runtime 
>> versions.
> My thinking is, in this case fields and methods are equivalent, the 
> method not giving any more flexibility than a field.  For example the 
> method JarFile.baseVersion will just return the value contained in the 
> private final static field BASE_VERSION.  Or the public final static 
> field BASE_VERSION can be directly accessed.  I see no advantage of a 
> method here.  But I’m willing to be enlightened.
> 
>> -Alan.
>>> 
>> 
> 

Re: RFR: 8150173: JAXBContext.newInstance causes PrivilegedActionException when createContext's declared in absract class extended by discovered JAXB implementation

[Lance Andersen]

Hi Daniel,

Looks OK.   I know you have run the JCK tests as well.

Best
Lance
> On Jun 21, 2016, at 1:39 PM, Daniel Fuchs  wrote:
> 
> Hi,
> 
> Please find below a somewhat trivial patch for
> 
> 8150173: JAXBContext.newInstance causes PrivilegedActionException
> when createContext's declared in absract class extended
> by discovered JAXB implementation
> https://bugs.openjdk.java.net/browse/JDK-8150173
> 
> Patch:
> http://cr.openjdk.java.net/~dfuchs/webrev_8150173/webrev.00
> 
> This is an oversight that was introduced with JDK-8145104.
> 
> The issue is simply that newInstance() must be invoked on
> the concrete class, not on the class that defines the
> createContext method.
> 
> best regards,
> 
> -- daniel

 
  

 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 8159245: Loggers created by system classes are not initialized correctly when configured programmatically from application code.

[Daniel Fuchs]

On 21/06/16 17:01, Mandy Chung wrote:



On Jun 21, 2016, at 8:39 AM, Daniel Fuchs  wrote:

I don't understand this scenario.
ConfigurationData should remain as simple as possible.
Logger.getLogger() / LogManager.demandSystemLogger() will never return
a logger before it has been configured.
When we're merging the configuration data the system logger has
not been configured yet. Level etc are already volatile in Logger and
we should not introduce any new synchronization there.


This is the scenario I was thinking about - would this ever happen?

T1 is creating a system logger named “foo” and calling this.importConfg(other) 
at line 462 after setLevel(otherLevel) is done.

this is the system logger “foo” and other is the user-created logger “foo”.

T2 is calling other.setLevel(newLevel) on user-created logger “foo”.


I see. Let me think on that one.

cheers,

-- daniel



Mandy

RFR: 8150173: JAXBContext.newInstance causes PrivilegedActionException when createContext's declared in absract class extended by discovered JAXB implementation

[Daniel Fuchs]

Hi,

Please find below a somewhat trivial patch for

8150173: JAXBContext.newInstance causes PrivilegedActionException
 when createContext's declared in absract class extended
 by discovered JAXB implementation
https://bugs.openjdk.java.net/browse/JDK-8150173

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

This is an oversight that was introduced with JDK-8145104.

The issue is simply that newInstance() must be invoked on
the concrete class, not on the class that defines the
createContext method.

best regards,

-- daniel

Re: RFR(L): 8143211: provide bytecode intrinsics for loop and try/finally executors

[Vladimir Ivanov]

InvokerBytecodeGenerator/IntrinsicMethodHandle: stringly typed
localStateDesc seems like a wart to me. Since this is all package
private, couldn't IntrinsicMethodHandle contain a @Stable Class[]
localStateTypes, then convert directly from List init to
the class array in MethodHandleImpl::makeLoop etc:


Good point, Claes. But BasicType[] looks like a better fit.
Unfortunately, it can't be used as a key into the cache, so some 
flattening is needed.


Best regards,
Vladimir Ivanov



Class[] localVarTypes = new Class[init.size()];
for (int i = 0; i < localVarTypes.length; i++) {
Class initRtype = init.get(i).type().returnType();
localVarTypes[i] =
BasicType.basicType(initRtype).basicTypeClass();
}
LambdaForm form = makeLoopForm(type.basicType(), localVarTypes);

The consuming code in InvokerBytecodeGenerator could then be simplified
a bit on top of that: use arrays instead of List should be safe since
it's private data that never escapes, the array from
IntrinsicMethodHandle can be used directly when available etc.

/Claes

On 2016-06-20 08:49, Michael Haupt wrote:

... gentle reminder ... :-)


Am 16.06.2016 um 15:17 schrieb Michael Haupt :

Dear all,

please review this change.
RFE: https://bugs.openjdk.java.net/browse/JDK-8143211
Webrev: http://cr.openjdk.java.net/~mhaupt/8143211/webrev.00/

The change puts the tryFinally and loop method handle combinator
implementations, which were introduced as part of the JEP 274 effort,
on a LambdaForm basis, which executes in bytecode generating
(default) and LambdaForm interpretation
(-Djava.lang.invoke.MethodHandle.COMPILE_THRESHOLD=-1) modes. It also
changes the output formatting of LambdaForms, introducing a
(hopefully) more readable format.

Thanks,

Michael

Re: RFR(L): 8143211: provide bytecode intrinsics for loop and try/finally executors

[Vladimir Ivanov]

Nice work, Michael!

Some comments follow:

===

src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java:

+private final List> loopClauseTypes;
+/** This list contains the actual local state types added by loop 
clauses, without {@code void} entries. */

+private final List> loopLocalStateTypes;
+/** The first slot after the arguments, to contain the first loop 
state element. */

+private final int firstLoopStateSlot;

You assume there's at most a single loop intrinsic in a compiled LF 
instance.


It's the case now, but it's not necessarily the case in the future. 
There's no such limitation for other intrinsics and I'd prefer to keep 
it that way.


Why not iterate over the LF to compute localsMap & localClasses? You can 
restore the information in emitLoop() from the intrinsic (localStateDesc).


===

src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java:

+ * The LambdaForm shape for the loop combinator is as follows 
(assuming one reference parameter passed in

...
+ *  tryFinally=Lambda(a0:L,a1:L)=>{
...
+ *t9:L=MethodHandleImpl.loop(t2:L,t3:L,t4:L,t5:L,t6:L);  // 
call the loop executor


Wrong LambdaForm name in the example (tryFinally vs loop).

===

1058 private static BoundMethodHandle 
makeBoundHandle(BoundMethodHandle.SpeciesData data, Object... args) {

1059 try {
1060 return (BoundMethodHandle) 
data.constructor().invokeWithArguments(args);

1061 } catch (Throwable ex) {
1062 throw uncaughtException(ex);
1063 }
1064 }

It can be used in makeGuardWithTest as well.

I like the helper method, but it can cause bootstrapping issues (due to 
MH.invokeWithArguments which depends on j.l.i machinery) if used 
unwisely. I think that's the main reason why invokeBasic was used in the 
first place.


===

+static final class IntrinsicMethodHandle extends 
DelegatingMethodHandle {

...
+private final String localStateDesc;

Maybe subclass IntrinsicMH instead?

LoopIntrinsic extends IntrinsicMethodHandle {
  private final String localStateDesc;

  LoopIntrinsic(MethodHandle target, String localStateDesc) {
super(target,Intrinsic.LOOP);
this.localStateDesc = localStateDesc;
  }
...
  String getLocalStateDesc() {
return localStateDesc;
  }
}

static MethodHandle makeLoopIntrinsic(MethodHandle target, String 
localStateDesc) {

  return new LoopIntrinsic(target, localStateDesc);
}


Also, you can consider getting rid of IMH and moving intrinsic 
information to LambdaForm.NamedFunction (akin to 
LambdaForm.Name.constraint). I was responsible for adding IMH, but 
LambdaForm.NameFunction looks like a better place now.


===

src/java.base/share/classes/java/lang/invoke/MethodTypeForm.java:

+Map> loopLambdaForms;

What about purging of stale entries from the cache?

Have you considered reusing LambdaFormEditor? E.g., put a root LF (w/ 
empty local state?) in MTF slot and fork new LFs (w/ non-empty local 
state) from it?


Best regards,
Vladimir Ivanov

On 6/16/16 4:17 PM, Michael Haupt wrote:

Dear all,

please review this change.
RFE: https://bugs.openjdk.java.net/browse/JDK-8143211
Webrev: http://cr.openjdk.java.net/~mhaupt/8143211/webrev.00/

The change puts the tryFinally and loop method handle combinator 
implementations, which were introduced as part of the JEP 274 effort, on a 
LambdaForm basis, which executes in bytecode generating (default) and 
LambdaForm interpretation 
(-Djava.lang.invoke.MethodHandle.COMPILE_THRESHOLD=-1) modes. It also changes 
the output formatting of LambdaForms, introducing a (hopefully) more readable 
format.

Thanks,

Michael

Re: [jdk9] RFR: 8159822: Non-synchronized access to shared members of com.sun.jndi.ldap.pool.Pool

[Ivan Gerasimov]

On 21.06.2016 18:43, Aleksey Shipilev wrote:

On 06/21/2016 06:14 PM, Ivan Gerasimov wrote:

Hello!

The Pool has a member `map`, which is accessed from different threads,
thus the access is synchronized.
However, in some code paths (mostly in debug printing) it is accessed
without proper synchronization, which results in intermediate
ConcurrentModificationException when the debug output is turned on.

Would you please help review the fix?

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8159822
WEBREV: http://cr.openjdk.java.net/~igerasim/8159822/00/webrev/

Missed synchronized-s:

  154 private Connections getConnections(Object id) {
  155 ConnectionsRef ref = map.get(id);
  156 return (ref != null) ? ref.getConnections() : null;
  157 }

...gets called via:

  168 public void expire(long threshold) {
  169 synchronized (map) {
   ...
  179 }
  180 expungeStaleConnections();
  181 }

...and also via:

  188 private static void expungeStaleConnections() {
...
  192 Connections conns = releaseRef.getConnections();
...
  203  }
  204 }
But this is ConnectionsWeakRef.getConnections() not 
Pool.getConnections(Object).
As far as I can see, an instance of ConnectionsWeakRef cannot be 
accessed concurrently.




...

  117 public PooledConnection getPooledConnection(Object id, long
timeout,
  118 PooledConnectionFactory factory) throws NamingException {
...
 // no synchronized prior here
  127 expungeStaleConnections();
...

expungeStaleConnections() should be safe to call concurrently as long as 
ReferenceQueue.poll() is thread-safe.


With kind regards,
Ivan

Re: RFR 8159245: Loggers created by system classes are not initialized correctly when configured programmatically from application code.

[Mandy Chung]

> On Jun 21, 2016, at 8:39 AM, Daniel Fuchs  wrote:
> 
> I don't understand this scenario.
> ConfigurationData should remain as simple as possible.
> Logger.getLogger() / LogManager.demandSystemLogger() will never return
> a logger before it has been configured.
> When we're merging the configuration data the system logger has
> not been configured yet. Level etc are already volatile in Logger and
> we should not introduce any new synchronization there.

This is the scenario I was thinking about - would this ever happen?

T1 is creating a system logger named “foo” and calling this.importConfg(other) 
at line 462 after setLevel(otherLevel) is done.

this is the system logger “foo” and other is the user-created logger “foo”.

T2 is calling other.setLevel(newLevel) on user-created logger “foo”.

Mandy

Re: [jdk9] RFR: 8159822: Non-synchronized access to shared members of com.sun.jndi.ldap.pool.Pool

[Aleksey Shipilev]

On 06/21/2016 06:14 PM, Ivan Gerasimov wrote:
> Hello!
> 
> The Pool has a member `map`, which is accessed from different threads,
> thus the access is synchronized.
> However, in some code paths (mostly in debug printing) it is accessed
> without proper synchronization, which results in intermediate
> ConcurrentModificationException when the debug output is turned on.
> 
> Would you please help review the fix?
> 
> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8159822
> WEBREV: http://cr.openjdk.java.net/~igerasim/8159822/00/webrev/

Missed synchronized-s:

 154 private Connections getConnections(Object id) {
 155 ConnectionsRef ref = map.get(id);
 156 return (ref != null) ? ref.getConnections() : null;
 157 }

...gets called via:

 168 public void expire(long threshold) {
 169 synchronized (map) {
  ...
 179 }
 180 expungeStaleConnections();
 181 }

...and also via:

 188 private static void expungeStaleConnections() {
...
 192 Connections conns = releaseRef.getConnections();
...
 203  }
 204 }

...

 117 public PooledConnection getPooledConnection(Object id, long
timeout,
 118 PooledConnectionFactory factory) throws NamingException {
...
// no synchronized prior here
 127 expungeStaleConnections();
...


Thanks,
-Aleksey

Re: RFR 8159245: Loggers created by system classes are not initialized correctly when configured programmatically from application code.

[Daniel Fuchs]

On 21/06/16 16:12, Mandy Chung wrote:



On Jun 21, 2016, at 4:54 AM, Daniel Fuchs  wrote:

Hi Hamlin,

I was mistaken in my first assessment.

The case where the system handler's list is not empty
should only happen if by misfortune two different threads
happen to attempt to merge the same two configurations
concurrently. Though of no consequence for level, filter,
etc... (single values) that would be an issue for the
handlers list if not handled correctly.

With that in mind I have slightly revised the fix - and
added a more verbose comment explaining the reason for the
isEmpty() check.

http://cr.openjdk.java.net/~dfuchs/webrev_8159245/webrev.01/index.html


A related one, what if setLevel is called on a logger while its config is not 
set to the system logger’s config but system logger’s config is already set 
with the level previously set?

Would it be simpler if ConfigurationData defines the getter/setter methods and 
synchronized if it’s being merged?


I don't understand this scenario.
ConfigurationData should remain as simple as possible.
Logger.getLogger() / LogManager.demandSystemLogger() will never return
a logger before it has been configured.
When we're merging the configuration data the system logger has
not been configured yet. Level etc are already volatile in Logger and
we should not introduce any new synchronization there.

best regards,

-- daniel



Mandy

Re: RFR(L): 8143211: provide bytecode intrinsics for loop and try/finally executors

[Claes Redestad]

Hi Michael,

this looks quite good to me.

Forgettable ramblings:

MethodTypeForm: could it be beneficial to have a cleaner to remove 
mappings from loopLambdaForms various LambdaForms are GCd?


InvokerBytecodeGenerator/IntrinsicMethodHandle: stringly typed 
localStateDesc seems like a wart to me. Since this is all package 
private, couldn't IntrinsicMethodHandle contain a @Stable Class[] 
localStateTypes, then convert directly from List init to 
the class array in MethodHandleImpl::makeLoop etc:


Class[] localVarTypes = new Class[init.size()];
for (int i = 0; i < localVarTypes.length; i++) {
Class initRtype = init.get(i).type().returnType();
localVarTypes[i] = 
BasicType.basicType(initRtype).basicTypeClass();

}
LambdaForm form = makeLoopForm(type.basicType(), localVarTypes);

The consuming code in InvokerBytecodeGenerator could then be simplified 
a bit on top of that: use arrays instead of List should be safe since 
it's private data that never escapes, the array from 
IntrinsicMethodHandle can be used directly when available etc.


/Claes

On 2016-06-20 08:49, Michael Haupt wrote:

... gentle reminder ... :-)


Am 16.06.2016 um 15:17 schrieb Michael Haupt :

Dear all,

please review this change.
RFE: https://bugs.openjdk.java.net/browse/JDK-8143211
Webrev: http://cr.openjdk.java.net/~mhaupt/8143211/webrev.00/

The change puts the tryFinally and loop method handle combinator 
implementations, which were introduced as part of the JEP 274 effort, on a 
LambdaForm basis, which executes in bytecode generating (default) and 
LambdaForm interpretation 
(-Djava.lang.invoke.MethodHandle.COMPILE_THRESHOLD=-1) modes. It also changes 
the output formatting of LambdaForms, introducing a (hopefully) more readable 
format.

Thanks,

Michael

[jdk9] RFR: 8159822: Non-synchronized access to shared members of com.sun.jndi.ldap.pool.Pool

[Ivan Gerasimov]

Hello!

The Pool has a member `map`, which is accessed from different threads, 
thus the access is synchronized.
However, in some code paths (mostly in debug printing) it is accessed 
without proper synchronization, which results in intermediate 
ConcurrentModificationException when the debug output is turned on.


Would you please help review the fix?

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8159822
WEBREV: http://cr.openjdk.java.net/~igerasim/8159822/00/webrev/

With kind regards,
Ivan

Re: RFR 8159245: Loggers created by system classes are not initialized correctly when configured programmatically from application code.

[Mandy Chung]

> On Jun 21, 2016, at 4:54 AM, Daniel Fuchs  wrote:
> 
> Hi Hamlin,
> 
> I was mistaken in my first assessment.
> 
> The case where the system handler's list is not empty
> should only happen if by misfortune two different threads
> happen to attempt to merge the same two configurations
> concurrently. Though of no consequence for level, filter,
> etc... (single values) that would be an issue for the
> handlers list if not handled correctly.
> 
> With that in mind I have slightly revised the fix - and
> added a more verbose comment explaining the reason for the
> isEmpty() check.
> 
> http://cr.openjdk.java.net/~dfuchs/webrev_8159245/webrev.01/index.html

A related one, what if setLevel is called on a logger while its config is not 
set to the system logger’s config but system logger’s config is already set 
with the level previously set?

Would it be simpler if ConfigurationData defines the getter/setter methods and 
synchronized if it’s being merged?

Mandy 

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

[Roger Riggs]

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: Get user.dir as a symlink not resolved

[Roger Riggs]

Hi Max,

The user.dir system property returns the value from the Unix 'getcwd' 
function which is defined

as the absolute pathname.

An absolute pathname would be preferred over a symbolic link which would 
need to be constantly
re-evaluated because it might be changed externally.  The absolute 
pathname is more stable.


I don't know of a simple reliable workaround that would work cross platform.

On some systems you might be able to query the environment for the 'PWD' 
value.

See System.getEnv("PWD"). (haven't tried it).

Roger



On 6/21/2016 4:50 AM, Wang Weijun wrote:

I'm on a Mac inside /tmp, which is a symlink to /private/tmp.

System.getProperty("user.dir") shows me "/private/tmp". Is there a way to get "/tmp" which is 
exactly what `pwd` return? I only like "/private/tmp" if I called "cd /private/tmp".

Thanks
Max

Re: RFR 8159245: Loggers created by system classes are not initialized correctly when configured programmatically from application code.

[Daniel Fuchs]

Hi Hamlin,

I was mistaken in my first assessment.

The case where the system handler's list is not empty
should only happen if by misfortune two different threads
happen to attempt to merge the same two configurations
concurrently. Though of no consequence for level, filter,
etc... (single values) that would be an issue for the
handlers list if not handled correctly.

With that in mind I have slightly revised the fix - and
added a more verbose comment explaining the reason for the
isEmpty() check.

http://cr.openjdk.java.net/~dfuchs/webrev_8159245/webrev.01/index.html

best regards,

-- daniel


On 21/06/16 07:57, Daniel Fuchs wrote:

Hi Hamlin,

There's no difference in behaviour - mergeConfigWithSystemPeer is called
unconditionally but the handlers from the application logger will be
copied in the system logger config only if the system's logger
list is empty - just like before.

This is something that will probably need to be revisited - maybe
the system handlers should be closed first and the application
handlers added unconditionally.
The issue here is that if the system logger had handlers
configured from the logging.properties file then the user handlers
are going to be garbage collected before being closed - which is
not ideal - even if it should rarely happen.

Let me think on it.

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

[Bhanu Gopularam]

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

Get user.dir as a symlink not resolved

[Wang Weijun]

I'm on a Mac inside /tmp, which is a symlink to /private/tmp.

System.getProperty("user.dir") shows me "/private/tmp". Is there a way to get 
"/tmp" which is exactly what `pwd` return? I only like "/private/tmp" if I 
called "cd /private/tmp".

Thanks
Max