Re: A bug in filesystem bootstrap (unix/ linux) prevents

[Xueming Shen]

The code cited is a little shortcut, if there is locale over there is 
indeed using
utf-16, or any encoding that needs to switch/shift into ASCII (or its 
single byte
charset area) with a shift/in/out character.. So far I'm not aware of 
any such
a locale on any our supported platform. Historically, this kind of 
assumption

might run into trouble when being ported to other platform, such as ebcdic
based system (but I don't think it's a problem in this case). Ideally, 
the code
probably should be coded to be able to deal with a mb type of "/", but 
obviously

it was decided to take the short-cut for better performance here.

"We" have been taking the stand that file.encoding is an 
informative/read-only

system property for a long time, mainly because of two reasons. First this
property is really defined/implemented/used as the default encoding that 
the jvm
uses to communicated with the underlying platform for local/encoding 
sensitive
stuff, the default encoding of the file content, the encoding of the 
file path and
the "text" encoding when use the platform APIs, for example. It's like a 
"contract"
between the jvm and the underlying platform, it needs to be understood 
by both
and agreed on by both. So it needs to be set based on what your 
underlying system
is using,  not something you want to set via either -D or 
System.setProperty. If
your underlying locale is not UTF-16, I don't think you should expect 
the jvm
could work correctly if it keeps "talking" in UTF-16 to the underlying 
system,

for example, pass in a file name in utf-16, when your are running on a utf-8
locale (it is more complicated on a windows platform, when you have system
locale and user locale, and historically file.encoding was used for 
both, consider

if your system locale and user locale are set differently...).

The property sun.jnu.encoding introduced in jdk6 (this is mainly
to address the issue we have with file.encoding on windows platform though)
somehow helps remove some "pressure" from the file.encoding, so in theory
file.encoding should be used to only for the encoding of "file content", and
the sun.jnu.encoding should be used when you need the encoding to talk to
those  platform APIs, so something might be done here (currently 
file.encoding

and sun.jnu.encoding are set to the same thing on non-Windows platform).

The other reason is the timing of how the file.encoding is being 
initialized and
how it is being used during the "complicated" system initialization 
stage, almost

everyone touched System. initializeSystemClass() got burned here and there
in the past:-)  So sometime you want to ask if it is worth the risk to 
change

something work for a use scenario that is not "supported". That said, as
I said above,  something might be done  to address this issue, but obviously
not a priority for now.

-Sherman

if you want to do -Dfile.encoding=xyz, you
are on your own, it might work, it might not work.

On 7/4/2012 11:00 PM, Dawid Weiss wrote:

Well, what's the "right" way to enforce an initial encoding for
charset-less string-to-byte conversions and legacy streams? I still
think that snippet of code is buggy, no matter if file.encoding is or
isn't a supported settable property.

Besides, from what I see in JDK code base everything seems to be code
in a way to allow external definition of file.encoding (comments
inside System.c for example). Where is it stated that file.encoding is
read-only?

Dawid

On Thu, Jul 5, 2012 at 3:09 AM, Xueming Shen  wrote:

-Dfile.encoding=xyz is NOT a supported configuration. file.encoding is
supposed to be a read-only informative system property.

-Sherman


On 7/4/2012 1:21 PM, Dawid Weiss wrote:

There is a similar bug:
Bug 6795536 - No system start for file.encoding=x-SJIS_0213

Yeah... I looked at the sources in that package and there is at least
one more place which converts a String to bytes using getBytes(). This
seems to be a trivial fix in UnixFileSystem though. Anyway, bug ID for
this is:

http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7181721

Dawid


In this case on Windows.

-Ulf


Am 04.07.2012 14:43, schrieb Dawid Weiss:

Hi folks.

Run the following with -Dfile.encoding=UTF-16:

public class TestBlah {
public static void main(String []) throws Exception {
  TimeZone.getDefault();
}
}

This on linux (and any unixish system I think) will result in:

java.lang.ExceptionInInitializerError
 at java.nio.file.FileSystems.getDefault(FileSystems.java:176)
 at sun.util.calendar.ZoneInfoFile$1.run(ZoneInfoFile.java:482)
 at sun.util.calendar.ZoneInfoFile$1.run(ZoneInfoFile.java:477)
...

There is an encoding-sensitive part calling getBytes on the initial
path (and this screws it up):

  // package-private
  UnixFileSystem(UnixFileSystemProvider provider, String dir) {
  this.provider = provider;
  this.defaultDirectory =
UnixPath.normalizeAndCheck(dir).getBytes();
  if (this.defaultDirectory[0] != '/') {
   

Re: A bug in filesystem bootstrap (unix/ linux) prevents

[Dawid Weiss]

Thanks for the explanation, it helps.

Maybe I should follow-up with the rationale of why we want to override
it in the first place. We randomize file.encoding to a small subset of
JVM-supported encodings to make sure there are no hidden
encoding-specific issues in Lucene/ Solr. This is actually a great way
of catching calls to String.getBytes() and such which typically work
fine but once moved to actual user environment start to break because
of different encodings.

Maybe I'm missing something but -Dfile.encoding seems to be the only
way to change the defaults used for String.getBytes(), new
String(byte[]) and such, correct? Sure, it's legacy APIs that are
broken/ problematic from the very beginning but they're there and
there should be a way to externally say "use this and that charset for
the default"...

Yes, we could do static analysis to catch these but the problem remains.

Dawid

On Thu, Jul 5, 2012 at 9:52 AM, Xueming Shen  wrote:
> The code cited is a little shortcut, if there is locale over there is indeed
> using
> utf-16, or any encoding that needs to switch/shift into ASCII (or its single
> byte
> charset area) with a shift/in/out character.. So far I'm not aware of any
> such
> a locale on any our supported platform. Historically, this kind of
> assumption
> might run into trouble when being ported to other platform, such as ebcdic
> based system (but I don't think it's a problem in this case). Ideally, the
> code
> probably should be coded to be able to deal with a mb type of "/", but
> obviously
> it was decided to take the short-cut for better performance here.
>
> "We" have been taking the stand that file.encoding is an
> informative/read-only
> system property for a long time, mainly because of two reasons. First this
> property is really defined/implemented/used as the default encoding that the
> jvm
> uses to communicated with the underlying platform for local/encoding
> sensitive
> stuff, the default encoding of the file content, the encoding of the file
> path and
> the "text" encoding when use the platform APIs, for example. It's like a
> "contract"
> between the jvm and the underlying platform, it needs to be understood by
> both
> and agreed on by both. So it needs to be set based on what your underlying
> system
> is using,  not something you want to set via either -D or
> System.setProperty. If
> your underlying locale is not UTF-16, I don't think you should expect the
> jvm
> could work correctly if it keeps "talking" in UTF-16 to the underlying
> system,
> for example, pass in a file name in utf-16, when your are running on a utf-8
> locale (it is more complicated on a windows platform, when you have system
> locale and user locale, and historically file.encoding was used for both,
> consider
> if your system locale and user locale are set differently...).
>
> The property sun.jnu.encoding introduced in jdk6 (this is mainly
> to address the issue we have with file.encoding on windows platform though)
> somehow helps remove some "pressure" from the file.encoding, so in theory
> file.encoding should be used to only for the encoding of "file content", and
> the sun.jnu.encoding should be used when you need the encoding to talk to
> those  platform APIs, so something might be done here (currently
> file.encoding
> and sun.jnu.encoding are set to the same thing on non-Windows platform).
>
> The other reason is the timing of how the file.encoding is being initialized
> and
> how it is being used during the "complicated" system initialization stage,
> almost
> everyone touched System. initializeSystemClass() got burned here and there
> in the past:-)  So sometime you want to ask if it is worth the risk to
> change
> something work for a use scenario that is not "supported". That said, as
> I said above,  something might be done  to address this issue, but obviously
> not a priority for now.
>
> -Sherman
>
> if you want to do -Dfile.encoding=xyz, you
> are on your own, it might work, it might not work.
>
>
> On 7/4/2012 11:00 PM, Dawid Weiss wrote:
>>
>> Well, what's the "right" way to enforce an initial encoding for
>> charset-less string-to-byte conversions and legacy streams? I still
>> think that snippet of code is buggy, no matter if file.encoding is or
>> isn't a supported settable property.
>>
>> Besides, from what I see in JDK code base everything seems to be code
>> in a way to allow external definition of file.encoding (comments
>> inside System.c for example). Where is it stated that file.encoding is
>> read-only?
>>
>> Dawid
>>
>> On Thu, Jul 5, 2012 at 3:09 AM, Xueming Shen
>> wrote:
>>>
>>> -Dfile.encoding=xyz is NOT a supported configuration. file.encoding is
>>> supposed to be a read-only informative system property.
>>>
>>> -Sherman
>>>
>>>
>>> On 7/4/2012 1:21 PM, Dawid Weiss wrote:
>
> There is a similar bug:
> Bug 6795536 - No system start for file.encoding=x-SJIS_0213

 Yeah... I looked at the sources in that package and there is at least
>>>

[Patch] Review request - bug 7147060 com/sun/org/apache/xml/internal/security/transforms/ClassLoaderTest.java doesn't run in agentvm mode

[Eric Wang]

Hi David & Stuart,

Can you please help to review the fix for the bug 7147060 
, Thanks you!


The test reports ClassNotFoundException which is caused by below reasons:
1. Class can be found in "test.classes" instead of "test.src".
2. In agentvm mode, the MyTransform.class is loaded by the child of the 
context ClassLoader of current thread, however, The method 
Transform.register(String, String) tries to use context ClassLoader to 
load class MyTransform.class which is not found.


The proposed fix is:
1. Replace "test.src" to "test.classes".
2. Replace the current thread ClassLoader to its child.

Regards,
Eric
--- old/test/ProblemList.txt2012-07-05 15:57:49.477164126 +0800
+++ new/test/ProblemList.txt2012-07-05 15:57:45.177070426 +0800
@@ -290,9 +290,6 @@
 # 7177556
 com/sun/crypto/provider/KeyFactory/TestProviderLeak.javageneric-all
 
-# 7147060
-com/sun/org/apache/xml/internal/security/transforms/ClassLoaderTest.java   
generic-all
-
 # Failing on Solaris i586, 3/9/2010, not a -samevm issue (jdk_security3)
 sun/security/pkcs11/Secmod/AddPrivateKey.java   solaris-i586
 sun/security/pkcs11/ec/ReadCertificates.javageneric-all
--- 
old/test/com/sun/org/apache/xml/internal/security/transforms/ClassLoaderTest.java
   2012-07-05 15:57:56.799140287 +0800
+++ 
new/test/com/sun/org/apache/xml/internal/security/transforms/ClassLoaderTest.java
   2012-07-05 15:57:54.790043435 +0800
@@ -39,7 +39,7 @@
 
 public class ClassLoaderTest {
 
-private final static String BASE = System.getProperty("test.src", "./");
+private final static String BASE = System.getProperty("test.classes", 
"./");
 
 public static void main(String[] args) throws Exception {
 
@@ -50,15 +50,26 @@
 URLClassLoader ucl = new URLClassLoader(urls);
 Class c = ucl.loadClass("MyTransform");
 Constructor cons = c.getConstructor();
-Object o = cons.newInstance();
-// Apache code swallows the ClassNotFoundExc, so we need to
-// check if the Transform has already been registered by registering
-// it again and catching an AlgorithmAlreadyRegisteredExc
+
+Thread curThread = Thread.currentThread();
+ClassLoader ctxLoader = curThread.getContextClassLoader();
+ClassLoader loader = MyTransform.class.getClassLoader();
 try {
+// In agentvm mode, the class MyTransform is loaded by the child of
+// context ClassLoader of this thread. Replace context ClassLoader
+// with its child to avoid ClassNotFoundException thrown from
+// Transform.register(String, String) method.
+curThread.setContextClassLoader(loader);
+Object o = cons.newInstance();
+// Apache code swallows the ClassNotFoundExc, so we need to
+// check if the Transform has already been registered by 
registering
+// it again and catching an AlgorithmAlreadyRegisteredExc
 Transform.register(MyTransform.URI, "MyTransform");
 throw new Exception("ClassLoaderTest failed");
 } catch (AlgorithmAlreadyRegisteredException e) {
 // test passed
+} finally {
+curThread.setContextClassLoader(ctxLoader);
 }
 }
 }

Re: A bug in filesystem bootstrap (unix/ linux) prevents

[Xueming Shen]

Given you are using -Dfile.encoding=xyz from command line, guess you might
not have problem to do something like (on a bash for example)

export LC_ALL=en_US.UTF-8; java Foo
export LC_ALL=ja_JP.SJIS; java Foo

you might need to install all those supported locales on you system 
though, but

isn't it something you are trying to test on? some actual users run on some
actual user environment. You only need to test the locales (with various 
supported
encodings) that really exist on your platform. In fact -Dfile.encoding 
actually

is not a good testing methodology here.

 I would assume there is no en_US.UTF-16 locale there :-)

-Sherman

On 7/5/2012 1:04 AM, Dawid Weiss wrote:

Thanks for the explanation, it helps.

Maybe I should follow-up with the rationale of why we want to override
it in the first place. We randomize file.encoding to a small subset of
JVM-supported encodings to make sure there are no hidden
encoding-specific issues in Lucene/ Solr. This is actually a great way
of catching calls to String.getBytes() and such which typically work
fine but once moved to actual user environment start to break because
of different encodings.

Maybe I'm missing something but -Dfile.encoding seems to be the only
way to change the defaults used for String.getBytes(), new
String(byte[]) and such, correct? Sure, it's legacy APIs that are
broken/ problematic from the very beginning but they're there and
there should be a way to externally say "use this and that charset for
the default"...

Yes, we could do static analysis to catch these but the problem remains.

Dawid

On Thu, Jul 5, 2012 at 9:52 AM, Xueming Shen  wrote:

The code cited is a little shortcut, if there is locale over there is indeed
using
utf-16, or any encoding that needs to switch/shift into ASCII (or its single
byte
charset area) with a shift/in/out character.. So far I'm not aware of any
such
a locale on any our supported platform. Historically, this kind of
assumption
might run into trouble when being ported to other platform, such as ebcdic
based system (but I don't think it's a problem in this case). Ideally, the
code
probably should be coded to be able to deal with a mb type of "/", but
obviously
it was decided to take the short-cut for better performance here.

"We" have been taking the stand that file.encoding is an
informative/read-only
system property for a long time, mainly because of two reasons. First this
property is really defined/implemented/used as the default encoding that the
jvm
uses to communicated with the underlying platform for local/encoding
sensitive
stuff, the default encoding of the file content, the encoding of the file
path and
the "text" encoding when use the platform APIs, for example. It's like a
"contract"
between the jvm and the underlying platform, it needs to be understood by
both
and agreed on by both. So it needs to be set based on what your underlying
system
is using,  not something you want to set via either -D or
System.setProperty. If
your underlying locale is not UTF-16, I don't think you should expect the
jvm
could work correctly if it keeps "talking" in UTF-16 to the underlying
system,
for example, pass in a file name in utf-16, when your are running on a utf-8
locale (it is more complicated on a windows platform, when you have system
locale and user locale, and historically file.encoding was used for both,
consider
if your system locale and user locale are set differently...).

The property sun.jnu.encoding introduced in jdk6 (this is mainly
to address the issue we have with file.encoding on windows platform though)
somehow helps remove some "pressure" from the file.encoding, so in theory
file.encoding should be used to only for the encoding of "file content", and
the sun.jnu.encoding should be used when you need the encoding to talk to
those  platform APIs, so something might be done here (currently
file.encoding
and sun.jnu.encoding are set to the same thing on non-Windows platform).

The other reason is the timing of how the file.encoding is being initialized
and
how it is being used during the "complicated" system initialization stage,
almost
everyone touched System. initializeSystemClass() got burned here and there
in the past:-)  So sometime you want to ask if it is worth the risk to
change
something work for a use scenario that is not "supported". That said, as
I said above,  something might be done  to address this issue, but obviously
not a priority for now.

-Sherman

if you want to do -Dfile.encoding=xyz, you
are on your own, it might work, it might not work.


On 7/4/2012 11:00 PM, Dawid Weiss wrote:

Well, what's the "right" way to enforce an initial encoding for
charset-less string-to-byte conversions and legacy streams? I still
think that snippet of code is buggy, no matter if file.encoding is or
isn't a supported settable property.

Besides, from what I see in JDK code base everything seems to be code
in a way to allow external definition of file.encoding (comments
inside System.c for 

Re: A bug in filesystem bootstrap (unix/ linux) prevents

[Dawid Weiss]

> export LC_ALL=en_US.UTF-8; java Foo

Not really, the shell won't let you use a multibyte locale (because of
issues with null-terminated strings). And multibyte (with BOM) is most
fun when you're trying to find buggy code ;)

>  I would assume there is no en_US.UTF-16 locale there :-)

I wish there were. It'd make people care more ;)

Dawid

Re: A bug in filesystem bootstrap (unix/ linux) prevents

[Xueming Shen]

On 07/05/2012 01:40 AM, Dawid Weiss wrote:

export LC_ALL=en_US.UTF-8; java Foo

Not really, the shell won't let you use a multibyte locale (because of
issues with null-terminated strings). And multibyte (with BOM) is most
fun when you're trying to find buggy code ;)


Encodings for those Chinese, Japnese, Korean locales are all "multibyte" 
. UTF-8
is a multibyte encoding, most recent unix/linux platform should have no 
problem
to work with "multibyte" locale. In fact UTF-16 is normally not 
categorized as
multibye (mb), but wide char, as "wc". There are reason(s) why you 
(normally)
only see utf-8 locale but no utf-16 locale on Unix/Linux based platforms 
and why
you have "W" version of APIs and "A" version of APIs (and even "T" 
version for

some APIs) on Windows platfrom.

I agree it might be helpful if there is mechanism that you can change 
the "default
charset" used by various Java APIs, similar to what you do with 
Locale.setDefault().
With the introduction of sun.jnu.encoding (which takes over the 
responsibility of
the encoding jvm used to talk to the underlying OS APIs) it might be 
possible to
reduce the scope of system property file.encoding to only for the 
default encoding
of the "file content" and do something here, but it is not on the 
priority list for now.


-Sherman

Btw, I need to make it clear here that sun.jnu.encoding is purely an 
implementation

detail, app is not supposed to use it for whatever purpose.


  I would assume there is no en_US.UTF-16 locale there :-)

I wish there were. It'd make people care more ;)

Dawid

Re: javax.sql.rowset.serial.SerialBlob doesn't support free and getBinaryStream

[Lance Andersen - Oracle]

Hi Deven,

We had a holiday here on Weds,  so I am still catching up.

First, thank you for taking the time to propose a patch.

 Here are a few comments based on the changes I have made in my workspace and  
comparing to your proposed changes 

- The same issue applies to SerialClob
- instead of duplicating all of the code to check if free has been called in 
each method, I created a private method which does the validation
- I do not see a reason to make the flag isFree transient, if the object has 
been freed, it has been freed
- free() needs to call blob.free if the blob object is not null prior to 
nulling out the object
- Your 2nd if statement in getBinaryStream() duplicates part of the check in 
the 1st if statement
- All of the public methods need to have their javadocs indicate that if called 
after free has been called, a SerialException will be thrown.  additional calls 
to free is a no-op
- The test case is is a good start.I would change this a bit though:
  + create separate test classes for free and getbinarystream
  + each individual test  be a method
  + If you catch the expected Exception, print that the test has passed
  + Each test needs a comment as to what it is trying to validate (just a 
simple comment is fine)
  + return 0 if the test passed, 1 if it fails  and then print the number of 
failed tests at the end after running through all of them


As I need to change the javadocs, I have create  a ccc request which I will do 
as part of the JDBC 4.2 work and will put the change back at that time.

If you would like to change your test and then create similar tests for 
SerialClob I will add those as part of the put-back.

Again, thank you for your input and time.

Best
Lance




On Jul 5, 2012, at 2:26 AM, Deven You wrote:

> Hi Lance,
> 
> Did you review the patch and compare it to yours. I just have some more words 
> for the patch.
> 
> 1. I think the current spec for free() is not clear, how about add below 
> comments:
> 
> After free has been called, any attempt to invoke a method other than 
> free will result in a  SerialException being thrown.
> 
> 2. getBinaryStream(long pos,long length)
> 
> add a javadoc:
> * @throws SerialException if this SerialBlob already be freed.
> 
> add throws SerialException from this method
> 
> Any suggestions?
> 
> Thanks a lot!
> On 07/02/2012 06:25 PM, Lance Andersen - Oracle wrote:
>> Hi Deven,
>> 
>> Thanks for the email and the proposed patch.  I will look at this later 
>> today or tomorrow.  I actually have made these changes in my workspace for 
>> JDK 8 but will compare your changes to mine.
>> 
>> Best
>> Lance
>> On Jul 2, 2012, at 5:04 AM, Deven You wrote:
>> 
>>> Hi All,
>>> Could anyone notice this problem?
>>> 
>>> Thanks a lot!
>>> On 06/25/2012 04:18 PM, Deven You wrote:
 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
>>> 
>> 
>> 
>> Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
>> Oracle Java Engineering 
>> 1 Network Drive 
>> Burlington, MA 01803
>> lance.ander...@oracle.com
>> 
> 
> 
> -- 
> Best Regards,
> 
> Deven


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

Code Review Request 7142596: RMI JPRT tests are failing

[Darryl Mocek]

Hello core-libs.  Please review this webrev to fix Bugs #7142596 and 
7161503.  Webrev can be found here: 
http://cr.openjdk.java.net/~dmocek/7142596/webrev.02. This commit fixes 
concurrency issues with the RMI tests.


- Added TestLibrary.createRegistryOnUnusedPort method.  This creates an 
RMIRegistry on an unused port. It will try up to 10 times before giving up.
- Added a TestLibrary.getRegistryPort(Registry) method to get the port 
number of the registry.
- Changed almost all tests from using hard port numbers to using random 
port numbers for running the RMI Registry and RMID.

- Removed othervm from those tests which don't need it.
- Added parameters for tests which spawn a separate VM to pass RMI 
Registry and RMID ports in cases where needed.

- Added PropertyPermission to security policy files where needed.
- Removed java/rmi and sun/rmi from tests which cannot be run concurrently.
- Added java/rmi/Naming to list of tests which cannot be run concurrently.

Thanks,
Darryl

hg: jdk8/tl/jdk: 6948101: java/rmi/transport/pinLastArguments/PinLastArguments.java failing intermittently

[stuart . marks]

Changeset: 97eb7a4b1fdd
Author:smarks
Date:  2012-07-05 15:12 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/97eb7a4b1fdd

6948101: java/rmi/transport/pinLastArguments/PinLastArguments.java failing 
intermittently
Reviewed-by: dholmes, smarks
Contributed-by: Eric Wang 

! test/ProblemList.txt
! test/java/rmi/transport/pinLastArguments/PinLastArguments.java

hg: jdk8/tl/jdk: 7123972: test/java/lang/annotation/loaderLeak/Main.java fails intermittently

[stuart . marks]

Changeset: 4ad204cc7433
Author:smarks
Date:  2012-07-05 15:13 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/4ad204cc7433

7123972: test/java/lang/annotation/loaderLeak/Main.java fails intermittently
Reviewed-by: dholmes, smarks
Contributed-by: Eric Wang 

! test/ProblemList.txt
! test/java/lang/annotation/loaderLeak/Main.java

Re: [PATCH] Review Request - Test bug: 6948101 java/rmi/transport/pinLastArguments/PinLastArguments.java failing intermittently

[Stuart Marks]

Pushed:

http://hg.openjdk.java.net/jdk8/tl/jdk/rev/97eb7a4b1fdd

s'marks

On 7/3/12 6:54 PM, Eric Wang wrote:

Opps, It is my carelessness, I will be more careful in the next bug fixes.
Thank you to review and change it.

Regards,
Eric

On 2012/7/4 8:58, Stuart Marks wrote:

Sure, I can take care of that.

If this is the only change, no need to generate another webrev, Eric.

s'marks


On 7/3/12 4:53 PM, David Holmes wrote:

Please fix the missing space in

if(ref.get()

before pushing.

Thanks,
David

On 4/07/2012 7:04 AM, Stuart Marks wrote:

Webrev now posted at

http://cr.openjdk.java.net/~smarks/yiming.wang/6948101/webrev.2/

The changes look good to me. If there's no further discussion, I'll push
them in a couple days (U.S. holiday coming up).

s'marks

On 7/3/12 12:20 AM, Stuart Marks wrote:

This approach looks good to me. Please go ahead and update 7123972 as
well, and
I'll post these webrevs tomorrow (my time).

Thanks.

s'marks

On 7/2/12 11:41 PM, Eric Wang wrote:

David,

I have added the comments before the loop, please help to review. If
it is OK,
I'll update the 7123972 as well.

Thanks,
Eric

On 2012/7/3 12:22, David Holmes wrote:

Hi Eric,

On 3/07/2012 1:28 PM, Eric Wang wrote:

Hi Stuart and David,

Thanks for the suggestion about the timeout. so there are three
ways to
handle the timeout.

1. Add comment to explain that test failed due to default timeout
2. Specify the the timeout option
3. Timeout programly

Is it ok for you that i'd like to choose the second way "explict
timeout" as the test looks simply and lucid?


If by #2 you mean add something like:

@run main/timeout=10

then no. Earlier this year there were changes to a bunch of tests
that set
timeouts shorter than the jtreg default, to delete the timeouts.
Given that,
my misgivings about relying on the harness are not relevant so we
need only
do #1 and add a comment to make it clear that a failing test will be
indicated via the harness timeout.

David
-


Regards,
Eric

On 2012/7/3 10:38, David Holmes wrote:

On 3/07/2012 7:08 AM, Stuart Marks wrote:

On 7/1/12 5:39 PM, David Holmes wrote:

Now, how can the test fail? If ref is never cleared, the while
loop
will
never terminate, and we rely on jtreg to timeout and kill this
test. It
would be good to have a brief comment above the while loop that
explains
this.


Agreed. Something like

// Might require multiple calls to System.gc() for weak-references
processing
to be complete.
// If the weak-reference is not cleared as expected we will hang
here
// until timed out by the test harness

Though now I write that I'm unhappy that this will only behave
nicely
if run
from jtreg. We do use explicit timeouts in other tests.


So, are you unhappy enough that we should ask Eric to add an
explicit
timeout?


No. But I'm not the authoritive voice in this area :)


I don't think it would be that difficult. Perhaps a technique
like the
following would suffice. Get System.currentTimeMillis() before
the loop,
and call it again each time through the loop, and fail if the
difference
is greater than the timeout (e.g., 60 sec). Doesn't involve other
threads or even Timer/TimerTask.


Or simply limit the number of loops so that N*sleepTime = timeout.
This then requires adding back the check for null outside the loop.


On the other hand if one is running this test directly instead of
through jtreg, one can just ^C it if it's taking an unexpectedly
long
time.


True.

David
-



s'marks

Re: Patch review request - Test bug 7123972 test/java/lang/annotation/loaderLeak/Main.java fails intermittently

[Stuart Marks]

Pushed:

http://hg.openjdk.java.net/jdk8/tl/jdk/rev/4ad204cc7433

s'marks

On 7/3/12 2:04 PM, Stuart Marks wrote:

Webrev posted here:

http://cr.openjdk.java.net/~smarks/yiming.wang/7123972/webrev.2/

The changes look good. If there's no further discussion, I'll push them in a
couple days.

s'marks

On 7/3/12 3:37 AM, Eric Wang wrote:

Hi Stuart,

I have made updates which is same as 6948101, please help to review, Thanks a
lot!

Regards,
Eric
On 2012/6/30 6:01, Stuart Marks wrote:

I've posted the updated webrev here:

http://cr.openjdk.java.net/~smarks/yiming.wang/7123972/webrev.1/

This code is pretty much the same as 6948101 and the same comments I had on
that bug apply here as well, so please make similar updates to this one.

Thanks.

s'marks

On 6/29/12 1:36 AM, Eric Wang wrote:

Hi Stuart & David??

Attachment is the new changes to make code simply by following David
suggestion, Can you help please review again, Thanks a lot!

Regards,
Eric
On 2012/6/28 12:40, Stuart Marks wrote:

I've posted the webrev here:

http://cr.openjdk.java.net/~smarks/yiming.wang/7123972/webrev.0/

I've looked at the changes and they seem fine.

It's interesting that the run times take 30-60 sec in your experience. I've
run them on my system (Linux in a virtual machine) and they usually run in
10-20 sec. In any case, as you say, if the test times out it indicates there
really was a failure.

I'll give people a chance to look at the webrev and if there aren't any more
comments in another day or so I'll push in the changeset.

Thanks for developing this!

s'marks

On 6/26/12 11:50 PM, Eric Wang wrote:

Hi David,

Thank you! I run the test several times on 3 platforms (windows, solaris and
linux), the average execution time is 30secs to 60secs. if the test hang 2
minutes, there should be something wrong.

Hi Marks,

I don't have the author role, Can you please help to review and post the
webrev
7123972.zip in attachment if it is OK, Thanks a lot!

Regards,
Eric

On 2012/6/27 14:19, David Holmes wrote:

Eric,

Ignore my comment about adding the timeouts. As Alan reminded me if the
test
would hang then jtreg will time it out after two minutes anyway.

So this is good to go as far as I am concerned.

Thanks,
David

On 27/06/2012 7:51 AM, David Holmes wrote:

Thanks Eric.

So to summarize:

- we create a custom classloader, load a class through it and store a
reference to that Class in a WeakReference
- we then drop the reference to the loader

At this point the only reference to the loader is from the Class loaded,
which in turn is only weakly reachable.

I must confess that I'm unclear why this test should be failing in its
original form. The first gc() should remove the strong reference to the
loader. That leaves a weak cycle: WeakRef -> Class -> Loader -> Class.
The second gc() should detect the cycle and clear the WeakRef. I guess
the problem is that depending on which gc is in use the actual gc()
calls may not in fact induce every possible GC action.

The fix seems reasonable in that regard - keep gc'ing and finalizing
till we see the loader is gone, and so the WeakReference should be
cleared. The original bug would cause a ref to the Class to remain (from
the annotation) and hence the WeakRef would not be cleared. However, in
that case the loader would also be strongly referenced and so never
become finalizable. So if this test was now run against a JDK with the
original bug, the test would hang. So I think we need to add a timeout
in there as well.

David

On 25/06/2012 6:06 PM, Eric Wang wrote:

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
, 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 - specifi

Re: [Patch] Review request - bug 7147060 com/sun/org/apache/xml/internal/security/transforms/ClassLoaderTest.java doesn't run in agentvm mode

[Stuart Marks]

(Note to core-libs-dev: further review and discussion of this bugfix is 
occurring on security-dev.)


On 7/5/12 1:18 AM, Eric Wang wrote:

Hi David & Stuart,

Can you please help to review the fix for the bug 7147060
, Thanks you!

The test reports ClassNotFoundException which is caused by below reasons:
1. Class can be found in "test.classes" instead of "test.src".
2. In agentvm mode, the MyTransform.class is loaded by the child of the context
ClassLoader of current thread, however, The method Transform.register(String,
String) tries to use context ClassLoader to load class MyTransform.class which
is not found.

The proposed fix is:
1. Replace "test.src" to "test.classes".
2. Replace the current thread ClassLoader to its child.

Regards,
Eric

hg: jdk8/tl/jdk: 7181353: Update error message to distinguish native OOM and java OOM in net

[luchsh]

Changeset: 15a6b0bceb1e
Author:zhouyx
Date:  2012-07-06 10:36 +0800
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/15a6b0bceb1e

7181353: Update error message to distinguish native OOM and java OOM in net
Reviewed-by: chegar

! src/solaris/native/java/net/Inet4AddressImpl.c
! src/solaris/native/java/net/Inet6AddressImpl.c
! src/solaris/native/java/net/NetworkInterface.c
! src/solaris/native/java/net/PlainDatagramSocketImpl.c
! src/windows/native/java/net/DualStackPlainDatagramSocketImpl.c
! src/windows/native/java/net/Inet6AddressImpl.c
! src/windows/native/java/net/NetworkInterface.c
! src/windows/native/java/net/TwoStacksPlainDatagramSocketImpl.c

Re: [PATCH] Review Request - Test bug: 6948101 java/rmi/transport/pinLastArguments/PinLastArguments.java failing intermittently

[Eric Wang]

Hi Marks,

Thanks for your great help to review and push my first fix.:-)

Regards,
Eric
On 2012/7/6 6:19, Stuart Marks wrote:

Pushed:

http://hg.openjdk.java.net/jdk8/tl/jdk/rev/97eb7a4b1fdd

s'marks

On 7/3/12 6:54 PM, Eric Wang wrote:
Opps, It is my carelessness, I will be more careful in the next bug 
fixes.

Thank you to review and change it.

Regards,
Eric

On 2012/7/4 8:58, Stuart Marks wrote:

Sure, I can take care of that.

If this is the only change, no need to generate another webrev, Eric.

s'marks


On 7/3/12 4:53 PM, David Holmes wrote:

Please fix the missing space in

if(ref.get()

before pushing.

Thanks,
David

On 4/07/2012 7:04 AM, Stuart Marks wrote:

Webrev now posted at

http://cr.openjdk.java.net/~smarks/yiming.wang/6948101/webrev.2/

The changes look good to me. If there's no further discussion, 
I'll push

them in a couple days (U.S. holiday coming up).

s'marks

On 7/3/12 12:20 AM, Stuart Marks wrote:
This approach looks good to me. Please go ahead and update 
7123972 as

well, and
I'll post these webrevs tomorrow (my time).

Thanks.

s'marks

On 7/2/12 11:41 PM, Eric Wang wrote:

David,

I have added the comments before the loop, please help to 
review. If

it is OK,
I'll update the 7123972 as well.

Thanks,
Eric

On 2012/7/3 12:22, David Holmes wrote:

Hi Eric,

On 3/07/2012 1:28 PM, Eric Wang wrote:

Hi Stuart and David,

Thanks for the suggestion about the timeout. so there are three
ways to
handle the timeout.

1. Add comment to explain that test failed due to default timeout
2. Specify the the timeout option
3. Timeout programly

Is it ok for you that i'd like to choose the second way "explict
timeout" as the test looks simply and lucid?


If by #2 you mean add something like:

@run main/timeout=10

then no. Earlier this year there were changes to a bunch of tests
that set
timeouts shorter than the jtreg default, to delete the timeouts.
Given that,
my misgivings about relying on the harness are not relevant so we
need only
do #1 and add a comment to make it clear that a failing test 
will be

indicated via the harness timeout.

David
-


Regards,
Eric

On 2012/7/3 10:38, David Holmes wrote:

On 3/07/2012 7:08 AM, Stuart Marks wrote:

On 7/1/12 5:39 PM, David Holmes wrote:
Now, how can the test fail? If ref is never cleared, the 
while

loop
will
never terminate, and we rely on jtreg to timeout and kill 
this

test. It
would be good to have a brief comment above the while loop 
that

explains
this.


Agreed. Something like

// Might require multiple calls to System.gc() for 
weak-references

processing
to be complete.
// If the weak-reference is not cleared as expected we will 
hang

here
// until timed out by the test harness

Though now I write that I'm unhappy that this will only behave
nicely
if run
from jtreg. We do use explicit timeouts in other tests.


So, are you unhappy enough that we should ask Eric to add an
explicit
timeout?


No. But I'm not the authoritive voice in this area :)


I don't think it would be that difficult. Perhaps a technique
like the
following would suffice. Get System.currentTimeMillis() before
the loop,
and call it again each time through the loop, and fail if the
difference
is greater than the timeout (e.g., 60 sec). Doesn't involve 
other

threads or even Timer/TimerTask.


Or simply limit the number of loops so that N*sleepTime = 
timeout.
This then requires adding back the check for null outside the 
loop.


On the other hand if one is running this test directly 
instead of
through jtreg, one can just ^C it if it's taking an 
unexpectedly

long
time.


True.

David
-



s'marks