Re: RFR [JDK8]: 7169894: JAXP Plugability Layer: using service loader
Hi all,
Thanks for all the comments!
I've updated the patch for the following recommended changes:
1. ServiceConfigurationError instead of ConfigurationError
2. Use factory class, class.forName section removed
3. Use load method without specifying classloader
4. To be clear on how the process handles exceptions, I've updated
JavaDoc with the following:
* Uses the service-provider loading facilities, defined by the
{@link java.util.ServiceLoader} class, to attempt
* to locate and load an implementation of the service. In case
multiple providers exist, the first
* valid 3rd party provider should be returned; If there is no
valid 3rd party provider,
* the default implementation is returned if it is on the classpath
or installed as a module.
*
* If ServiceConfigurationError is thrown, it is interpreted as an
invalid provider is encountered.
* The process shall continue the search until all are exhausted.
The JavaDoc is similar but slightly different for SchemaFactory
and XPathFactory:
* pUses the service-provider loading facilities, defined by the
{@link java.util.ServiceLoader} class, to attempt
* to locate and load an implementation of the service.
* Each potential service provider is required to implement the
method:/p
* pre
*{@link #isSchemaLanguageSupported(String schemaLanguage)}
* /pre
* A service provider is deemed as valid if it supports the
specified schema language.
*
*p
* In case multiple providers exist, the first
* valid 3rd party provider should be returned; If there is no
valid 3rd party provider,
* the default implementation is returned if it is on the classpath
or installed as a module.
* /p
* If ServiceConfigurationError is thrown, it is interpreted as an
invalid provider is encountered.
* The process shall continue the search until all are exhausted.
We can continue discussing on a solution for the duplication of the
classes. For this patch, if you agree, I'd like to check it in as it
stands now and work out another patch for the duplicated classes once we
have a solution.
New webrev:
http://cr.openjdk.java.net/~joehw/jdk8/7169894/webrev/
Thanks,
Joe
On 6/22/2012 10:24 AM, Joe Wang wrote:
On 6/22/2012 2:39 AM, Alan Bateman wrote:
On 21/06/2012 22:53, Joe Wang wrote:
:
We're not considering this change for jdk7 or older, i.e. I haven't
thought about fixing 6975142. For jdk8, it seems we have to live
with the way it is in (2), a seemly incompatible behavior in
backward compatible mode, although, it is a very rare case (I
regret to have taught or sent standalone jar files to those
customers :) ).
No, this isn't for jdk7 or older.
I don't fully grok the issue in 6975142. That looks to me to be a
case where the service provider implementation is including its own
copy of the javax.xml.stream.* classes and this is the cause of the
ClassCastException.
Yes, it's GF packaged woodstox that included its own API packages.
Joe
-Alan.
Re: Patch review request - Test bug 7123972 test/java/lang/annotation/loaderLeak/Main.java fails intermittently
On 2012/6/21 20:16, David Holmes wrote: Hi Eric, On 21/06/2012 8:57 PM, Eric Wang wrote: Hi David, Thanks for your review, I have updated the code by following your suggestion. please see the attachment. I'm not sure whether there's a better way to guarantee object finalized by GC definitely within the given time. The proposed fix may work in most cases but may still throw InterruptException if execution is timeout (2 minutes of JTreg by default). There is no way to guarantee finalization in a specific timeframe, but if a simple test hasn't executed finalizers in 2 minutes then that in itself indicates a problem. Can you post a full webrev for this patch? I don't like seeing it out of context and am wondering exactly what we are trying to GC here. David Regards, Eric On 2012/6/21 14:32, David Holmes wrote: Hi Eric, On 21/06/2012 4:05 PM, Eric Wang wrote: I come from Java SQE team who are interested in regression test bug fix. Here is the first simple fix for bug 7123972 http://monaco.us.oracle.com/detail.jsf?cr=7123972, Can you please help to review and comment? Attachment is the patch Thanks! This bug is caused by wrong assumption that the GC is started immediately to recycle un-referenced objects after System.gc() called one or two times. The proposed solution is to make sure the un-referenced object is recycled by GC before checking if the reference is null. Your patch makes its own assumptions - specifically that finalization must eventually run. At a minimum you should add System.runFinalization() calls after the System.gc() inside the loop. Even that is no guarantee in a general sense, though it should work for hotspot. David Regards, Eric Hi Alan David, Thank you for your comments, sorry for reply this mail late as i was just back from the dragon boat holiday. I have updated the code again based on your suggestions: rename the flag variable, increase the sleep time and remove it from problem list. The attachment is the full webrev for this patch, Can you please review again? Thanks a lot! Regards, Eric
javax.sql.rowset.serial.SerialBlob doesn't support free and getBinaryStream
Hi All, First of all, if the jdbc problem has a better mailing list to post please tell me. I find that javax.sql.rowset.serial.SerialBlob is not fully implemented in OpenJDK 8. Methods public InputStream getBinaryStream(long pos,long length) throws SQLException public void free() throws SQLException only throw UnsupportedOperationException. I have made a patch[1] to implement these 2 methods. Could anyone take a look to review it. BTW: I think the spec for SerialBlob is not very clear like it doesn't mention if all method rather than free() need throw any exception after free() is invoked. However that behavior seems reasonable. [1] http://cr.openjdk.java.net/~littlee/OJDK-576/webrev Thanks a lot. -- Best Regards, Deven
Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)
Hi, Rob,
src/solaris/native/java/lang/UNIXProcess_md.c:
You never check the return code of kill(2). kill() may fail if you
have no permission to kill the process (EPERM), or if the process id
is invalid, maybe it has already been reaped. In both cases an
exception would be nice, or if you decide this cannot happen in the
context of Process.java, at least an assert().
The same applies to GetExitCodeProcess() on windows.
Kind Regards,
Thomas Stuefe
SAP Germany
On Sun, Jun 24, 2012 at 2:57 PM, Rob McKenna rob.mcke...@oracle.com wrote:
Hi folks,
5th, and hopefully final review has been posted to:
http://cr.openjdk.java.net/~robm/4244896/webrev.05/
http://cr.openjdk.java.net/%7Erobm/4244896/webrev.05/
Let me know if there are any comments or concerns, and thanks a lot for the
help so far.
-Rob
On 01/06/12 01:41, David Holmes wrote:
Hi Rob,
This looks good to me. I'm glad to see that destroyForcibly mandates that
Process instances from ProcessBuilder.start and Runtime.exec must do a
forcible destroy. That addresses my concern about documenting the actual
implementations.
Two minor comments:
Process.java:
236 * {@link ProcessBuilder#start} and {@link Runtime#exec} are of
type that
are of type - are of a type ...
ProcessKillTest.sh:
BIT_FLAG is now unused.
Thanks,
David
On 1/06/2012 1:05 AM, Rob McKenna wrote:
Latest version including work on the spec language:
http://cr.openjdk.java.net/~robm/4244896/webrev.04/
http://cr.openjdk.java.net/%7Erobm/4244896/webrev.03/
-Rob
On 10/05/12 19:56, Rob McKenna wrote:
Hi folks,
The latest version is at:
http://cr.openjdk.java.net/~robm/4244896/webrev.03/
http://cr.openjdk.java.net/%7Erobm/4244896/webrev.03/
Feedback greatly appreciated.
-Rob
On 19/04/12 12:05, Alan Bateman wrote:
On 19/04/2012 01:05, David Holmes wrote:
On 18/04/2012 11:44 PM, Jason Mehrens wrote:
Rob,
It looks like waitFor is calling Object.wait(long) without owning
this objects monitor. If I pass Long.MAX_VALUE to waitFor,
shouldn't waitFor return if the early if the process ends?
Also waitFor doesn't call wait() under the guard of a looping
predicate so it will suffer from lost signals and potentially
spurious wakeups. I also don't see anything calling notify[All] to
indicate the process has now terminated. It would appear that
wait(timeout) is being used as a sleep mechanism and that is wrong
on a number of levels.
I assume waitFor(timout) will require 3 distinct implementations, one
for Solaris/Linux/Mac, another for Windows, and a default
implementations for Process implementations that exist outside of the
JDK.
It's likely the Solaris/Linux/Mac implementation will involve two
threads, one to block in waitpid and the other to interrupt it via a
signal if the timeout elapses before the child terminates. The
Windows implementation should be trivial because it can be a timed
wait.
I assume the default implementation (which is what is being discussed
here) will need to loop calling exitValue until the timeout elapses
or the child terminates. Not very efficient but at least it won't be
used when when creating Processes via Runtime.exec or ProcessBuilder.
I think the question we need to consider is whether waitFor(timeout)
is really needed. If it's something that it pushed out for another
day then it brings up the question as to whether to include isAlive
now or not (as waitFor without timeout gives us an isAlive equivalent
too).
-Alan.
Re: RFR: 7161229 - PriorityBlockingQueue keeps hard reference to last removed element
Hi David,
I think the test
if (n 0) {
should not be in siftDown* but should be done at all callsites,
by example in heapify, you can hoist the test outside of the loop.
In dequeue, it's a matter of style but the 'else' is not needed anymore.
Otherwise the changes looks ok for me.
Rémi
On 06/25/2012 06:26 AM, David Holmes wrote:
webrev:
http://cr.openjdk.java.net/~dholmes/7161229/webrev/
When removing the last element from the PBQ the sift down logic
would store it back in as array[0]. The simple fix is for the
sift-down to be a no-op if the queue size is zero.
The fix has been contributed by Doug Lea. We are taking this
opportunity to synchronize PBQ with Doug's latest version at:
http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/main/java/util/concurrent/PriorityBlockingQueue.java?view=log
So in addition to the above functional fix there are a number of
miscellaneous code and comment cleanups. The only non-trivial, but
still very simple, change is to the drainTo method to make it more
robust if the add() on the destination collection throws an exception.
I am of course a Reviewer for this.
I have provided the update to the LastElement test (as this is
certainly related to the last element) but there are some reservations
about examining the PBS internals this way. Unfortunately there is no
good way to test for these kinds of retention issues. You either need
a whitebox test like this, or need to rely on non-guaranteed GC and
finalization behaviour.
Thanks,
David Holmes
Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)
On 25/06/2012 09:56, Thomas Stüfe wrote: Hi, Rob, src/solaris/native/java/lang/UNIXProcess_md.c: You never check the return code of kill(2). kill() may fail if you have no permission to kill the process (EPERM), or if the process id is invalid, maybe it has already been reaped. In both cases an exception would be nice, or if you decide this cannot happen in the context of Process.java, at least an assert(). One thing to add to this is that the destroy method has been there since jdk1.0 and is not defined or specified to throw any exceptions so we can't really change that now. From the history it doesn't look like the return from kill(2) has ever been checked. As it can only ne used to send a signal to processes created by the ProcessBuilder or Runtime.exec API (not arbitrary process) then it is unlikely to fail. Whether the new destroy method should specify an exception may require consideration and maybe a separate bug should be created to consider that. -Alan
Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)
On 24/06/2012 13:57, Rob McKenna wrote: Hi folks, 5th, and hopefully final review has been posted to: http://cr.openjdk.java.net/~robm/4244896/webrev.05/ http://cr.openjdk.java.net/%7Erobm/4244896/webrev.05/ Let me know if there are any comments or concerns, and thanks a lot for the help so far. -Rob Overall I think this has worked out well. I agree with David on the needless calls to System.nanoTime in Process.waitFor although it's not a major issue given that the implementation in the JDK overrides this method. The test additions, both in coverage and implementation, are much cleaned now, and better test coverage too. A lot of the discussion here has been on the default implementation of waitFor that Process provides but it's not covered by the tests. I didn't notice this before but it would be good to include a basic test for this, just so that code is exercise. The simplest may be to create a concrete implementation of Process that delegates to a Process instance returned by ProcessBuilder.start. Assuming you don't override the waitFor implementation then it means you will be able to exercise the default waitFor rather than the platform implementation. -Alan.
Re: RFR: 7161229 - PriorityBlockingQueue keeps hard reference to last removed element
Hi Remi,
On 25/06/2012 7:01 PM, Rémi Forax wrote:
Hi David,
I think the test
if (n 0) {
should not be in siftDown* but should be done at all callsites,
by example in heapify, you can hoist the test outside of the loop.
I originally had it at the call-site (it already exists in a number of
them) but Doug prefers it to be internalized. He wrote:
... a better strategy is to move the n 0 check to the siftDown code
itself, so (existing and potential) callers don't need the check.
... an explicit n 0 check acts to hoist array index checks in loop so
turns out to be faster in general.
In dequeue, it's a matter of style but the 'else' is not needed anymore.
Otherwise the changes looks ok for me.
Thanks,
David
Rémi
On 06/25/2012 06:26 AM, David Holmes wrote:
webrev:
http://cr.openjdk.java.net/~dholmes/7161229/webrev/
When removing the last element from the PBQ the sift down logic
would store it back in as array[0]. The simple fix is for the
sift-down to be a no-op if the queue size is zero.
The fix has been contributed by Doug Lea. We are taking this
opportunity to synchronize PBQ with Doug's latest version at:
http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/main/java/util/concurrent/PriorityBlockingQueue.java?view=log
So in addition to the above functional fix there are a number of
miscellaneous code and comment cleanups. The only non-trivial, but
still very simple, change is to the drainTo method to make it more
robust if the add() on the destination collection throws an exception.
I am of course a Reviewer for this.
I have provided the update to the LastElement test (as this is
certainly related to the last element) but there are some reservations
about examining the PBS internals this way. Unfortunately there is no
good way to test for these kinds of retention issues. You either need
a whitebox test like this, or need to rely on non-guaranteed GC and
finalization behaviour.
Thanks,
David Holmes
Re: String.subSequence and CR#6924259: Remove offset and count fields from java.lang.String
On 22/06/2012 23:15, Mike Duigou wrote: : - The CharSequences returned by subSequence would follow only the general CharSequence rules for equals()/hashCode(). Any current usages of the result of subSequence for equals() or hashing, even though it's not advised, would break. We could add equals() and hashCode() implementations to the CharSequence returned but they would probably be expensive. I think Jason's idea to use CharBuffer.wrap(...).asReadOnlyBuffer() is a great idea too. However as you mention hashCode/equals then one thing to remember is that they depend on the number of remaining elements. It should be okay assuming that someone doesn't cast to a CharBuffer and invokes a method that changes the position or limit but even so, you won't get any caching of the hash code. -Alan
Re: RFR [JDK8]: 7169894: JAXP Plugability Layer: using service loader
On 25/06/2012 08:38, Joe Wang wrote:
Hi all,
Thanks for all the comments!
I've updated the patch for the following recommended changes:
1. ServiceConfigurationError instead of ConfigurationError
2. Use factory class, class.forName section removed
3. Use load method without specifying classloader
4. To be clear on how the process handles exceptions, I've updated
JavaDoc with the following:
* Uses the service-provider loading facilities, defined by the
{@link java.util.ServiceLoader} class, to attempt
* to locate and load an implementation of the service. In case
multiple providers exist, the first
* valid 3rd party provider should be returned; If there is no
valid 3rd party provider,
* the default implementation is returned if it is on the
classpath or installed as a module.
I think this is better, and I'm okay with your suggestion to deal with
the code duplication issue separately.
On the above wording then I think term 3rd party provider may provoke
questions. How about If there are providers other than the
implementation specific default located, then the first provider that is
not the default is instantiated and returned.
I'm not sure about catching and ignoring the ServiceConfigurationError
(or keep on trucking as Paul termed it in one of the replies). The
existing specification reads Any Exception thrown during the
instantiation process is wrapped as a somethingConfigurationException
so this seems a significant change to me.
Would it be cleaner if the FactoryFinder.find methods were changed to:
static T T find(ClassT c, String ...)
rather than using a raw type and returning Object? That would eliminate
some of the casts in this case. I guess if the short term is to have a
package-private copy of FactoryFinder in each package then it could be
changed to return the specific type. Related to this is the changes
increase the number of warnings by using ServiceLoader without a type
parameter.
-Alan.
Re: RFR: 7161229 - PriorityBlockingQueue keeps hard reference to last removed element
On 06/25/2012 01:17 PM, David Holmes wrote:
Hi Remi,
On 25/06/2012 7:01 PM, Rémi Forax wrote:
Hi David,
I think the test
if (n 0) {
should not be in siftDown* but should be done at all callsites,
by example in heapify, you can hoist the test outside of the loop.
I originally had it at the call-site (it already exists in a number of
them) but Doug prefers it to be internalized. He wrote:
... a better strategy is to move the n 0 check to the siftDown code
itself, so (existing and potential) callers don't need the check.
This can be mitigated by modifying the doc comment to say that n should
be strictly positive.
... an explicit n 0 check acts to hoist array index checks in loop
so turns out to be faster in general.
also true if n 0 is done at callsites and methods shiftDown* is
inlined, but this is not something
that is easy to guarantee because it depends on the complexity of the
comparator implementation.
so thumb up :)
In dequeue, it's a matter of style but the 'else' is not needed anymore.
Otherwise the changes looks ok for me.
Thanks,
David
cheers,
Rémi
Re: Bug 7176907 - Patches for javac warnings cleanup (text and util) from Adopt OpenJDK
Hi all,
Apologies for the delay. So it was simply a case of human error in
missing that last fallthrough (we wanted to double check that our
warnings script wasn't failing, hence the delay in getting back to
you). I've respun the patch with the extra SuppressWarning.
Hopefully the patch is complete now.
Cheers,
Martijn
On 20 June 2012 17:18, Martijn Verburg martijnverb...@gmail.com wrote:
We'll look into it, hopefully have an answer for you shortly - M
On 20 June 2012 17:07, Kurchi Subhra Hazra
kurchi.subhra.ha...@oracle.com wrote:
Hi,
I was just going through the patches, there are some more fallthrough
cases in
src/share/classes/java/util/regex/Pattern.java.(for example in line 2247).
Are these not generating warnings?
- Kurchi
On 6/20/12 7:30 AM, Martijn Verburg wrote:
Hi all,
Apologies, I didn't check that attachments were stripped. The patches
can be found at:
https://raw.github.com/AdoptOpenJDK/PatchReview/master/submitted/core_java_text.patch
https://raw.github.com/AdoptOpenJDK/PatchReview/master/submitted/core_java_util.patch
Cheers,
Martijn
Hi Martijn,
the two patches looks good.
A minor nit, why is there a space between the '(' and the readUByte() in
readUShort.
Thanks for the quick review! No reason on the whitespace, I've fixed that
now.
Quick question. Is there a checkstyle or jcheck that we should be
applying to any corelib patches going forwards?
Cheers,
Martijn
Re: [8] Review request for 7162111 change tests run in headless mode [macosx]
Hi Alan and Jason, On 06/23/12 11:28, Alan Bateman wrote: On 23/06/2012 02:01, Jason Uh wrote: This fix was for regression tests failing on Mac OS X on remotely executed environments. The changed tests now run in headless mode and have been taken off the Problem List. Webrev: http://cr.openjdk.java.net/~juh/7162111/webrev.00/ The CR: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7162111 Note that test/demo/jvmti/mtrace/TraceJFrame.java was not fixed here because headless mode is not supported for JFrame. A separate CR will be created for this. It's good to see these tests changed to run headless and will make the test execution much more reliable. Aside from the mtrace demo there are a couple of other tests that periodically hang initializing AWT, at least when running via ssh and then depending on whether someone is logged in and other configuration settings. Some of the shell tests for serialization come to mind (BTW: no problem doing that via a separate bug, just mentioning that there are other tests that are problems too). One question, and this may be a question for Artem or others, is that in CommonSetup.sh you set AWT_TOOLKIT=XToolkit. Is that right? I don't think we need to force XToolkit on the Mac. We don't quite support it on that platform actually. The normal headless CToolkit should work just fine. Could you please revert the changes to CommonSetup.sh and verify if the tests pass? -- best regards, Anthony
hg: jdk8/tl/jdk: 7176784: Windows authentication not working on some computers
Changeset: 4a4a04bfeece Author:chegar Date: 2012-06-25 14:19 +0100 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/4a4a04bfeece 7176784: Windows authentication not working on some computers Reviewed-by: michaelm ! src/windows/native/sun/net/www/protocol/http/ntlm/NTLMAuthSequence.c
Re: RFR [7u6]: 7166896: DocumentBuilder.parse(String uri) is not IPv6 enabled. It throws MalformedURLException
Hi Joe, What happens if there is a space character or other characters in the string that should be encoded ? http://greenbytes.de/tech/webdav/rfc2396.html#rfc.section.2.4.3 I suspect escapeNonUSAscii is slightly misleading, it should be really called something like escapeCharactersInUriString. Note that '[' and ']' are not valid URI characters outside of an IPv6 encoded address. Paul. On Jun 23, 2012, at 1:09 AM, Joe Wang wrote: Hi, This is a patch to fix the IPv6 issue. In a previous patch to fix an issue with system id containing international characters, an extra character escaping was added so that any URL passed to the parser goes through method escapeNonUSAscii before it's used to construct an URL. However, literal IPv6 addresses are enclosed in square brackets. The escapeNonUSAscii encoded address will become unrecognizable to URL that would throw a java.net.MalformedURLException. An address such as http://[fe80::la03:73ff:fead:f7b0]/note.xml is encoded as http://%5Bfe80::la03:73ff:fead:f7b0%5D/note.xml;, resulting in java.net.MalformedURLException: For input string: :la03:73ff:fead:f7b0%5D. This patch skips the encoding process and returns it as is if there're no non-ascii characters. webrev: http://cr.openjdk.java.net/~joehw/7u6/7166896/webrev/ Please review. Thanks, Joe
Re: RFR [JDK8]: 7169894: JAXP Plugability Layer: using service loader
H Joe,
I just focused on javax.xml.datatype and assumed the rest follows the same
pattern :-)
Like Alan i am still not sure about swallowing the ServiceConfigurationError.
It enables something that did not work before so i guess it does not take
anything away, if choose that this should be part of the spec then we should
document it (as we should the dependence on TCCL).
Also i notice that the previous code that explicitly loaded META-INF/service
files deferred to SecuritySupport that wraps stuff in
AccessController.doPrivileged blocks. Do we need to wrap the use of
ServiceLoader in such blocks too? since ServiceLoader will internally call
ClassLoader.getResources() and Thread.currentThread().getContextClassLoader().
Paul.
On Jun 25, 2012, at 9:38 AM, Joe Wang wrote:
Hi all,
Thanks for all the comments!
I've updated the patch for the following recommended changes:
1. ServiceConfigurationError instead of ConfigurationError
2. Use factory class, class.forName section removed
3. Use load method without specifying classloader
4. To be clear on how the process handles exceptions, I've updated JavaDoc
with the following:
* Uses the service-provider loading facilities, defined by the {@link
java.util.ServiceLoader} class, to attempt
* to locate and load an implementation of the service. In case multiple
providers exist, the first
* valid 3rd party provider should be returned; If there is no valid 3rd
party provider,
* the default implementation is returned if it is on the classpath or
installed as a module.
*
* If ServiceConfigurationError is thrown, it is interpreted as an invalid
provider is encountered.
* The process shall continue the search until all are exhausted.
The JavaDoc is similar but slightly different for SchemaFactory and
XPathFactory:
* pUses the service-provider loading facilities, defined by the {@link
java.util.ServiceLoader} class, to attempt
* to locate and load an implementation of the service.
* Each potential service provider is required to implement the
method:/p
* pre
*{@link #isSchemaLanguageSupported(String schemaLanguage)}
* /pre
* A service provider is deemed as valid if it supports the specified
schema language.
*
*p
* In case multiple providers exist, the first
* valid 3rd party provider should be returned; If there is no valid 3rd
party provider,
* the default implementation is returned if it is on the classpath or
installed as a module.
* /p
* If ServiceConfigurationError is thrown, it is interpreted as an invalid
provider is encountered.
* The process shall continue the search until all are exhausted.
We can continue discussing on a solution for the duplication of the classes.
For this patch, if you agree, I'd like to check it in as it stands now and
work out another patch for the duplicated classes once we have a solution.
New webrev:
http://cr.openjdk.java.net/~joehw/jdk8/7169894/webrev/
Thanks,
Joe
On 6/22/2012 10:24 AM, Joe Wang wrote:
On 6/22/2012 2:39 AM, Alan Bateman wrote:
On 21/06/2012 22:53, Joe Wang wrote:
:
We're not considering this change for jdk7 or older, i.e. I haven't
thought about fixing 6975142. For jdk8, it seems we have to live with the
way it is in (2), a seemly incompatible behavior in backward compatible
mode, although, it is a very rare case (I regret to have taught or sent
standalone jar files to those customers :) ).
No, this isn't for jdk7 or older.
I don't fully grok the issue in 6975142. That looks to me to be a case
where the service provider implementation is including its own copy of the
javax.xml.stream.* classes and this is the cause of the ClassCastException.
Yes, it's GF packaged woodstox that included its own API packages.
Joe
-Alan.
Re: RFR [JDK8]: 7169894: JAXP Plugability Layer: using service loader
On 6/25/2012 5:14 AM, Alan Bateman wrote:
On 25/06/2012 08:38, Joe Wang wrote:
Hi all,
Thanks for all the comments!
I've updated the patch for the following recommended changes:
1. ServiceConfigurationError instead of ConfigurationError
2. Use factory class, class.forName section removed
3. Use load method without specifying classloader
4. To be clear on how the process handles exceptions, I've updated
JavaDoc with the following:
* Uses the service-provider loading facilities, defined by the
{@link java.util.ServiceLoader} class, to attempt
* to locate and load an implementation of the service. In case
multiple providers exist, the first
* valid 3rd party provider should be returned; If there is no
valid 3rd party provider,
* the default implementation is returned if it is on the
classpath or installed as a module.
I think this is better, and I'm okay with your suggestion to deal with
the code duplication issue separately.
Ok.
On the above wording then I think term 3rd party provider may
provoke questions. How about If there are providers other than the
implementation specific default located, then the first provider that
is not the default is instantiated and returned.
That looks to be a better wording.
I'm not sure about catching and ignoring the ServiceConfigurationError
(or keep on trucking as Paul termed it in one of the replies). The
existing specification reads Any Exception thrown during the
instantiation process is wrapped as a
somethingConfigurationException so this seems a significant change
to me.
Indeed, that would be a significant change. I think I said I'm not going
to fix 6975142, or make it work for jdk7 and older in that matter, but
then I still allowed that to dictate the JavaDoc. I guess it's in my
mind for too long, or it's because the whole ServiceLoader thing was
actually started when I looked at old bugs and found 6975142 :)
Would it be cleaner if the FactoryFinder.find methods were changed to:
static T T find(ClassT c, String ...)
rather than using a raw type and returning Object? That would
eliminate some of the casts in this case. I guess if the short term is
to have a package-private copy of FactoryFinder in each package then
it could be changed to return the specific type. Related to this is
the changes increase the number of warnings by using ServiceLoader
without a type parameter.
JAXP code supported -source 1.4 -target 1.4, and changed to 1.5 not long
ago. There are lot of old code in JAXP for sure.
-Joe
-Alan.
Re: RFR [JDK8]: 7169894: JAXP Plugability Layer: using service loader
On 25/06/2012 18:03, Joe Wang wrote: : I'm not sure about catching and ignoring the ServiceConfigurationError (or keep on trucking as Paul termed it in one of the replies). The existing specification reads Any Exception thrown during the instantiation process is wrapped as a somethingConfigurationException so this seems a significant change to me. Indeed, that would be a significant change. I think I said I'm not going to fix 6975142, or make it work for jdk7 and older in that matter, but then I still allowed that to dictate the JavaDoc. I guess it's in my mind for too long, or it's because the whole ServiceLoader thing was actually started when I looked at old bugs and found 6975142 :) 6975142 seems to be a case of someone trying to use two versions of the JAXP API in the same application. That's a bit different to having different service provider implementations installed. For now I would suggest focusing on the latter. Jigsaw aside, I think it's it would be good for JAXP to be using ServiceLoader anyway. -Alan.
Re: RFR [JDK8]: 7169894: JAXP Plugability Layer: using service loader
On 6/25/2012 9:34 AM, Paul Sandoz wrote:
H Joe,
I just focused on javax.xml.datatype and assumed the rest follows the same
pattern :-)
Yeah, they are mostly similar, except Schema and XPath that do a little
extra check.
Like Alan i am still not sure about swallowing the ServiceConfigurationError.
It enables something that did not work before so i guess it does not take
anything away, if choose that this should be part of the spec then we should
document it (as we should the dependence on TCCL).
Agree. It would be a compatibility concern.
Also i notice that the previous code that explicitly loaded META-INF/service
files deferred to SecuritySupport that wraps stuff in
AccessController.doPrivileged blocks. Do we need to wrap the use of
ServiceLoader in such blocks too? since ServiceLoader will internally call
ClassLoader.getResources() and Thread.currentThread().getContextClassLoader().
Good catch, yes we do! We're virtually delegating those calls to the
ServiceLoader that could be blocked when security is on.
Joe
Paul.
On Jun 25, 2012, at 9:38 AM, Joe Wang wrote:
Hi all,
Thanks for all the comments!
I've updated the patch for the following recommended changes:
1. ServiceConfigurationError instead of ConfigurationError
2. Use factory class, class.forName section removed
3. Use load method without specifying classloader
4. To be clear on how the process handles exceptions, I've updated JavaDoc with
the following:
* Uses the service-provider loading facilities, defined by the {@link
java.util.ServiceLoader} class, to attempt
* to locate and load an implementation of the service. In case multiple
providers exist, the first
* valid 3rd party provider should be returned; If there is no valid 3rd
party provider,
* the default implementation is returned if it is on the classpath or
installed as a module.
*
* If ServiceConfigurationError is thrown, it is interpreted as an invalid
provider is encountered.
* The process shall continue the search until all are exhausted.
The JavaDoc is similar but slightly different for SchemaFactory and
XPathFactory:
*pUses the service-provider loading facilities, defined by the {@link
java.util.ServiceLoader} class, to attempt
* to locate and load an implementation of the service.
* Each potential service provider is required to implement the
method:/p
*pre
*{@link #isSchemaLanguageSupported(String schemaLanguage)}
*/pre
* A service provider is deemed as valid if it supports the specified
schema language.
*
*p
* In case multiple providers exist, the first
* valid 3rd party provider should be returned; If there is no valid 3rd
party provider,
* the default implementation is returned if it is on the classpath or
installed as a module.
*/p
* If ServiceConfigurationError is thrown, it is interpreted as an invalid
provider is encountered.
* The process shall continue the search until all are exhausted.
We can continue discussing on a solution for the duplication of the classes.
For this patch, if you agree, I'd like to check it in as it stands now and work
out another patch for the duplicated classes once we have a solution.
New webrev:
http://cr.openjdk.java.net/~joehw/jdk8/7169894/webrev/
Thanks,
Joe
On 6/22/2012 10:24 AM, Joe Wang wrote:
On 6/22/2012 2:39 AM, Alan Bateman wrote:
On 21/06/2012 22:53, Joe Wang wrote:
:
We're not considering this change for jdk7 or older, i.e. I haven't thought
about fixing 6975142. For jdk8, it seems we have to live with the way it is in
(2), a seemly incompatible behavior in backward compatible mode, although, it
is a very rare case (I regret to have taught or sent standalone jar files to
those customers :) ).
No, this isn't for jdk7 or older.
I don't fully grok the issue in 6975142. That looks to me to be a case where
the service provider implementation is including its own copy of the
javax.xml.stream.* classes and this is the cause of the ClassCastException.
Yes, it's GF packaged woodstox that included its own API packages.
Joe
-Alan.
Re: Review Request: CR 7100996 - (spec str) IndexOutOfBoundsException when using a StringBuffer from multiple threads
Hopefully, this will address most, if not all of the suggestions made to
date.
Jim
diff -r f37afaa214e9 src/share/classes/java/lang/StringBuffer.java
--- a/src/share/classes/java/lang/StringBuffer.javaMon Jun 25
14:22:42 2012 -0400
+++ b/src/share/classes/java/lang/StringBuffer.javaMon Jun 25
15:30:57 2012 -0400
@@ -39,40 +39,38 @@
* that is consistent with the order of the method calls made by each of
* the individual threads involved.
* p
- * The principal operations on a codeStringBuffer/code are the
- * codeappend/code and codeinsert/code methods, which are
+ * The principal operations on a {@code StringBuffer} are the
+ * {@code append} and {@code insert} methods, which are
* overloaded so as to accept data of any type. Each effectively
* converts a given datum to a string and then appends or inserts the
* characters of that string to the string buffer. The
- * codeappend/code method always adds these characters at the end
- * of the buffer; the codeinsert/code method adds the characters at
+ * {@code append} method always adds these characters at the end
+ * of the buffer; the {@code insert} method adds the characters at
* a specified point.
* p
- * For example, if codez/code refers to a string buffer object
- * whose current contents are codestart/code, then
- * the method call codez.append(le)/code would cause the string
- * buffer to contain codestartle/code, whereas
- * codez.insert(4, le)/code would alter the string buffer to
- * contain codestarlet/code.
+ * For example, if {@code z} refers to a string buffer object
+ * whose current contents are {@code start}, then
+ * the method call {@code z.append(le)} would cause the string
+ * buffer to contain {@code startle}, whereas
+ * {@code z.insert(4, le)} would alter the string buffer to
+ * contain {@code starlet}.
* p
- * In general, if sb refers to an instance of a codeStringBuffer/code,
- * then codesb.append(x)/code has the same effect as
- * codesb.insert(sb.length(),nbsp;x)/code.
+ * In general, if sb refers to an instance of a {@code StringBuffer},
+ * then {@code sb.append(x)} has the same effect as
+ * {@code sb.insert(sb.length(),nbsp;x)}.
* p
* Whenever an operation occurs involving a source sequence (such as
- * appending or inserting from a source sequence) this class synchronizes
+ * appending or inserting from a source sequence), this class synchronizes
* only on the string buffer performing the operation, not on the source.
- * p
- * Although {@code StringBuffer} is designed to be safe to use
- * concurrently from multiple threads, if the source data passed
- * to the constructor, i.e. {@code StringBuffer(source)}, or to the
- * {@code append(source)}, or {@code insert(source)} operations
- * is shared across threads, it must be ensured that the operations have
- * a consistent and unchanging view of the source data for the duration
- * of the operation.
+ * {@code StringBuffer} is designed to be safe to use
+ * concurrently from multiple threads, but if the source data passed
+ * to the constructor or to the {@code append()}, or {@code insert()}
+ * operations is shared across threads, the calling code must be ensure
+ * that the operations have a consistent and unchanging view of the source
+ * data for the duration of the operation.
* This could be satisfied by the caller holding a lock during the
- * operation's call, or because the source data is
- * immutable, or because the source data is not shared across threads.
+ * operation's call, or by the source data being
+ * immutable, or by the source data not being shared across threads.
* p
* Every string buffer has a capacity. As long as the length of the
* character sequence contained in the string buffer does not exceed
@@ -112,8 +110,8 @@
* the specified initial capacity.
*
* @param capacity the initial capacity.
- * @exception NegativeArraySizeException if the codecapacity/code
- * argument is less than code0/code.
+ * @exception NegativeArraySizeException if the {@code capacity}
+ * argument is less than {@code 0}.
*/
public StringBuffer(int capacity) {
super(capacity);
@@ -122,10 +120,10 @@
/**
* Constructs a string buffer initialized to the contents of the
* specified string. The initial capacity of the string buffer is
- * code16/code plus the length of the string argument.
+ * {@code 16} plus the length of the string argument.
*
* @param str the initial contents of the buffer.
- * @exception NullPointerException if codestr/code is
codenull/code
+ * @exception NullPointerException if {@code str} is {@code null}
*/
public StringBuffer(String str) {
super(str.length() + 16);
@@ -134,16 +132,16 @@
/**
* Constructs a string buffer that contains the same characters
- * as the specified codeCharSequence/code. The initial capacity of
- * the
RFR : 7162902 / 6893617 Umbrella port of a number of corba bug fixes from JDK 6 to jdk7u/8
Hi, I'm looking for a code review around the following corba changes. It turns out that we've a few bug fixes in corba area for jdk6 that were never forward ported to jdk7 or 8. The port is pretty much identical to what was used in JDK6. Some formatting and diamond operator changes introduced but that's about it. There were a few bug fixes to follow up on regressions around the initial fix but this umbrella port should cover all issues. The initial bugs fixes were fixed under the jets category in the bug tool and that category entered read only mode over a year ago. Hence the requirement for a new bug ID. The main bug fix is 6725987 which is an issue where references to the ORB were being kept after ORB.destroy was called. http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7162902 An accompanying CNCtx fix in JDK repo is also made to correct an issue where the java.naming.corba.orb system property wasn't used correctly. (CR 6893617) http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6893617 Corba testsuites run and no issues seen. I've also run some JCK testing and saw no issues. http://cr.openjdk.java.net/~coffeys/webrev.7162902.jdk8/ (corba) http://cr.openjdk.java.net/~coffeys/webrev.6893617.jdk8/ (jdk) The corba codebase code formatting seems to vary quite a bit. I've reformatted in a small number of areas but the whole corba repo probably warrants it's own clean up project at a later date. regards, Sean.
Re: Review Request: CR 7100996 - (spec str) IndexOutOfBoundsException when using a StringBuffer from multiple threads
Hi Jim,
maybe you like to read some more comments...
I'm not sure, wich would be better, but IMO should be used consistent:
{@code append()}
{@code append}
I think, the double quotes belong to the code for {@code start} etc. -- {@code
start}
I do not understand, why you change from term source sequence to term source
data at some point.
Problem with commas, grammar and wording:
* concurrently from multiple threads, but if the source data passed
* to the constructor or to the {@code append()}, or {@code insert()}
* operations is shared across threads, the calling code must be ensure
* that the operations have a consistent and unchanging view of the source
* data for the duration of the operation.
* This could be satisfied by the caller holding a lock during the
I would write:
* concurrently from multiple threads, but if the source data, passed
* to a constructor, to the {@code append()} or {@code insert()}
* operations, is shared across threads, the calling code must ensure,
* that they have a consistent and unchanging view on the source
* data for the duration of the operation.
* This could be satisfied by the caller, holding a lock during the
Wording:
* operation's call, or by the source data being
* immutable, or by the source data not being shared across threads.
I would write:
* operation's call, by immutable source data,
* or by not sharing the source data across threads.
Missing commas:
* character sequence contained in the string buffer does not exceed
* character sequence, contained in the string buffer, does not exceed
Missing comma at 2 locations of:
* object but does not synchronize on the source ({@code sb}).
* object, but does not synchronize on the source ({@code sb}).
-Ulf
Am 25.06.2012 21:32, schrieb Jim Gish:
Hopefully, this will address most, if not all of the suggestions made to date.
Jim
diff -r f37afaa214e9 src/share/classes/java/lang/StringBuffer.java
--- a/src/share/classes/java/lang/StringBuffer.javaMon Jun 25 14:22:42 2012
-0400
+++ b/src/share/classes/java/lang/StringBuffer.javaMon Jun 25 15:30:57 2012
-0400
@@ -39,40 +39,38 @@
* that is consistent with the order of the method calls made by each of
* the individual threads involved.
* p
- * The principal operations on a codeStringBuffer/code are the
- * codeappend/code and codeinsert/code methods, which are
+ * The principal operations on a {@code StringBuffer} are the
+ * {@code append} and {@code insert} methods, which are
* overloaded so as to accept data of any type. Each effectively
* converts a given datum to a string and then appends or inserts the
* characters of that string to the string buffer. The
- * codeappend/code method always adds these characters at the end
- * of the buffer; the codeinsert/code method adds the characters at
+ * {@code append} method always adds these characters at the end
+ * of the buffer; the {@code insert} method adds the characters at
* a specified point.
* p
- * For example, if codez/code refers to a string buffer object
- * whose current contents are codestart/code, then
- * the method call codez.append(le)/code would cause the string
- * buffer to contain codestartle/code, whereas
- * codez.insert(4, le)/code would alter the string buffer to
- * contain codestarlet/code.
+ * For example, if {@code z} refers to a string buffer object
+ * whose current contents are {@code start}, then
+ * the method call {@code z.append(le)} would cause the string
+ * buffer to contain {@code startle}, whereas
+ * {@code z.insert(4, le)} would alter the string buffer to
+ * contain {@code starlet}.
* p
- * In general, if sb refers to an instance of a codeStringBuffer/code,
- * then codesb.append(x)/code has the same effect as
- * codesb.insert(sb.length(),nbsp;x)/code.
+ * In general, if sb refers to an instance of a {@code StringBuffer},
+ * then {@code sb.append(x)} has the same effect as
+ * {@code sb.insert(sb.length(),nbsp;x)}.
* p
* Whenever an operation occurs involving a source sequence (such as
- * appending or inserting from a source sequence) this class synchronizes
+ * appending or inserting from a source sequence), this class synchronizes
* only on the string buffer performing the operation, not on the source.
- * p
- * Although {@code StringBuffer} is designed to be safe to use
- * concurrently from multiple threads, if the source data passed
- * to the constructor, i.e. {@code StringBuffer(source)}, or to the
- * {@code append(source)}, or {@code insert(source)} operations
- * is shared across threads, it must be ensured that the operations have
- * a consistent and unchanging view of the source data for the duration
- * of the operation.
+ * {@code StringBuffer} is designed to be safe to use
+ * concurrently from multiple threads, but if the source data passed
+ * to the constructor or to the {@code append()}, or {@code insert()}
+ * operations is shared across threads, the calling code must be ensure
+
Re: Review Request: CR 7100996 - (spec str) IndexOutOfBoundsException when using a StringBuffer from multiple threads
Yes be ensure was certainly not intended. Thanks for catching that slip-up.
Also, source sequence was intended. I'll check out the inconsistencies so
that we can nail this down on the next iteration.
Thanks,
Jim
- Original Message -
From: ulf.zi...@cosoco.de
To: jim.g...@oracle.com
Cc: core-libs-dev@openjdk.java.net
Sent: Monday, June 25, 2012 5:34:26 PM GMT -05:00 US/Canada Eastern
Subject: Re: Review Request: CR 7100996 - (spec str) IndexOutOfBoundsException
when using a StringBuffer from multiple threads
Hi Jim,
maybe you like to read some more comments...
I'm not sure, wich would be better, but IMO should be used consistent:
{@code append()}
{@code append}
I think, the double quotes belong to the code for {@code start} etc. --
{@code start}
I do not understand, why you change from term source sequence to term source
data at some point.
Problem with commas, grammar and wording:
* concurrently from multiple threads, but if the source data passed
* to the constructor or to the {@code append()}, or {@code insert()}
* operations is shared across threads, the calling code must be ensure
* that the operations have a consistent and unchanging view of the source
* data for the duration of the operation.
* This could be satisfied by the caller holding a lock during the
I would write:
* concurrently from multiple threads, but if the source data, passed
* to a constructor, to the {@code append()} or {@code insert()}
* operations, is shared across threads, the calling code must ensure,
* that they have a consistent and unchanging view on the source
* data for the duration of the operation.
* This could be satisfied by the caller, holding a lock during the
Wording:
* operation's call, or by the source data being
* immutable, or by the source data not being shared across threads.
I would write:
* operation's call, by immutable source data,
* or by not sharing the source data across threads.
Missing commas:
* character sequence contained in the string buffer does not exceed
* character sequence, contained in the string buffer, does not exceed
Missing comma at 2 locations of:
* object but does not synchronize on the source ({@code sb}).
* object, but does not synchronize on the source ({@code sb}).
-Ulf
Am 25.06.2012 21:32, schrieb Jim Gish:
Hopefully, this will address most, if not all of the suggestions made to date.
Jim
diff -r f37afaa214e9 src/share/classes/java/lang/StringBuffer.java
--- a/src/share/classes/java/lang/StringBuffer.javaMon Jun 25 14:22:42
2012 -0400
+++ b/src/share/classes/java/lang/StringBuffer.javaMon Jun 25 15:30:57
2012 -0400
@@ -39,40 +39,38 @@
* that is consistent with the order of the method calls made by each of
* the individual threads involved.
* p
- * The principal operations on a codeStringBuffer/code are the
- * codeappend/code and codeinsert/code methods, which are
+ * The principal operations on a {@code StringBuffer} are the
+ * {@code append} and {@code insert} methods, which are
* overloaded so as to accept data of any type. Each effectively
* converts a given datum to a string and then appends or inserts the
* characters of that string to the string buffer. The
- * codeappend/code method always adds these characters at the end
- * of the buffer; the codeinsert/code method adds the characters at
+ * {@code append} method always adds these characters at the end
+ * of the buffer; the {@code insert} method adds the characters at
* a specified point.
* p
- * For example, if codez/code refers to a string buffer object
- * whose current contents are codestart/code, then
- * the method call codez.append(le)/code would cause the string
- * buffer to contain codestartle/code, whereas
- * codez.insert(4, le)/code would alter the string buffer to
- * contain codestarlet/code.
+ * For example, if {@code z} refers to a string buffer object
+ * whose current contents are {@code start}, then
+ * the method call {@code z.append(le)} would cause the string
+ * buffer to contain {@code startle}, whereas
+ * {@code z.insert(4, le)} would alter the string buffer to
+ * contain {@code starlet}.
* p
- * In general, if sb refers to an instance of a codeStringBuffer/code,
- * then codesb.append(x)/code has the same effect as
- * codesb.insert(sb.length(),nbsp;x)/code.
+ * In general, if sb refers to an instance of a {@code StringBuffer},
+ * then {@code sb.append(x)} has the same effect as
+ * {@code sb.insert(sb.length(),nbsp;x)}.
* p
* Whenever an operation occurs involving a source sequence (such as
- * appending or inserting from a source sequence) this class synchronizes
+ * appending or inserting from a source sequence), this class synchronizes
* only on the string buffer performing the operation, not on the source.
- * p
- * Although {@code StringBuffer} is designed to be safe to use
- * concurrently from multiple
