Deadlock between FileHandler and ConsoleHandler when using customized formatter

[Shi Jun Zhang]

Hi,

We get a deadlock between java.util.logging.FileHandler and 
java.util.logging.ConsoleHandler when we are using a customized 
formatter in these handlers.


Here is the simple test case. 
http://cr.openjdk.java.net/~zhangshj/deadlockInHandlers/deadlockInHandlers.zip


Run "java -Djava.util.logging.config.file=logging.properties 
TestThread", it will hang immediately.


And this is the stack trace in dump file
"Thread-1":
at java.util.logging.StreamHandler.publish(StreamHandler.java:205)
- waiting to lock <0xec52e0c0> (a 
java.util.logging.ConsoleHandler)

at java.util.logging.ConsoleHandler.publish(ConsoleHandler.java:118)
at java.util.logging.Logger.log(Logger.java:617)
at java.util.logging.Logger.doLog(Logger.java:638)
at java.util.logging.Logger.log(Logger.java:661)
at java.util.logging.Logger.info(Logger.java:1289)
at CustomerFormatter.format(CustomerFormatter.java:16)
at java.util.logging.StreamHandler.publish(StreamHandler.java:210)
- locked <0xec51ab90> (a java.util.logging.FileHandler)
at java.util.logging.FileHandler.publish(FileHandler.java:614)
- locked <0xec51ab90> (a java.util.logging.FileHandler)
at java.util.logging.Logger.log(Logger.java:617)
at java.util.logging.Logger.doLog(Logger.java:638)
at java.util.logging.Logger.log(Logger.java:661)
at java.util.logging.Logger.info(Logger.java:1289)
at TestThread$1.run(TestThread.java:14)
at java.lang.Thread.run(Thread.java:724)
"Thread-2":
at java.util.logging.FileHandler.publish(FileHandler.java:611)
- waiting to lock <0xec51ab90> (a 
java.util.logging.FileHandler)

at java.util.logging.Logger.log(Logger.java:617)
at java.util.logging.Logger.doLog(Logger.java:638)
at java.util.logging.Logger.log(Logger.java:661)
at java.util.logging.Logger.info(Logger.java:1289)
at CustomerFormatter.format(CustomerFormatter.java:16)
at java.util.logging.StreamHandler.publish(StreamHandler.java:210)
- locked <0xec52e0c0> (a java.util.logging.ConsoleHandler)
at java.util.logging.ConsoleHandler.publish(ConsoleHandler.java:118)
at java.util.logging.Logger.log(Logger.java:617)
at java.util.logging.Logger.doLog(Logger.java:638)
at java.util.logging.Logger.log(Logger.java:661)
at java.util.logging.Logger.info(Logger.java:1289)
at TestThread$1.run(TestThread.java:14)
at java.lang.Thread.run(Thread.java:724)

The problem is that we use a logger in CustomerFormatter and this causes 
Logger.log call Logger.log itself. As FileHandler.publish and 
StreamHandler.publish is synchronized, but the iteration to call publish 
method for all handlers in Logger.log is not synchronized, the deadlock 
happens.


This violates the Java doc for java.util.logging.Logger that says "All 
methods on Logger are multi-thread safe."


Currently we have 2 workarounds.
First is to add 2 lines in the logging.properties to force 
CustomerFormatter use new instance of FileHandler and ConsoleHandler 
like this,
CustomerFormatter.handlers = java.util.logging.FileHandler, 
java.util.logging.ConsoleHandler

CustomerFormatter.useParentHandlers = false

Second is to synchronize the logger.info call in TestThread test case.

I'm not sure whether we should fix this problem in JDK. The fix I can 
image is to synchronize each call to handler.publish() in Logger.log, 
but I think this would cause performance degradation.


Another solution is to add some documents in Java doc to state that 
Logger is not "so much" multi-thread safe.


Any suggestion or better fix?

--
Regards,

Shi Jun Zhang

hg: jdk8/tl/jdk: 8021418: Intermittent: SSLSocketSSLEngineTemplate.java test fails with timeout

[jason . uh]

Changeset: 5ac7cd164300
Author:juh
Date:  2013-11-27 15:25 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/5ac7cd164300

8021418: Intermittent: SSLSocketSSLEngineTemplate.java test fails with timeout
Reviewed-by: xuelei, wetmore
Contributed-by: rajan.hal...@oracle.com

! test/sun/security/ssl/templates/SSLSocketSSLEngineTemplate.java

Re: 8029281: Synchronization issues in Logger and LogManager

[Peter Levart]

Hi Daniel,

I have started looking at the LogManager change first, and here's the 
1st race I found...


 The new method LoggerWeakRef.dispose() can be called from 3 places:

- LoggerContext.addLocalLogger
- LoggerContext.findLogger
- LogManager.drainLoggerRefQueueBounded which is called from public 
method LogManager.addLogger


The 1st and 2nd are guarded by LoggerContext.this lock, but the 3rd is 
unguarded.


What can happen is the following:

Thread1:
LogManager.addLogger(someLogger)
LogManager.drainLoggerRefQueueBounded() // finds some enqueued 
LoggerWeakRef for name == 'X.Y' and...

LoggerWeakRef.dispose() // this is 1st call to dispose, so...
synchronized (LoggerWeakRef.this) {
   disposed = true;
}

---context switch---

Thread2:
LogManager.addLogger(logger{name=='X.Y'})
LogManager.drainLoggerRefQueueBounded()  // no enqueued refs...
LoggerContext.addLocalLogger(logger{name=='X.Y'}, true)
LoggerWeakRef ref = namedLoggers.get(name); // returns 
a non-null ref ('X.Y' still points to a cleared weak ref)

if (ref.get() == null) {
ref.dispose(); // this is a 2nd call to this ref, 
so it returns quickly

}
// ... namedLoggers gets new entry, etc...
LogNode node = getNode(name); // get node for 'X.Y'
*node.loggerRef = ref;*

---context switch---

Thread1: (still in LoggerWeakRef.dispose())
if (node != null) {
*node.loggerRef = null;*  // clear LogNode's weak ref to 
us *!!! NOT QUITE !!!*



... so Thread1 erroneously clears the reference from Node to new 
LoggerWeakRef that was just added by Thread2 ...



One way to fix this is to synchronize the clearing of node.logerRef on 
node.context too. I would make LoggerWeakRef.node and parentRef fields 
volatile and do the following:



final class LoggerWeakRef extends WeakReference {
private String name; // for 
namedLoggers cleanup
private volatile LogNode   node; // for loggerRef 
cleanup
private volatile WeakReference parentRef;  // for kids 
cleanup


LoggerWeakRef(Logger logger) {
super(logger, loggerRefQueue);
name = logger.getName();  // save for namedLoggers cleanup
}

// dispose of this LoggerWeakRef object
void dispose() {
LogNode node = this.node;
if (node != null) {
synchronized (node.context) {
node = this.node; // double-checked locking
if (node != null) {
// if we have a LogNode, then we were a named 
Logger

// so clear namedLoggers weak ref to us
node.context.removeLoggerRef(name, this);
name = null;  // clear our ref to the Logger's name

node.loggerRef = null;  // clear LogNode's weak 
ref to us

this.node = null;   // clear our ref to LogNode
}
}
}

WeakReference parentRef = this.parentRef;
if (parentRef != null) {
// this LoggerWeakRef has or had a parent Logger
this.parentRef = null;  // clear our weak ref to the 
parent Logger (racy, but harmless)

Logger parent = parentRef.get();
if (parent != null) {
// the parent Logger is still there so clear the
// parent Logger's weak ref to us
parent.removeChildLogger(this);
}
}
}

...


What do you think?

That's all for today. Will check the rest of patch tomorrow.


Regards, Peter



On 11/27/2013 08:23 PM, Daniel Fuchs wrote:

Hi,

Please find below a webrev for:

8029281: Synchronization issues in Logger and LogManager


Now some comments on the fix:

LogManager:

When a logger was garbage collected, and then a new logger
of the same name requested, addlocalLogger used to call
LoggerContext.removeLogger to remove the stale reference
and replace it by a new one. But sometime, the stale reference
was still in the reference queue, which caused the *new* logger
to be wrongly removed later when the reference queue was drained.

LogManager was also missing some synchronization for its 'props'
variable.

Logger:

userBundle & resourceBundleName were sometimes accessed within
a synchronized block, and sometimes without. In particular the
getters were not synchronized, which could cause race conditions
because an other thread could see incons

Re: JDK 8 RFR 4891331: BigInteger a.multiply(a) should use squaring code

[Joe Darcy]

Hi Brian,

Looks good; approved to go back.

Thanks,

-Joe

On 11/27/2013 11:39 AM, Brian Burkhalter wrote:

Continuing from the end of this thread 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-October/022399.html 
...

Issue:  https://bugs.openjdk.java.net/browse/JDK-4891331
Webrev: http://cr.openjdk.java.net/~bpb/4891331/webrev.2

After running more micro-benchmark performance tests using a threshold on the 
int-length of BigInteger above which squaring code is used for multiply(this), 
the value 20 appears more apropos. This is the value in the updated webrev. 
This threshold appears to prevent regressions across the array of machines 
tested (Linux, Mac, Windows, AMD, Intel). In some cases of course, the 
threshold prevents performance improvement which would otherwise occur. This is 
the cost of avoiding regressions.

It should be noted that a small regression, on the order of 1%-3% is often 
observed for the case of unity int-length, i.e., when the BigInteger is 
equivalent to an int. This is apparently the cost of the added conditional val 
== this. It does not seem to me however to be of sufficient import to argue 
against going forward with the patch.

Thanks,

Brian

JDK 8 RFR 4891331: BigInteger a.multiply(a) should use squaring code

[Brian Burkhalter]

Continuing from the end of this thread 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-October/022399.html 
...

Issue:  https://bugs.openjdk.java.net/browse/JDK-4891331
Webrev: http://cr.openjdk.java.net/~bpb/4891331/webrev.2

After running more micro-benchmark performance tests using a threshold on the 
int-length of BigInteger above which squaring code is used for multiply(this), 
the value 20 appears more apropos. This is the value in the updated webrev. 
This threshold appears to prevent regressions across the array of machines 
tested (Linux, Mac, Windows, AMD, Intel). In some cases of course, the 
threshold prevents performance improvement which would otherwise occur. This is 
the cost of avoiding regressions.

It should be noted that a small regression, on the order of 1%-3% is often 
observed for the case of unity int-length, i.e., when the BigInteger is 
equivalent to an int. This is apparently the cost of the added conditional val 
== this. It does not seem to me however to be of sufficient import to argue 
against going forward with the patch.

Thanks,

Brian

8029281: Synchronization issues in Logger and LogManager

[Daniel Fuchs]

Hi,

Please find below a webrev for:

8029281: Synchronization issues in Logger and LogManager


Now some comments on the fix:

LogManager:

When a logger was garbage collected, and then a new logger
of the same name requested, addlocalLogger used to call
LoggerContext.removeLogger to remove the stale reference
and replace it by a new one. But sometime, the stale reference
was still in the reference queue, which caused the *new* logger
to be wrongly removed later when the reference queue was drained.

LogManager was also missing some synchronization for its 'props'
variable.

Logger:

userBundle & resourceBundleName were sometimes accessed within
a synchronized block, and sometimes without. In particular the
getters were not synchronized, which could cause race conditions
because an other thread could see inconsistent data.

I also had to refactor the two methods getEffectiveResourceBundle()
and getResourceBundleName() into a single method.
The code there might be a bit more difficult to follow,
because I made sure it preserved the former behavior wrt to
what will be set in the LogRecord in doLog().

Tests:

The new TestLoggerBundleSync.java has been great to test
the issues above in both Logger & LogManager (it detected
the drainLoggerRefQueueBounded issue).

Since I had to touch at drainLoggerRefQueueBounded to make
TestLoggerBundleSync.java pass I also threw in a
TestLogConfigurationDeadLock.java to make sure I hadn't introduced
any new deadlock (and it detected the lack of synchronization
in LogManager.props).

best regards,

-- daniel

hg: jdk8/tl/jdk: 8028771: regression test java/util/Locale/LocaleProviders.sh failed

[naoto . sato]

Changeset: 2370d285d08b
Author:naoto
Date:  2013-11-27 10:01 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/2370d285d08b

8028771: regression test java/util/Locale/LocaleProviders.sh failed
Reviewed-by: alanb

! test/java/util/Locale/LocaleProviders.java
! test/java/util/Locale/LocaleProviders.sh

Re: RFR (JAXP) 8028822 : Error in the documentation for newFactory method of the javax.xml.stream factories

[huizhe wang]

On 11/27/2013 2:03 AM, Alan Bateman wrote:

On 26/11/2013 22:13, huizhe wang wrote:


Ah, I missed that. I took it for granted that since 
ServiceLoader.load(service) uses TCCL, load(service, null) would use 
TCCL. I've updated the webrev now:


http://cr.openjdk.java.net/~joehw/jdk8/jaxp16MR/8028822/webrev/

It looks good now but I'm still wondering about tests. Are you 
planning to push any tests with this?


I'm not adding new tests. I labeled it noreg-jck and Paul has identified 
the specific test cases.


-Joe



-Alan

Re: Review Request for 8029216: (jdeps) Provide a specific option to report JDK internal APIs

[Mandy Chung]

On 11/27/2013 7:52 AM, Dalibor Topic wrote:

On 11/27/13 1:04 PM, Alan Bateman wrote:

On 26/11/2013 23:59, Mandy Chung wrote:

This is a simple patch that adds a new jdeps -jdkinternals option to make it 
easier for developers to find dependencies on the JDK internal APIs:
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8029216/webrev.00/

This looks good (and I can't think of a better name for the option). Do you 
think this needs a test as otherwise this option will not be exercised?


I will add a test for this option.


-jdkinternals doesn't necessarily imply that using the JDK internal APIs is 
dangerous. Maybe calling it -warn would be better, and offer a path forward to 
in the future warn about other objectionable dependency constructs (circles, 
etc.).


By default it already highlights any dependency on JDK internal APIs.  
This option is to make it easier for developers to filter them and I 
think -jdkinternals is more obvious.  I'll keep the -warn idea to 
explore in the future.


thanks
Mandy

Re: RFR (S + L test) : 8016839 : JSR292: AME instead of IAE when calling a method

[Peter Levart]

On 11/27/2013 02:40 PM, David Chase wrote:

On 2013-11-27, at 6:53 AM, Peter Levart  wrote:

Ah, I have misunderstood the back-porting issue. It was not about not having 
new class but about which existing class to use as a host that might not exist 
in older version of platform...

Sorry for noise.

Noise is okay.  This fix was a PITA, init order can hide surprises, and I don't 
mind going over the details.

As to the activation of the code in question, practically never, except in tests or when 
bozo-bytecode-generators (a remark I resemble) are making mistakes.  The 
"initialize" of the throwIAE method is deferred until loading of a class that 
performs an illegal-access-override of an interface method -- e.g., interface I { int 
m(); } class C implements I { private int m(); } so even that requires an inconsistent 
set of classes, which is
unlikely in any normal initialization.

Longer-term/hoped-for plan is to create a custom throwIAE method each time this happens 
so that an informative error message can be included -- "C.m() overrides I.m() but 
is private" (or protected, or package-inaccessible) -- but that is tricky because we 
don't have a good place to put the method; by the time we learn this about class C, we 
have rewritten its constant pool and it is difficult to add more, and the constant pool 
cache is fragile-yet-critical-to-interpreter-performance.  So for now, I did this.

I did test it as much as I could figure out how, including running the 
regression test in Netbeans and poking around, and also running it under jdb on 
some embedded platforms and trying to think of commands that might trigger 
embarrassing failures.

The push is in the pipeline, but if you have other tests you can suggest, it is 
not too late to pull the emergency stop (in particular, I am gatekeeper for 
hs-comp this month, so I can put off the next turn of the crank till the last 
moment).

David



Your and David Holmes' answers have pretty much silenced me. I can't 
think of any more troubles...


About creating an informative error message: is there a way to 
dynamically dig-out from the VM, while executing the single system-wide 
throwIAE method, the symbolic invocation attributes of a call-site 
together with the target object in case of instance method invocation? 
From that data it could be possible to re-compute the context in 
question, I assume...


Regards, Peter

Re: Review Request for 8029216: (jdeps) Provide a specific option to report JDK internal APIs

[Dalibor Topic]

On 11/27/13 1:04 PM, Alan Bateman wrote:
> On 26/11/2013 23:59, Mandy Chung wrote:
>> This is a simple patch that adds a new jdeps -jdkinternals option to make it 
>> easier for developers to find dependencies on the JDK internal APIs:
>> http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8029216/webrev.00/
> This looks good (and I can't think of a better name for the option). Do you 
> think this needs a test as otherwise this option will not be exercised?

-jdkinternals doesn't necessarily imply that using the JDK internal APIs is 
dangerous. Maybe calling it -warn would be better, and offer a path forward to 
in the future warn about other objectionable dependency constructs (circles, 
etc.).

cheers,
dalibor topic

-- 
Oracle 
Dalibor Topic | Principal Product Manager
Phone: +494089091214  | Mobile: +491737185961 

Oracle Java Platform Group

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

ORACLE Deutschland B.V. & Co. KG
Hauptverwaltung: Riesstr. 25, D-80992 München
Registergericht: Amtsgericht München, HRA 95603
Geschäftsführer: Jürgen Kunz

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

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

Re: Poor Javadoc of Map default methods [Re: RFR: 8029055: Map.merge must refuse null values]

[Stephen Colebourne]

On 26 November 2013 17:35, Martin Buchholz  wrote:
> I haven't looked in depth, but I agree with Stephen's analysis.  This API
> and its javadoc needs work.
> E.g. It's not clear that the purpose of Map.compute is to *update* the
> mapping for key in the map.

I actually felt that the names of all four methods felt wrong. compute
and merge seem like unfortunate choices.

> Instead of "The default implementation makes no guarantees about
> synchronization or atomicity properties of this method."  we should boldly
> say that the default implementation is *not* atomic, even if the underlying
> map is.

Saying that the default implementation is not atomic sounds uncontroversial.

Stephen

Re: RFR (S + L test) : 8016839 : JSR292: AME instead of IAE when calling a method

[David Chase]

On 2013-11-27, at 6:53 AM, Peter Levart  wrote:
> Ah, I have misunderstood the back-porting issue. It was not about not having 
> new class but about which existing class to use as a host that might not 
> exist in older version of platform...
> 
> Sorry for noise.

Noise is okay.  This fix was a PITA, init order can hide surprises, and I don't 
mind going over the details.

As to the activation of the code in question, practically never, except in 
tests or when bozo-bytecode-generators (a remark I resemble) are making 
mistakes.  The "initialize" of the throwIAE method is deferred until loading of 
a class that performs an illegal-access-override of an interface method -- 
e.g., interface I { int m(); } class C implements I { private int m(); } so 
even that requires an inconsistent set of classes, which is
unlikely in any normal initialization.

Longer-term/hoped-for plan is to create a custom throwIAE method each time this 
happens so that an informative error message can be included -- "C.m() 
overrides I.m() but is private" (or protected, or package-inaccessible) -- but 
that is tricky because we don't have a good place to put the method; by the 
time we learn this about class C, we have rewritten its constant pool and it is 
difficult to add more, and the constant pool cache is 
fragile-yet-critical-to-interpreter-performance.  So for now, I did this.

I did test it as much as I could figure out how, including running the 
regression test in Netbeans and poking around, and also running it under jdb on 
some embedded platforms and trying to think of commands that might trigger 
embarrassing failures.

The push is in the pipeline, but if you have other tests you can suggest, it is 
not too late to pull the emergency stop (in particular, I am gatekeeper for 
hs-comp this month, so I can put off the next turn of the crank till the last 
moment).

David

Re: Request to review JDK-8028094

[Balchandra Vaidya]

Hi Martin,

I have incorporated your contribution into following
webrev.  Please review the changes.
http://cr.openjdk.java.net/~bvaidya/8/8028094/webrev.01/

I have tested it on a linux machine, and the test is passing.
The sleep processes are killed when the pkill command exists
on the machine. The test is passing without killing sleep when the
pkill command is not present.


Thanks
Balchandra


On 11/26/13 06:15 PM, Martin Buchholz wrote:
I dredged up some more memories about this part of ProcessBuilder and 
this part of Basic.java.


As the comment indicates, there are remaining issues (might even call 
it a "bug") where the grandchild can keep a file descriptor open and 
thus cause a hang in the java process.  Which is very hard to fix; I 
advise you not to try, unless perhaps your name is Alan Bateman.


But here's an improvement to Basic.java which should kill off the 
sleep if pkill is available, not fail if pkill is not available, and 
keep the wakeupJeff-protected code working as intended until some 
brave soul tries to tackle the lingering bug in ProcessBuilder.


https://www.youtube.com/watch?v=XP7q2o1Z0w8

--- a/test/java/lang/ProcessBuilder/Basic.java
+++ b/test/java/lang/ProcessBuilder/Basic.java
@@ -2017,7 +2017,6 @@
 && new File("/bin/bash").exists()
 && new File("/bin/sleep").exists()) {
 final String[] cmd = { "/bin/bash", "-c", 
"(/bin/sleep )" };
-final String[] cmdkill = { "/bin/bash", "-c", 
"(/usr/bin/pkill -f \"sleep \")" };

 final ProcessBuilder pb = new ProcessBuilder(cmd);
 final Process p = pb.start();
 final InputStream stdout = p.getInputStream();
@@ -2045,13 +2044,13 @@
 stdout.close();
 stderr.close();
 stdin.close();
-new ProcessBuilder(cmdkill).start();
 //--
 // There remain unsolved issues with asynchronous close.
 // Here's a highly non-portable experiment to 
demonstrate:

 //--
 if (Boolean.getBoolean("wakeupJeff!")) {
 System.out.println("wakeupJeff!");
+long startTime = System.nanoTime();
 // Initialize signal handler for INTERRUPT_SIGNAL.
 new 
FileInputStream("/bin/sleep").getChannel().close();
 // Send INTERRUPT_SIGNAL to every thread in this 
java.

@@ -2064,8 +2063,18 @@
 };
 new ProcessBuilder(wakeupJeff).start().waitFor();
 // If wakeupJeff worked, reader probably got EBADF.
-reader.join();
+long timeout = 60L * 1000L;
+reader.join(timeout);
+long elapsedTimeMillis =
+(System.nanoTime() - startTime) / (1024L * 
1024L);

+check(elapsedTimeMillis < timeout);
+check(!reader.isAlive());
 }
+// Try to clean up the sleep process, but don't fret 
about it.

+try {
+new ProcessBuilder("/usr/bin/pkill", "-f", "sleep 
")

+.start();
+} catch (IOException noPkillCommandInstalled) { }
 }
 } catch (Throwable t) { unexpected(t); }




On Thu, Nov 21, 2013 at 4:26 AM, Balchandra Vaidya 
mailto:balchandra.vai...@oracle.com>> 
wrote:



Hi Martin,



> +check(elapsedTimeMillis < timeout);
> + *check(!reader.isAlive());*


The line check(!reader.isAlive()) is causing the test failure
when the pkill command is not available.  Any idea ?  The
test is passing with check(reader.isAlive()).


The modified test is passing when the pkill command is
available.


When the pkill command  is not available, the test is/was
failing without try block around
new ProcessBuilder(cmdkill).start().

So, without placing the above line under try block was a
mistake.  I will make the correction.


Thanks
Balchandra



On 11/20/13 03:11 PM, Balchandra Vaidya wrote:


Thanks Martin.  I will incorporate your suggested improvements, and
will send out another webrev.

Regards
Balchandra

On 19/11/2013 22:53, Martin Buchholz wrote:

I see this is already submitted.

I thought I could do better than using pkill, but no - there is
no convenient communication from the java process to the
grandchild.  This is a hairy test!

Nevertheless, I would like you to incorporate the following
improvements:
- invoke pkill directly
- do some extra checking
- join with reader thread
- don't fail if pkill is not available

--- a/test/java/lang/ProcessBuilder/Basic.java
+++ b/test/java/lang/Proces

Re: Review Request for 8029216: (jdeps) Provide a specific option to report JDK internal APIs

[Alan Bateman]

On 26/11/2013 23:59, Mandy Chung wrote:
This is a simple patch that adds a new jdeps -jdkinternals option to 
make it easier for developers to find dependencies on the JDK internal 
APIs:

http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8029216/webrev.00/
This looks good (and I can't think of a better name for the option). Do 
you think this needs a test as otherwise this option will not be exercised?


-Alan

Re: RFR (S + L test) : 8016839 : JSR292: AME instead of IAE when calling a method

[Peter Levart]

On 11/27/2013 11:03 AM, David Holmes wrote:

Hi Peter,

On 27/11/2013 7:20 PM, Peter Levart wrote:

On 11/27/2013 08:40 AM, David Holmes wrote:

On 27/11/2013 2:16 AM, David Chase wrote:
On 2013-11-26, at 8:12 AM, David Chase  
wrote:

On 2013-11-26, at 7:32 AM, David Holmes 
wrote:

On 26/11/2013 10:16 PM, David Chase wrote:

On 2013-11-26, at 7:12 AM, David Holmes 
wrote:

On 26/11/2013 9:56 PM, David Chase wrote:

On 2013-11-25, at 9:18 PM, David Holmes
 wrote:

We do have the jdk.internal namespace. But I think Unsafe is as
good a place as any - though maybe sun.misc.VM is marginally
better?


Does anyone have any problems with sun.misc.VM as a choice?
I have to do a minor revision to the hotspot commit anyway.
Is sun.misc.VM also loaded very early anyway?


No you would have to add it as for Unsafe.


But it's loaded early anyway as a normal consequence of other
class loading, right?


What do you mean by "early"? It isn't a pre-loaded class but it
will be loaded during system initialization. It is approx the 120th
class to be loaded. Unsafe is about 135th.


120 is earlier than 135, so by that measure it is superior.
Do you see any other problems with the change?
The method's not at all "Unsafe" in the technical sense of the word,
so it is just a matter of choosing a good home.


On further investigation, change to sun.misc.VM would be the first
time that hotspot knows of the existence of sun.misc.VM;
sun.misc.Unsafe is already filled with methods that the runtime knows
about (intrinsics, etc).  I think Unsafe is better.


Okay.

David


Hi David(s),

Excuse me for my ignorance, but does pre-loading the class involve it's
initialization? Is static initializer called at that time?


No, pre-load is simply loading not initialization. Static 
initialization gets triggerred via a controlled process as it is very 
delicate.


Even if it is

not at that time, it surely should be called before first invocation of
a method on that class (the throwIllegalAccessError() method in this
case). You need a reference to this method very early - even before
system initialization starts. How early do you expect first "faulty
invocations" could occur that need to call this method? What if calling
that method triggers non-trivial initialization which in turn encounters
another faulty invocation?


These faults should only appear in application classes so generally 
everything should be initialized well before you need to use this 
method. If a core library class has a bug that triggered this need to 
call this method then yes it is possible that it might happen too 
early to succeed - but that is quite normal for the core classes, 
there are a number of things that can happen too early in the 
initialization process to work (eg throwing exceptions, using 
assertions).


That said I'm not sure how this could fail in that all we need is a 
reference to the method to put in the vtable and we have that after 
loading. Then all that can go wrong is that we can't actually throw 
the exception.



sun.misc.Unsafe has a non-trivial static initialization which involves
"registerNatives()" native method invocation, and it also calls:


registerNative is not an issue

sun.reflect.Reflection.registerMethodsToFilter(Unsafe.class, 
"getUnsafe");


...which initializes hell lot of things (like java.util.HashMap for
example, which in who knows which incarnation included random hash seed
initialization which triggered random number generator initialization
with gathering of random seed from various sources, ...)



sun.misc.VM on the other hand, only has the following static
initialization:

 private static final Object lock = new Object();

 static {
 initialize();
 }
 private native static void initialize();


Are you okay with all possible interactions of this initialization on
all platforms?


The only time the initialization order would be changed by this fix is 
if one of the classes initialized before-hand had a bug that required 
this fix to come into affect. That is obviously a JDK bug and is 
extremely unlikely to happen.


Note that sun.misc.VM is currently initialized 44th while Unsafe is 61st.

I don't see any issues with this fix in that regard.


I see. Thanks for explanation, David.




I think a new class with only this method would be a safer choice.
Regarding back-porting, even if sun.misc.Unsafe is used as the holder
for that method, this new method will have to be added to
sun.misc.Unsafe on those legacy platforms in order to obtain this new
Method *. If the method is not found, the VM would have to behave as
before. Couldn't the pre-loading of classes be made such that some of
them are optional?


It already supports optional classes.


Ah, I have misunderstood the back-porting issue. It was not about not 
having new class but about which existing class to use as a host that 
might not exist in older version of platform...


Sorry for noise.

Regards, Peter



David H.





Regard, 

Re: RFR 8029164: Race condition in CompletableFuture.thenCompose with asynchronous task

[Chris Hegarty]

Looks good to me Paul.

-Chris.

On 27/11/13 11:02, Paul Sandoz wrote:

Hi,

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

http://cr.openjdk.java.net/~psandoz/tl/JDK-8029164-thenCompose-async/webrev/

This fixes an issue with CompletableFuture.thenCompose.

If the composing future is already completed before the call to thenCompose (or 
completes during some part of the call to thenCompose) and the composing 
function returns a future that is not yet completed then there is race to set 
the result, usually won by thenCompose which incorrectly sets the result to 
null, due to an errant conditional statement.

Doug has reviewed and committed the fix to the 166 repo and we have double 
checked there are no similar errors lurking in other areas of CompletableFuture 
code.

Paul.

RFR 8029164: Race condition in CompletableFuture.thenCompose with asynchronous task

[Paul Sandoz]

Hi,

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

http://cr.openjdk.java.net/~psandoz/tl/JDK-8029164-thenCompose-async/webrev/

This fixes an issue with CompletableFuture.thenCompose.  

If the composing future is already completed before the call to thenCompose (or 
completes during some part of the call to thenCompose) and the composing 
function returns a future that is not yet completed then there is race to set 
the result, usually won by thenCompose which incorrectly sets the result to 
null, due to an errant conditional statement.

Doug has reviewed and committed the fix to the 166 repo and we have double 
checked there are no similar errors lurking in other areas of CompletableFuture 
code.

Paul.

Re: RFR (S + L test) : 8016839 : JSR292: AME instead of IAE when calling a method

[David Holmes]

Hi Peter,

On 27/11/2013 7:20 PM, Peter Levart wrote:

On 11/27/2013 08:40 AM, David Holmes wrote:

On 27/11/2013 2:16 AM, David Chase wrote:

On 2013-11-26, at 8:12 AM, David Chase  wrote:

On 2013-11-26, at 7:32 AM, David Holmes 
wrote:

On 26/11/2013 10:16 PM, David Chase wrote:

On 2013-11-26, at 7:12 AM, David Holmes 
wrote:

On 26/11/2013 9:56 PM, David Chase wrote:

On 2013-11-25, at 9:18 PM, David Holmes
 wrote:

We do have the jdk.internal namespace. But I think Unsafe is as
good a place as any - though maybe sun.misc.VM is marginally
better?


Does anyone have any problems with sun.misc.VM as a choice?
I have to do a minor revision to the hotspot commit anyway.
Is sun.misc.VM also loaded very early anyway?


No you would have to add it as for Unsafe.


But it's loaded early anyway as a normal consequence of other
class loading, right?


What do you mean by "early"? It isn't a pre-loaded class but it
will be loaded during system initialization. It is approx the 120th
class to be loaded. Unsafe is about 135th.


120 is earlier than 135, so by that measure it is superior.
Do you see any other problems with the change?
The method's not at all "Unsafe" in the technical sense of the word,
so it is just a matter of choosing a good home.


On further investigation, change to sun.misc.VM would be the first
time that hotspot knows of the existence of sun.misc.VM;
sun.misc.Unsafe is already filled with methods that the runtime knows
about (intrinsics, etc).  I think Unsafe is better.


Okay.

David


Hi David(s),

Excuse me for my ignorance, but does pre-loading the class involve it's
initialization? Is static initializer called at that time?


No, pre-load is simply loading not initialization. Static initialization 
gets triggerred via a controlled process as it is very delicate.


Even if it is

not at that time, it surely should be called before first invocation of
a method on that class (the throwIllegalAccessError() method in this
case). You need a reference to this method very early - even before
system initialization starts. How early do you expect first "faulty
invocations" could occur that need to call this method? What if calling
that method triggers non-trivial initialization which in turn encounters
another faulty invocation?


These faults should only appear in application classes so generally 
everything should be initialized well before you need to use this 
method. If a core library class has a bug that triggered this need to 
call this method then yes it is possible that it might happen too early 
to succeed - but that is quite normal for the core classes, there are a 
number of things that can happen too early in the initialization process 
to work (eg throwing exceptions, using assertions).


That said I'm not sure how this could fail in that all we need is a 
reference to the method to put in the vtable and we have that after 
loading. Then all that can go wrong is that we can't actually throw the 
exception.



sun.misc.Unsafe has a non-trivial static initialization which involves
"registerNatives()" native method invocation, and it also calls:


registerNative is not an issue


sun.reflect.Reflection.registerMethodsToFilter(Unsafe.class, "getUnsafe");

...which initializes hell lot of things (like java.util.HashMap for
example, which in who knows which incarnation included random hash seed
initialization which triggered random number generator initialization
with gathering of random seed from various sources, ...)



sun.misc.VM on the other hand, only has the following static
initialization:

 private static final Object lock = new Object();

 static {
 initialize();
 }
 private native static void initialize();


Are you okay with all possible interactions of this initialization on
all platforms?


The only time the initialization order would be changed by this fix is 
if one of the classes initialized before-hand had a bug that required 
this fix to come into affect. That is obviously a JDK bug and is 
extremely unlikely to happen.


Note that sun.misc.VM is currently initialized 44th while Unsafe is 61st.

I don't see any issues with this fix in that regard.


I think a new class with only this method would be a safer choice.
Regarding back-porting, even if sun.misc.Unsafe is used as the holder
for that method, this new method will have to be added to
sun.misc.Unsafe on those legacy platforms in order to obtain this new
Method *. If the method is not found, the VM would have to behave as
before. Couldn't the pre-loading of classes be made such that some of
them are optional?


It already supports optional classes.

David H.





Regard, Peter




David

Re: RFR (JAXP) 8028822 : Error in the documentation for newFactory method of the javax.xml.stream factories

[Alan Bateman]

On 26/11/2013 22:13, huizhe wang wrote:


Ah, I missed that. I took it for granted that since 
ServiceLoader.load(service) uses TCCL, load(service, null) would use 
TCCL. I've updated the webrev now:


http://cr.openjdk.java.net/~joehw/jdk8/jaxp16MR/8028822/webrev/

It looks good now but I'm still wondering about tests. Are you planning 
to push any tests with this?


-Alan

Re: RFR (S + L test) : 8016839 : JSR292: AME instead of IAE when calling a method

[Peter Levart]

On 11/27/2013 08:40 AM, David Holmes wrote:



On 27/11/2013 2:16 AM, David Chase wrote:


On 2013-11-26, at 8:12 AM, David Chase  wrote:
On 2013-11-26, at 7:32 AM, David Holmes  
wrote:

On 26/11/2013 10:16 PM, David Chase wrote:


On 2013-11-26, at 7:12 AM, David Holmes  
wrote:

On 26/11/2013 9:56 PM, David Chase wrote:


On 2013-11-25, at 9:18 PM, David Holmes 
 wrote:
We do have the jdk.internal namespace. But I think Unsafe is as 
good a place as any - though maybe sun.misc.VM is marginally 
better?


Does anyone have any problems with sun.misc.VM as a choice?
I have to do a minor revision to the hotspot commit anyway.
Is sun.misc.VM also loaded very early anyway?


No you would have to add it as for Unsafe.


But it's loaded early anyway as a normal consequence of other 
class loading, right?


What do you mean by "early"? It isn't a pre-loaded class but it 
will be loaded during system initialization. It is approx the 120th 
class to be loaded. Unsafe is about 135th.


120 is earlier than 135, so by that measure it is superior.
Do you see any other problems with the change?
The method's not at all "Unsafe" in the technical sense of the word, 
so it is just a matter of choosing a good home.


On further investigation, change to sun.misc.VM would be the first 
time that hotspot knows of the existence of sun.misc.VM; 
sun.misc.Unsafe is already filled with methods that the runtime knows 
about (intrinsics, etc).  I think Unsafe is better.


Okay.

David


Hi David(s),

Excuse me for my ignorance, but does pre-loading the class involve it's 
initialization? Is static initializer called at that time? Even if it is 
not at that time, it surely should be called before first invocation of 
a method on that class (the throwIllegalAccessError() method in this 
case). You need a reference to this method very early - even before 
system initialization starts. How early do you expect first "faulty 
invocations" could occur that need to call this method? What if calling 
that method triggers non-trivial initialization which in turn encounters 
another faulty invocation?


sun.misc.Unsafe has a non-trivial static initialization which involves 
"registerNatives()" native method invocation, and it also calls:


sun.reflect.Reflection.registerMethodsToFilter(Unsafe.class, "getUnsafe");

...which initializes hell lot of things (like java.util.HashMap for 
example, which in who knows which incarnation included random hash seed 
initialization which triggered random number generator initialization 
with gathering of random seed from various sources, ...)


sun.misc.VM on the other hand, only has the following static initialization:

private static final Object lock = new Object();

static {
initialize();
}
private native static void initialize();


Are you okay with all possible interactions of this initialization on 
all platforms?


I think a new class with only this method would be a safer choice. 
Regarding back-porting, even if sun.misc.Unsafe is used as the holder 
for that method, this new method will have to be added to 
sun.misc.Unsafe on those legacy platforms in order to obtain this new 
Method *. If the method is not found, the VM would have to behave as 
before. Couldn't the pre-loading of classes be made such that some of 
them are optional?



Regard, Peter




David

Re: Proposed fix for JDK-8028804 (Deflater.deflateBytes() may produce corrupted output on Deflater level/strategy change)

[Thomas Stüfe]

>
>
>   My personal view is that we should link to libz where possible (already
> so on Mac, but not the default on Linux or Solaris). Clearly we still have
> to allow for platforms where it doesn't exist (Windows only I think) and in
> that case we should periodically update it (as we did in both JDK 7 and JDK
> 8). It may be time to look at upgrading it again (for JDK 9 as this is not
> something to touch in the end-game of a release).
>
>
I can see arguments for both sides (linking statically vs using the system
zlib) and I'm leaning toward the former. Errors or incompatibilities in the
zlib could be really harmful, and I rather use the zlib I ran all my tests
with instead of the system zlib which may or may not be out of date or show
regressions.

So, if I were to recode this fix again to not change the zlib - which is
not so trivial - do you think there is a chance that this fix gets examined
and maybe pulled into the OpenJDK?

..Thomas