Re: FilePermission Canonical path optimization

[deven you]

It sounds to me like  we need additional means to protect the file
permission without canocialization. I am looking forward to seeing the new
proposal with appropriate solution for problem Peter raised.

Thanks a lot!

2015-02-09 14:50 GMT+08:00 Wang Weijun :

>
> > On Feb 9, 2015, at 14:42, Peter Levart  wrote:
> >
> > Hi Max,
> >
> > Of course you are aware that by trusting the symlinks, you potentially
> give much more permission than you would hope to. Suppose that some code
> has permission to read and write into a particular directory (for temporary
> files). With this permission the code can actually read and/or write any
> file in the filesystem that OS grants access to the java process. Merely by
> creating a symlink in the read/write-able directory and accessing the file
> through it. That's why Apache HTTP Server by default disables
> "FollowSymLinks" option.
>
> Yes, we will be careful.
>
> In Java, a LinkPermission is needed to create a link. Of course, there
> might be other (existing) symlinks created by other non-Java processes. We
> will evaluate this possibility.
>
> Thanks
> Max
>
>

Re: FilePermission Canonical path optimization

[deven you]

Hi Weijun,

>From my understanding, the new proposal will let implies method only
depends on the absolute path in policy file, correct? So it's user's
responsibility to ensure files who want to access is relative to the
absolute path in some policy file?

I personal agree this proposal. Is there any doc or link for this new
proposal? Or if you can update the information for this proposal here, I
will be very appreciate!

Thanks a lot!

2015-02-09 11:51 GMT+08:00 Wang Weijun :

>
> > On Feb 9, 2015, at 11:22, deven you  wrote:
> >
> > Hi Weijun,
> >
> > I see JDK-4141872 marked as Not an Issue, is there any further task
> continue, or there is any link else to track this problem to remove the
> canonical path?
>
> It was marked as Not an Issue, but we are reconsidering about it.
>
> >
> > It's a big improvement if canonical path can be totally removed but I
> can't figure out how we get the result of the implies* methods without
> canonical path? Any more detail?
>
> The current proposed idea is that if you want to access a file using
> absolute path, you should add a FilePermission line in the policy file with
> an absolute path. If relative, relative. The overall idea is that the
> implies method should be implemented without consulting the actual file
> system but only by looking at the names themselves.
>
> That's why I said there is a very big incompatible change. We hope people
> only needs to modify their policy files and do not need to rewrite their
> apps, but we are still investigating if this can always be true.
>
> Thanks
> Max
>
> >
> > Thanks a lot!
>
>

Re: FilePermission Canonical path optimization

[deven you]

Hi Weijun,

I see JDK-4141872 <https://bugs.openjdk.java.net/browse/JDK-4141872> marked
as Not an Issue, is there any further task continue, or there is any link
else to track this problem to remove the canonical path?

It's a big improvement if canonical path can be totally removed but I can't
figure out how we get the result of the implies* methods without canonical
path? Any more detail?

Thanks a lot!


2015-02-07 7:10 GMT+08:00 Wang Weijun :

> Hi Deven
>
> Sorry for the noise, but in fact we are looking into removing the
> canonicalization step because of
>
>  4141872: FilePermission makes symlinks useless
>  https://bugs.openjdk.java.net/browse/JDK-4141872
>
> This will be a very big incompatible change and we are still doing a
> feasibility study.
>
> Of course, we won't touch jdk8u or earlier.
>
> Thanks
> Max
>
> > On Feb 6, 2015, at 14:09, deven you  wrote:
> >
> > Hi All,
> >
> > I have updated the patch[1] according to above discussion. Please review
> it.
> >
> > Thanks a lot
> >
> > [1] http://cr.openjdk.java.net/~youdwei/ojdk-912/webrev.03/
> >
>
>

Re: FilePermission Canonical path optimization

[deven you]

Hi All,

I have updated the patch[1] according to above discussion. Please review it.

Thanks a lot

[1] http://cr.openjdk.java.net/~youdwei/ojdk-912/webrev.03/


2015-02-03 16:04 GMT+08:00 Peter Levart :

> Hi Deven,
>
>
> On 02/03/2015 08:42 AM, deven you wrote:
>
>> Hi Sean,
>>
>> The performance degradation was reported by creating an object with always
>> getting its canonical path and there is no degradation heard after we made
>> the lazy load  patch for the canonical path.
>>
>> I have asked related people to give me an env to create this problem so
>> that I can take a close look how the application uses FilePermissions and
>> may report my analysis later.
>>
>> However, as far as I know at present, lazy loading the canonical path
>> should be a relative better solution:
>>
>> 1. fast set up time
>> 2. at run time, only necessary, the canonical path will be retrieved, the
>> best case is no canonical path used at all and the worst case is only take
>> almost the same effort as loading it at start up time.
>> 3. According to FilePermissionCollection, this is also true, the implies
>> method won't need to iterator the whole set of permissions, it will return
>> as soon as a proper permission found.
>>
>> Therefore, from general situation, I think this patch makes sense.
>>
>> To Peter,
>>
>> Even with your approach to change 'cpath' 'directory' and 'recursive' to
>> volatile and prepare their orders so that "directory" and "recursive"
>> first, "cpath" last, there is still some problem in the pattern in
>> equals()
>> and impliesIgnoreMask():
>>
>> if (cpath == null) initCpathDirectoryAndRecursive();
>> ... use 'cpath' or ''directory' or 'recursive' ...
>>
>> cpath could be null but initCpathDirectoryAndRecursive may be already
>> running in another thread therefore initCpathDirectoryAndRecursive might
>> be
>> invoked twice.
>>
>
> Good catch! Usually such redundant idempotent initialization is harmless -
> but it *must* be idempotent. Since it involves canonical path computation
> on the filesystem and the filesystem is a mutable "object", this is not
> guaranteed.
>
>
>> To solve this problem, based on your volatile solution, but still make
>> initCpathDirectoryAndRecursive as synchronized method and add below
>> statement at the beginning of this method:
>> if (cpath != null) {
>>  return;
>> }
>>
>> Even if there is another thread running, initCpathDirecotryAndRecursive()
>> will wait the completion of first thread and check cpath value and then
>> return. This solution ensures the initiating logic is run just once.
>>
>
> Right, double-checked locking (with use of volatiles and correct order of
> initialization) would be the best solution here.
>
> Regards, Peter
>
>
>
>> Thanks a lot!
>>
>>
>> Thanks a lot!
>>
>> 2014-12-18 2:35 GMT+08:00 Sean Mullan :
>>
>>  Hi,
>>>
>>> Can you elaborate more on the performance degradation that you are seeing
>>> at startup? Are you seeing this when you are running with or without a
>>> SecurityManager? If without a SecurityManager, can you provide some code
>>> paths/examples? As far as I can see, with the proposed fix you are moving
>>> the performance hit to runtime, and it will be triggered the first time a
>>> FilePermission check is made, and at worst case it may need to
>>> canonicalize
>>> all the FilePermissions in the FilePermissionCollection. Also, with the
>>> latest proposed fix you are potentially making performance worse by
>>> introducing synchronized blocks (which as Peter noted, still have some
>>> race
>>> conditions). I can understand why you want to improve application
>>> startup,
>>> but I want to make sure I understand the use case(s) a little better
>>> first.
>>>
>>> Thanks,
>>> Sean
>>>
>>
>

Re: FilePermission Canonical path optimization

[deven you]

Hi Sean,

The performance degradation was reported by creating an object with always
getting its canonical path and there is no degradation heard after we made
the lazy load  patch for the canonical path.

I have asked related people to give me an env to create this problem so
that I can take a close look how the application uses FilePermissions and
may report my analysis later.

However, as far as I know at present, lazy loading the canonical path
should be a relative better solution:

1. fast set up time
2. at run time, only necessary, the canonical path will be retrieved, the
best case is no canonical path used at all and the worst case is only take
almost the same effort as loading it at start up time.
3. According to FilePermissionCollection, this is also true, the implies
method won't need to iterator the whole set of permissions, it will return
as soon as a proper permission found.

Therefore, from general situation, I think this patch makes sense.

To Peter,

Even with your approach to change 'cpath' 'directory' and 'recursive' to
volatile and prepare their orders so that "directory" and "recursive"
first, "cpath" last, there is still some problem in the pattern in equals()
and impliesIgnoreMask():

if (cpath == null) initCpathDirectoryAndRecursive();
... use 'cpath' or ''directory' or 'recursive' ...

cpath could be null but initCpathDirectoryAndRecursive may be already
running in another thread therefore initCpathDirectoryAndRecursive might be
invoked twice.

To solve this problem, based on your volatile solution, but still make
initCpathDirectoryAndRecursive as synchronized method and add below
statement at the beginning of this method:
if (cpath != null) {
return;
}

Even if there is another thread running, initCpathDirecotryAndRecursive()
will wait the completion of first thread and check cpath value and then
return. This solution ensures the initiating logic is run just once.

Thanks a lot!


Thanks a lot!

2014-12-18 2:35 GMT+08:00 Sean Mullan :

> Hi,
>
> Can you elaborate more on the performance degradation that you are seeing
> at startup? Are you seeing this when you are running with or without a
> SecurityManager? If without a SecurityManager, can you provide some code
> paths/examples? As far as I can see, with the proposed fix you are moving
> the performance hit to runtime, and it will be triggered the first time a
> FilePermission check is made, and at worst case it may need to canonicalize
> all the FilePermissions in the FilePermissionCollection. Also, with the
> latest proposed fix you are potentially making performance worse by
> introducing synchronized blocks (which as Peter noted, still have some race
> conditions). I can understand why you want to improve application startup,
> but I want to make sure I understand the use case(s) a little better first.
>
> Thanks,
> Sean

Re: FilePermission Canonical path optimization

[deven you]

Hi All,

I have updated the patch[3] to reflect all of your suggestions.

[3] http://cr.openjdk.java.net/~youdwei/ojdk-912/webrev.02/

Thanks a lot!

2014-12-05 10:39 GMT+08:00 deven you :

> Hi Alan,
>
> I am not clear whether canonicalization cache enabled or disabled, however
> I think even the cache is enable, it may not help much for the start up
> stage especially if the app uses many different files.
>
> The cache should speed up applications for common case especially after
> start up and this patch will specifically solve the problem for
> FilePermssion.
>
> I will update the patch soon with your suggestion and Wenjian's.
>
> Thanks a lot!
>
>
>
> 2014-12-01 20:50 GMT+08:00 Alan Bateman :
>
>> On 01/12/2014 08:06, deven you wrote:
>>
>>> Hi All,
>>>   File.getCanonicalPath() is a very time-consuming method, we observed
>>> significant performance degradation from some application's startup stage
>>> with java.io.FilePermission. However, lazying load the calls to
>>> getCanonicalPath() from java.ioFilePermission is straightforward and
>>> solve
>>> this problem effectively. Openjdk bug[1]  tracks this bug and here is the
>>> patch [2]. Could anyone take a look?
>>>
>>> [1] https://bugs.openjdk.java.net/browse/JDK-8066211
>>> [2] http://cr.openjdk.java.net/~youdwei/ojdk-912/webrev.00/
>>>
>>
>> Do you run with canonicalization cache enabled or disabled? While I don't
>> agree with this cache, it seem to mostly eliminate complaints in this area
>> after it was added (in JDK 1.4 I think).
>>
>> As regards the patch then it is a bit ugly. Could you use cpath == null
>> instead of introducing a flag? If a flag is required then I think it will
>> need a better name. Also if init is split then it might be cleaner to have
>> initMask and initCanonicalPath.
>>
>> -Alan
>>
>>
>>
>

Re: FilePermission Canonical path optimization

[deven you]

Hi Bernd,


I will update the patch for the underscores. As to security manager I think
in most cases, FilePermission is used with it together.

>From the spec:

Absolute path:is complete in that no other information is required in
order to locate the file that it denotes

Canonical Path: is both absolute and unique.
The precise definition of canonical form is system-dependent. This method
first converts this pathname to absolute form if necessary, as if by
invoking the getAbsolutePath()
<http://docs.oracle.com/javase/8/docs/api/java/io/File.html#getAbsolutePath-->
 method, and then maps it to its unique form in a system-dependent way.
This typically involves removing redundant names such as "."and ".." from
the pathname, resolving symbolic links (on UNIX platforms), and converting
drive letters to a standard case (on Microsoft Windows platforms).

>From above, we can only use canonical path rather than absolute path
because absolute path may not be unique.

Thanks a lot!


2014-12-01 16:43 GMT+08:00 Bernd :

> Hello,
>
> I thik the underscores in method and field do not match very the other
> names in that file (or the JCL).
>
> If I understand right, this is an optimization for the case a security
> manager is not present? For the other case, maybe having absolute but not
> canonical file names is an option? Or maybe have a special optimization for
> JAVA_HOME prefixed names? Or does it help in that case as well?
>
> Gruss
> Bernd
> Am 01.12.2014 09:18 schrieb "deven you" :
>
> > Hi All,
> >  File.getCanonicalPath() is a very time-consuming method, we observed
> > significant performance degradation from some application's startup stage
> > with java.io.FilePermission. However, lazying load the calls to
> > getCanonicalPath() from java.ioFilePermission is straightforward and
> solve
> > this problem effectively. Openjdk bug[1]  tracks this bug and here is the
> > patch [2]. Could anyone take a look?
> >
> > [1] https://bugs.openjdk.java.net/browse/JDK-8066211
> > [2] http://cr.openjdk.java.net/~youdwei/ojdk-912/webrev.00/
> >
>

Re: FilePermission Canonical path optimization

[deven you]

Hi Weijun,

The original init() methods invoked by FilePermission constructors and
readObject() for deserialization. The constructors will be invoked only
once for each FilePermission Object and the ObjectInputStream will create
new FilePermission for each deserialization so the old init() is fine
without synchronization. This patch however requires there is only one
thread invokes the get_dir_rec() method to construct the cpath so it's true
this method need syncrhonization. I will update the patch later accordingly.

Thanks a lot!


2014-12-01 16:36 GMT+08:00 Wang Weijun :

> Do you need some kind of synchronization on the get_dir_rec() method?
>
> --Max
>
> > On Dec 1, 2014, at 16:06, deven you  wrote:
> >
> > Hi All,
> > File.getCanonicalPath() is a very time-consuming method, we observed
> > significant performance degradation from some application's startup stage
> > with java.io.FilePermission. However, lazying load the calls to
> > getCanonicalPath() from java.ioFilePermission is straightforward and
> solve
> > this problem effectively. Openjdk bug[1]  tracks this bug and here is the
> > patch [2]. Could anyone take a look?
> >
> > [1] https://bugs.openjdk.java.net/browse/JDK-8066211
> > [2] http://cr.openjdk.java.net/~youdwei/ojdk-912/webrev.00/
>
>

Re: FilePermission Canonical path optimization

[deven you]

Hi Alan,

I am not clear whether canonicalization cache enabled or disabled, however
I think even the cache is enable, it may not help much for the start up
stage especially if the app uses many different files.

The cache should speed up applications for common case especially after
start up and this patch will specifically solve the problem for
FilePermssion.

I will update the patch soon with your suggestion and Wenjian's.

Thanks a lot!



2014-12-01 20:50 GMT+08:00 Alan Bateman :

> On 01/12/2014 08:06, deven you wrote:
>
>> Hi All,
>>   File.getCanonicalPath() is a very time-consuming method, we observed
>> significant performance degradation from some application's startup stage
>> with java.io.FilePermission. However, lazying load the calls to
>> getCanonicalPath() from java.ioFilePermission is straightforward and solve
>> this problem effectively. Openjdk bug[1]  tracks this bug and here is the
>> patch [2]. Could anyone take a look?
>>
>> [1] https://bugs.openjdk.java.net/browse/JDK-8066211
>> [2] http://cr.openjdk.java.net/~youdwei/ojdk-912/webrev.00/
>>
>
> Do you run with canonicalization cache enabled or disabled? While I don't
> agree with this cache, it seem to mostly eliminate complaints in this area
> after it was added (in JDK 1.4 I think).
>
> As regards the patch then it is a bit ugly. Could you use cpath == null
> instead of introducing a flag? If a flag is required then I think it will
> need a better name. Also if init is split then it might be cleaner to have
> initMask and initCanonicalPath.
>
> -Alan
>
>
>

Better NewIO2 file system implementation for AIX platform

[deven you]

Hi All,

Our current NIO2 file system support to AIX is very limited, hence I took
some time to try to complete it. Openjdk bug[1] tracks this bug and here is
my patch[2] which enhances the AIX file system especially by adding the
support by Implementing AixDosFileAttributeView.java and
AixUserDefinedFileAttributeView.java.

Could anyone take a look?


[1] https://bugs.openjdk.java.net/browse/JDK-8066212
[2] http://cr.openjdk.java.net/~youdwei/ojdk-909/webrev.00/

Thanks a lot!

FilePermission Canonical path optimization

[deven you]

Hi All,
 File.getCanonicalPath() is a very time-consuming method, we observed
significant performance degradation from some application's startup stage
with java.io.FilePermission. However, lazying load the calls to
getCanonicalPath() from java.ioFilePermission is straightforward and solve
this problem effectively. Openjdk bug[1]  tracks this bug and here is the
patch [2]. Could anyone take a look?

[1] https://bugs.openjdk.java.net/browse/JDK-8066211
[2] http://cr.openjdk.java.net/~youdwei/ojdk-912/webrev.00/

A question about Depth of container annotations

[deven you]

Hi All,

I have a question about repeated annotation depth. If this is not the
proper mailing list group, please tell me where I should send it to.

My question will be about the depth of container annotations. For instance,
assume there are 3 annotations.
- RepeatedAnn
- ContainerAnn
- ContainerContainerAnn

So, ContainerContainerAnn can have ContainerAnn and that can also have
RepeatbleAnn in it.

In this case, get*AnnotationsByType(RepeatableAnn) APIs wont return
repeteableAnns in depth 2.

Java docs don't talk about the depth. I wonder if the get*AnnotationsByType
api should return the annotations of all depth?

If we have below annotations:
@Retention(RetentionPolicy.RUNTIME)
@Repeatable(ContainerAnn.class)
@interface RepeatableAnn { String value(); }

@Inherited
@Retention(RetentionPolicy.RUNTIME)
@Repeatable(ContainerContainerAnn.class)
@interface ContainerAnn { RepeatableAnn[] value(); }

@Inherited
@Retention(RetentionPolicy.RUNTIME)
@interface ContainerContainerAnn { ContainerAnn[] value(); }

And the main class is annotated by :

@ContainerAnn({@RepeatableAnn("Parent-3")})
@ContainerAnn({@RepeatableAnn("Parent-4")})
public class Test { ...

when we call getAnnotationsByType(RepeatableAnn.class) on this class, we
get nothing because RepeatableAnn is contained by ContainerAnn which is
also contained by ContainerContainerAnn. In other words, RepeatableAnn is
in depth 2 and get*AnnotationsByType APIs only searches depth 1. The test
results are:

/home/deven/jdk8_20/jdk1.8.0_20/bin$ java repeated.Test
CLASS = repeated.Test
getDeclaredAnnotationsByType(RepeatableAnn.class)
getDeclaredAnnotationsByType(ContainerAnn.class)

@repeated.ContainerAnn(value=[@repeated.RepeatableAnn(value=Parent-3)])

@repeated.ContainerAnn(value=[@repeated.RepeatableAnn(value=Parent-4)])
getDeclaredAnnotationsByType(ContainerContainerAnn.class)

@repeated.ContainerContainerAnn(value=[@repeated.ContainerAnn(value=[@repeated.RepeatableAnn(value=Parent-3)]),
@repeated.ContainerAnn(value=[@repeated.RepeatableAnn(value=Parent-4)])])
getAnnotationsByType(RepeatableAnn.class)
getAnnotationsByType(ContainerAnn.class)

@repeated.ContainerAnn(value=[@repeated.RepeatableAnn(value=Parent-3)])

@repeated.ContainerAnn(value=[@repeated.RepeatableAnn(value=Parent-4)])
getAnnotationsByType(ContainerContainerAnn.class)

@repeated.ContainerContainerAnn(value=[@repeated.ContainerAnn(value=[@repeated.RepeatableAnn(value=Parent-3)]),
@repeated.ContainerAnn(value=[@repeated.RepeatableAnn(value=Parent-4)])])

Re: Non Inherited repeated annotations should not be searched from child Class

[Deven You]

Hi All,

I didn't see this mail in the mailing list for a long time, so I just 
comment here to ensure everyone can receive this message.


Thanks a lot!
On 06/04/2013 09:37 PM, Deven You wrote:


Hi All,

I have written a test case[1] to show this problem. (If it is 
confirmed a real bug, I will convert this test case to jtreg)


My expected output is no any output but OpenJDK returns:

@NonInheritedAnnotationRepeated(name=A)
@NonInheritedAnnotationRepeated(name=B)

The reasons are:

1. From the spec, Inherited means:

Indicates that an annotation type is automatically inherited. If an 
Inherited meta-annotation is present on an annotation type 
declaration, and the user queries the annotation type on a class 
declaration, and the class declaration has no annotation for this 
type, then the class's superclass will automatically be queried for 
the annotation type. This process will be repeated until an annotation 
for this type is found, or the top of the class hierarchy (Object) is 
reached. If no superclass has an annotation for this type, then the 
query will indicate that the class in question has no such annotation.


2. For repeated annotations, according to the explanation of 1., If it 
is non-inherited, querying Child class of this annotation should 
return null.


3. Now the problem is if the repeated annotation is non-inherited, but 
its container annotation is inherited, OpenJDK will return the 
repeated annotations of Parent class if we query the Child class.


It seems according to the Inherited semantics, even when container 
annotation is inherited, we should not retrieve parent class 
non-inherited repeated annotation from a child class.


[1] http://cr.openjdk.java.net/~youdwei/ojdk-810/NonInheritedTest/ 
<http://cr.openjdk.java.net/%7Eyoudwei/ojdk-810/NonInheritedTest/> 

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

[Deven You]

Hi Lance,

I am glad to see the patch for implementing these methods are committed.

Do you have a plan to add the test cases[1] I created too?

Thanks a lot!

[1]http://cr.openjdk.java.net/~youdwei/OJDK-576/webrev.01/


On 01/10/2013 08:31 PM, Lance Andersen - Oracle wrote:

Deven,

This was pushed a while ago you should have seen the putback on the alias

See http://hg.openjdk.java.net/jdk8/tl/jdk/rev/7a8978a5bb6e

Best
Lance


On Jan 10, 2013, at 12:49 AM, Deven You wrote:


Hi Lance,

Is there any update for this issue. If you have anything I can do, 
please let me know.


Thanks a lot!
On 11/24/2012 12:45 AM, Lance Andersen - Oracle wrote:
It is on my list.  to update the javadocs I need a ccc which I have 
not done yet and is needed as part of this change


On Nov 23, 2012, at 3:07 AM, Deven You wrote:


Hi Lance,

Is there any plan for this issue, if any could you update to me?

Thanks a lot!
On 10/29/2012 06:39 PM, Lance Andersen - Oracle wrote:

Hi Deven,

I will address the needed updates a bit later.

Thank you for your input

Best
Lance
On Oct 29, 2012, at 3:51 AM, Deven You wrote:


Hi Alan,

The Java Spec does not mention the thread safe for JDBC API. But I see the 
other code in SerialBlob/SerialClob have not consider it.

I think use buff == null to replace isFree is a good idea because it also avoid the 
problem for the condition buf == null && isFree == false so we won't need 
create a readObject method.

Thanks for your suggestion for isFree, I will correct it later.

Lance: How about your suggestion? Since you mentioned you will develop the 
implementation yourself. I use my implementation mainly for the test cases. But 
you may also take a look my implementation.

Thanks a lot!

On 09/21/2012 04:37 PM, Alan Bateman wrote:

On 21/09/2012 04:21, Deven You wrote:

Hi Lance,

I am very busy with other work so I can't work with the SerialBlob/SerialClob 
item for long time. I am very happy to refine the current test case and create 
new tests for SerialClob.

I have create a new webre[1] for this task, please review it.

[1]http://cr.openjdk.java.net/~youdwei/OJDK-576/webrev.01/  
<http://cr.openjdk.java.net/%7Eyoudwei/OJDK-576/webrev.01/>

PS: If the isFree is not transient, I want to know how we add this field to the 
javadoc serialized form?

I don't know very much about the rowset API and I can't see anything to specify 
whether it is meant to be safe for use by concurrent threads. There are clearly 
lots of issues here and implementing free introduces a lot more, especially 
with the possibility of an asynchronous free or more than one thread calling 
free at around the same time.

Have you considered "buf == null" to mean that the resources are freed? That might 
avoid needing to change the serialized form. Also as these types are serializable it means you 
have to consider the case where you deserialize to buf == null && isFree == false for 
example. On that point, it looks to me that this code needs a readObject anyway (for several 
reasons).

A small point is that "isFree" is a odd name for a method that doesn't return a 
boolean. If the patch goes ahead then I think it needs a better name, ensureNotFree or 
requireNotFree or something like that.

-Alan.




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





<http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance 
Andersen| Principal Member of Technical Staff | +1.781.442.2037

Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>





<http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance 
Andersen| Principal Member of Technical Staff | +1.781.442.2037

Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>

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

[Deven You]

Hi Lance,

Is there any update for this issue. If you have anything I can do, 
please let me know.


Thanks a lot!
On 11/24/2012 12:45 AM, Lance Andersen - Oracle wrote:
It is on my list.  to update the javadocs I need a ccc which I have 
not done yet and is needed as part of this change


On Nov 23, 2012, at 3:07 AM, Deven You wrote:


Hi Lance,

Is there any plan for this issue, if any could you update to me?

Thanks a lot!
On 10/29/2012 06:39 PM, Lance Andersen - Oracle wrote:

Hi Deven,

I will address the needed updates a bit later.

Thank you for your input

Best
Lance
On Oct 29, 2012, at 3:51 AM, Deven You wrote:


Hi Alan,

The Java Spec does not mention the thread safe for JDBC API. But I see the 
other code in SerialBlob/SerialClob have not consider it.

I think use buff == null to replace isFree is a good idea because it also avoid the 
problem for the condition buf == null && isFree == false so we won't need 
create a readObject method.

Thanks for your suggestion for isFree, I will correct it later.

Lance: How about your suggestion? Since you mentioned you will develop the 
implementation yourself. I use my implementation mainly for the test cases. But 
you may also take a look my implementation.

Thanks a lot!

On 09/21/2012 04:37 PM, Alan Bateman wrote:

On 21/09/2012 04:21, Deven You wrote:

Hi Lance,

I am very busy with other work so I can't work with the SerialBlob/SerialClob 
item for long time. I am very happy to refine the current test case and create 
new tests for SerialClob.

I have create a new webre[1] for this task, please review it.

[1]http://cr.openjdk.java.net/~youdwei/OJDK-576/webrev.01/  
<http://cr.openjdk.java.net/%7Eyoudwei/OJDK-576/webrev.01/>

PS: If the isFree is not transient, I want to know how we add this field to the 
javadoc serialized form?

I don't know very much about the rowset API and I can't see anything to specify 
whether it is meant to be safe for use by concurrent threads. There are clearly 
lots of issues here and implementing free introduces a lot more, especially 
with the possibility of an asynchronous free or more than one thread calling 
free at around the same time.

Have you considered "buf == null" to mean that the resources are freed? That might 
avoid needing to change the serialized form. Also as these types are serializable it means you 
have to consider the case where you deserialize to buf == null && isFree == false for 
example. On that point, it looks to me that this code needs a readObject anyway (for several 
reasons).

A small point is that "isFree" is a odd name for a method that doesn't return a 
boolean. If the patch goes ahead then I think it needs a better name, ensureNotFree or 
requireNotFree or something like that.

-Alan.




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





<http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance 
Andersen| Principal Member of Technical Staff | +1.781.442.2037

Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>

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

[Deven You]

Hi Lance,

Is there any plan for this issue, if any could you update to me?

Thanks a lot!
On 10/29/2012 06:39 PM, Lance Andersen - Oracle wrote:

Hi Deven,

I will address the needed updates a bit later.

Thank you for your input

Best
Lance
On Oct 29, 2012, at 3:51 AM, Deven You wrote:


Hi Alan,

The Java Spec does not mention the thread safe for JDBC API. But I see the 
other code in SerialBlob/SerialClob have not consider it.

I think use buff == null to replace isFree is a good idea because it also avoid the 
problem for the condition buf == null && isFree == false so we won't need 
create a readObject method.

Thanks for your suggestion for isFree, I will correct it later.

Lance: How about your suggestion? Since you mentioned you will develop the 
implementation yourself. I use my implementation mainly for the test cases. But 
you may also take a look my implementation.

Thanks a lot!

On 09/21/2012 04:37 PM, Alan Bateman wrote:

On 21/09/2012 04:21, Deven You wrote:

Hi Lance,

I am very busy with other work so I can't work with the SerialBlob/SerialClob 
item for long time. I am very happy to refine the current test case and create 
new tests for SerialClob.

I have create a new webre[1] for this task, please review it.

[1] http://cr.openjdk.java.net/~youdwei/OJDK-576/webrev.01/ 
<http://cr.openjdk.java.net/%7Eyoudwei/OJDK-576/webrev.01/>

PS: If the isFree is not transient, I want to know how we add this field to the 
javadoc serialized form?

I don't know very much about the rowset API and I can't see anything to specify 
whether it is meant to be safe for use by concurrent threads. There are clearly 
lots of issues here and implementing free introduces a lot more, especially 
with the possibility of an asynchronous free or more than one thread calling 
free at around the same time.

Have you considered "buf == null" to mean that the resources are freed? That might 
avoid needing to change the serialized form. Also as these types are serializable it means you 
have to consider the case where you deserialize to buf == null && isFree == false for 
example. On that point, it looks to me that this code needs a readObject anyway (for several 
reasons).

A small point is that "isFree" is a odd name for a method that doesn't return a 
boolean. If the patch goes ahead then I think it needs a better name, ensureNotFree or 
requireNotFree or something like that.

-Alan.




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

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

[Deven You]

Hi Alan,

The Java Spec does not mention the thread safe for JDBC API. But I see 
the other code in SerialBlob/SerialClob have not consider it.


I think use buff == null to replace isFree is a good idea because it 
also avoid the problem for the condition buf == null && isFree == false 
so we won't need create a readObject method.


Thanks for your suggestion for isFree, I will correct it later.

Lance: How about your suggestion? Since you mentioned you will develop 
the implementation yourself. I use my implementation mainly for the test 
cases. But you may also take a look my implementation.


Thanks a lot!

On 09/21/2012 04:37 PM, Alan Bateman wrote:

On 21/09/2012 04:21, Deven You wrote:

Hi Lance,

I am very busy with other work so I can't work with the 
SerialBlob/SerialClob item for long time. I am very happy to refine 
the current test case and create new tests for SerialClob.


I have create a new webre[1] for this task, please review it.

[1] http://cr.openjdk.java.net/~youdwei/OJDK-576/webrev.01/ 
<http://cr.openjdk.java.net/%7Eyoudwei/OJDK-576/webrev.01/>


PS: If the isFree is not transient, I want to know how we add this 
field to the javadoc serialized form?
I don't know very much about the rowset API and I can't see anything 
to specify whether it is meant to be safe for use by concurrent 
threads. There are clearly lots of issues here and implementing free 
introduces a lot more, especially with the possibility of an 
asynchronous free or more than one thread calling free at around the 
same time.


Have you considered "buf == null" to mean that the resources are 
freed? That might avoid needing to change the serialized form. Also as 
these types are serializable it means you have to consider the case 
where you deserialize to buf == null && isFree == false for example. 
On that point, it looks to me that this code needs a readObject anyway 
(for several reasons).


A small point is that "isFree" is a odd name for a method that doesn't 
return a boolean. If the patch goes ahead then I think it needs a 
better name, ensureNotFree or requireNotFree or something like that.


-Alan.

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

[Deven You]

Hi Lance,

I am very busy with other work so I can't work with the 
SerialBlob/SerialClob item for long time. I am very happy to refine the 
current test case and create new tests for SerialClob.


I have create a new webre[1] for this task, please review it.

[1] http://cr.openjdk.java.net/~youdwei/OJDK-576/webrev.01/ 
<http://cr.openjdk.java.net/%7Eyoudwei/OJDK-576/webrev.01/>


PS: If the isFree is not transient, I want to know how we add this field 
to the javadoc serialized form?


Thanks a lot!

On 07/06/2012 04:46 AM, Lance Andersen - Oracle wrote:

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 
<http://cr.openjdk.java.net/%7Elittlee/OJDK-576/webrev>


Thanks a lot.




--
Best Regards,

Deven



<http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance 
Andersen| Principal Member of Technical Staff | +1.781.442.2037

Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>




--
Best Regards,

Deven


<http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance 
Andersen| Principal Member of Technical Staff | +1.781.442.2037

Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>

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

[Deven You]

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 
<http://cr.openjdk.java.net/%7Elittlee/OJDK-576/webrev>


Thanks a lot.




--
Best Regards,

Deven



<http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance 
Andersen| Principal Member of Technical Staff | +1.781.442.2037

Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>




--
Best Regards,

Deven

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

[Deven You]

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

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

[Deven You]

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: hg: jdk8/tl/jdk: 7094176: (tz) Incorrect TimeZone display name when DST not applicable / disabled

[Deven You]

Hi Alan,

Thanks for your reminder, I was going to point that this a manual test 
case and need someone guide me how to configure it as manual with jtreg. 
But I missed it by chance.


So Masayoshi or someone else, I'd very appreciate if you could guide me 
how to configure a test case as manual in the jtreg?


Thanks a lot!

On 05/25/2012 03:27 PM, Alan Bateman wrote:

Deven,

The test with this fix seems to be a manual test but doesn't seem to 
be tagged as such (/manual). I assume this means it will fail on all 
platforms. Do you mind re-examining it? Masayoshi may have some advice 
on how configuration like this can be tested. It may be that it's just 
not possible to test this in an automated way, in which case the path 
of least pain may be to just remove it.


-Alan.

On 25/05/2012 06:31, litt...@linux.vnet.ibm.com wrote:

Changeset: 71cf74329a9e
Author:youdwei
Date:  2012-05-25 13:28 +0800
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/71cf74329a9e

7094176: (tz) Incorrect TimeZone display name when DST not applicable 
/ disabled

Reviewed-by: okutsu

! src/windows/native/java/util/TimeZone_md.c
+ test/java/util/TimeZone/DstTzTest.java






--
Best Regards,

Deven

Re: sun.management.Agent: the properties.putAll API may fail with ConcurrentModifcationException on multi-thread scenario

[Deven You]

Hi Dmitry,

I have verified the change set. Thanks for your commit!

On 05/17/2012 03:44 PM, Dmitry Samersoff wrote:

Deven,

The fix is submitted to openjdk  tl workspace

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

-Dmitry


On 2012-04-26 05:21, Deven You wrote:

Hi Dmitry,

Thanks for your help. I have created a CR with internal id 2236492 which
hasn't be published yet. So please set this internal CR id as duplicate
to 716419 as well.

This is the original mail for this problem:



---

Hi core-libs-devs,

I am not sure if sun.management.Agent belongs to jmx-dev mailing list,
if so please anyone tell me.

This issue is that the sun.management.Agent.loadManagementProperties()
will invoke properties.putAll which will throw
ConcurrentModifcationException if there are other threads which modify
the properties concurrently.

I have made a patch[1] which synchronize the sysProps so that putAll can
work on multi-thread scenario. The test case is also available in [1].

Thanks a lot!

[1] http://cr.openjdk.java.net/~littlee/OJDK-256/webrev.00
<http://cr.openjdk.java.net/%7Elittlee/OJDK-256/webrev.00>

--


Thanks a lot!



On 04/25/2012 08:32 PM, Dmitry Samersoff wrote:

Deven,

CR number is  7164191 .

Could you re-send me your original e-mail with problem description and
webrev link.

   I'll put it to CR comment field.


-Dmitry



On 2012-04-24 16:15, Dmitry Samersoff wrote:

Deven,

After close look and off-line discussion with David Holmes,
the changes looks good for me.

I'll take care of the rest.

We have one more place in Agent.java executing exactly the same code
so I'll change both of them on your behalf.

-Dmitry


On 2012-04-23 11:43, Deven You wrote:

Thanks David,

So is it ok for you to contribute this patch?

On 04/23/2012 02:36 PM, David Holmes wrote:

Except of course that Properties is a Hashtable and synchronizes on
'this' for all public methods. So locking the properties object in the
client code will guarantee exclusive access to it.

Sorry about that.

David
-

On 23/04/2012 4:30 PM, David Holmes wrote:

Deven,

On 23/04/2012 3:54 PM, Deven You wrote:

On 04/18/2012 02:20 PM, Deven You wrote:

On 04/18/2012 01:34 PM, Mandy Chung wrote:

I think this could still run into CME. System Properties is not a
synchronized map and the setter methods (System.setProperty or
Properties.put method) doesn't synchronize on the Properties
object.


The setter methods I'm referring to are System.setProperty and
System.getProperties().put().


I have gone through the Agent.java, I think other set/put methods
related to properties are protected properly.

public static void agentmain using parseString(args) which return a
properties which is a local var and is not possible to cause
concurrent problem when call config_props.putAll(arg_props).

private static synchronized void startLocalManagementAgent() is
synchronized already.

private static synchronized void startRemoteManagementAgent(String
args) is synchronized also.

Could you point where the CME may ocurr?

Is there any suggestion from the mailing list?

The problem is that System.getProperties() returns a globally
accessible
set of properties. So even if you prevent the Agent code from
modifying
those properties concurrently with other use in the Agent, you
have no
such guard for any other piece of code in the system which might also
modify the properties. So the race condition you were trying to fix
still exists. I don't see any way to fix this. No matter what you do
another thread can modify the system properties while you are
iterating
them. Instead you need to anticipate the CME and try to recover
from it
(also non-trivial).

Cheers,
David Holmes







--
Best Regards,

Deven

Pass a pointer to JNI_GetCreatedJavaVMs() instead of null

[Deven You]

Hi All,

There is a potential problem in 
jdk/src/share/native/com/sun/java/util/jar/pack/jni.cpp.


(Maybe it is not suitable for posting this on core-lib, anyone could 
tell me which mailing list is prefer?)


L85:

JNI_GetCreatedJavaVMs(&vm, 1, null) in which the 3rd parameter is a 
pointer to an integer. See[1], the latest JNI Invocation API spec does 
not say anything about allowing a null as the last parameter.


I think it is more reasonable to change null to an integer variable. 
Here is my fix[2]


[1] 
http://docs.oracle.com/javase/7/docs/technotes/guides/jni/spec/invocation.html#wp633


[2] http://cr.openjdk.java.net/~littlee/ojdk-432/webrev.00/ 




Please review this mail!

Thanks a lot!

--
Best Regards,

Deven

Re: sun.management.Agent: the properties.putAll API may fail with ConcurrentModifcationException on multi-thread scenario

[Deven You]

Hi Dmitry,

Thanks for your help. I have created a CR with internal id 2236492 which 
hasn't be published yet. So please set this internal CR id as duplicate 
to 716419 as well.


This is the original mail for this problem:



---
Hi core-libs-devs,

I am not sure if sun.management.Agent belongs to jmx-dev mailing list, 
if so please anyone tell me.


This issue is that the sun.management.Agent.loadManagementProperties() 
will invoke properties.putAll which will throw 
ConcurrentModifcationException if there are other threads which modify 
the properties concurrently.


I have made a patch[1] which synchronize the sysProps so that putAll can 
work on multi-thread scenario. The test case is also available in [1].


Thanks a lot!

[1] http://cr.openjdk.java.net/~littlee/OJDK-256/webrev.00 
<http://cr.openjdk.java.net/%7Elittlee/OJDK-256/webrev.00>


--

Thanks a lot!



On 04/25/2012 08:32 PM, Dmitry Samersoff wrote:

Deven,

CR number is  7164191 .

Could you re-send me your original e-mail with problem description and
webrev link.

  I'll put it to CR comment field.


-Dmitry



On 2012-04-24 16:15, Dmitry Samersoff wrote:

Deven,

After close look and off-line discussion with David Holmes,
the changes looks good for me.

I'll take care of the rest.

We have one more place in Agent.java executing exactly the same code
so I'll change both of them on your behalf.

-Dmitry


On 2012-04-23 11:43, Deven You wrote:

Thanks David,

So is it ok for you to contribute this patch?

On 04/23/2012 02:36 PM, David Holmes wrote:

Except of course that Properties is a Hashtable and synchronizes on
'this' for all public methods. So locking the properties object in the
client code will guarantee exclusive access to it.

Sorry about that.

David
-

On 23/04/2012 4:30 PM, David Holmes wrote:

Deven,

On 23/04/2012 3:54 PM, Deven You wrote:

On 04/18/2012 02:20 PM, Deven You wrote:

On 04/18/2012 01:34 PM, Mandy Chung wrote:


I think this could still run into CME. System Properties is not a
synchronized map and the setter methods (System.setProperty or
Properties.put method) doesn't synchronize on the Properties object.


The setter methods I'm referring to are System.setProperty and
System.getProperties().put().


I have gone through the Agent.java, I think other set/put methods
related to properties are protected properly.

public static void agentmain using parseString(args) which return a
properties which is a local var and is not possible to cause
concurrent problem when call config_props.putAll(arg_props).

private static synchronized void startLocalManagementAgent() is
synchronized already.

private static synchronized void startRemoteManagementAgent(String
args) is synchronized also.

Could you point where the CME may ocurr?

Is there any suggestion from the mailing list?

The problem is that System.getProperties() returns a globally
accessible
set of properties. So even if you prevent the Agent code from modifying
those properties concurrently with other use in the Agent, you have no
such guard for any other piece of code in the system which might also
modify the properties. So the race condition you were trying to fix
still exists. I don't see any way to fix this. No matter what you do
another thread can modify the system properties while you are iterating
them. Instead you need to anticipate the CME and try to recover from it
(also non-trivial).

Cheers,
David Holmes









--
Best Regards,

Deven

Re: sun.management.Agent: the properties.putAll API may fail with ConcurrentModifcationException on multi-thread scenario

[Deven You]

Thanks David,

So is it ok for you to contribute this patch?

On 04/23/2012 02:36 PM, David Holmes wrote:
Except of course that Properties is a Hashtable and synchronizes on 
'this' for all public methods. So locking the properties object in the 
client code will guarantee exclusive access to it.


Sorry about that.

David
-

On 23/04/2012 4:30 PM, David Holmes wrote:

Deven,

On 23/04/2012 3:54 PM, Deven You wrote:

On 04/18/2012 02:20 PM, Deven You wrote:

On 04/18/2012 01:34 PM, Mandy Chung wrote:



I think this could still run into CME. System Properties is not a
synchronized map and the setter methods (System.setProperty or
Properties.put method) doesn't synchronize on the Properties object.


The setter methods I'm referring to are System.setProperty and
System.getProperties().put().



I have gone through the Agent.java, I think other set/put methods
related to properties are protected properly.

public static void agentmain using parseString(args) which return a
properties which is a local var and is not possible to cause
concurrent problem when call config_props.putAll(arg_props).

private static synchronized void startLocalManagementAgent() is
synchronized already.

private static synchronized void startRemoteManagementAgent(String
args) is synchronized also.

Could you point where the CME may ocurr?


Is there any suggestion from the mailing list?


The problem is that System.getProperties() returns a globally accessible
set of properties. So even if you prevent the Agent code from modifying
those properties concurrently with other use in the Agent, you have no
such guard for any other piece of code in the system which might also
modify the properties. So the race condition you were trying to fix
still exists. I don't see any way to fix this. No matter what you do
another thread can modify the system properties while you are iterating
them. Instead you need to anticipate the CME and try to recover from it
(also non-trivial).

Cheers,
David Holmes





--
Best Regards,

Deven

Re: sun.management.Agent: the properties.putAll API may fail with ConcurrentModifcationException on multi-thread scenario

[Deven You]

On 04/18/2012 02:20 PM, Deven You wrote:

On 04/18/2012 01:34 PM, Mandy Chung wrote:



On 4/17/2012 12:33 AM, Deven You wrote:
I think this could still run into CME.  System Properties is not a 
synchronized map and the setter methods (System.setProperty or 
Properties.put method) doesn't synchronize on the Properties object.



Hi Mandy,

I didn't catch you. Do you mean there are other setter methods of 
System properties in the Agent.java which are not synchronized?


The setter methods I'm referring to are System.setProperty and 
System.getProperties().put().


Mandy


Hi Mandy,

I have gone through the Agent.java, I think other set/put methods 
related to properties are protected properly.


public static void agentmain using parseString(args) which return 
a properties which is a local var and is not possible to cause 
concurrent problem when call config_props.putAll(arg_props).


private static synchronized void startLocalManagementAgent() is 
synchronized already.


private static synchronized void startRemoteManagementAgent(String 
args) is synchronized also.


Could you point where the CME may ocurr?

Thanks a lot!



Hi All,

Is there any suggestion from the mailing list?

--
Best Regards,

Deven

Re: sun.management.Agent: the properties.putAll API may fail with ConcurrentModifcationException on multi-thread scenario

[Deven You]

On 04/18/2012 01:34 PM, Mandy Chung wrote:



On 4/17/2012 12:33 AM, Deven You wrote:
I think this could still run into CME.  System Properties is not a 
synchronized map and the setter methods (System.setProperty or 
Properties.put method) doesn't synchronize on the Properties object.



Hi Mandy,

I didn't catch you. Do you mean there are other setter methods of 
System properties in the Agent.java which are not synchronized?


The setter methods I'm referring to are System.setProperty and 
System.getProperties().put().


Mandy


Hi Mandy,

I have gone through the Agent.java, I think other set/put methods 
related to properties are protected properly.


public static void agentmain using parseString(args) which return a 
properties which is a local var and is not possible to cause concurrent 
problem when call config_props.putAll(arg_props).


private static synchronized void startLocalManagementAgent() is 
synchronized already.


private static synchronized void startRemoteManagementAgent(String 
args) is synchronized also.


Could you point where the CME may ocurr?

Thanks a lot!


--
Best Regards,

Deven

Re: sun.management.Agent: the properties.putAll API may fail with ConcurrentModifcationException on multi-thread scenario

[Deven You]

On 04/12/2012 10:09 AM, Mandy Chung wrote:

On 4/11/2012 12:36 AM, Deven You wrote:

Hi core-libs-devs,

I am not sure if sun.management.Agent belongs to jmx-dev mailing 
list, if so please anyone tell me.




serviceability-dev (cc'ed) is the mailing list for this patch.

This issue is that the 
sun.management.Agent.loadManagementProperties() will invoke 
properties.putAll which will throw ConcurrentModifcationException if 
there are other threads which modify the properties concurrently.


I have made a patch[1] which synchronize the sysProps so that putAll 
can work on multi-thread scenario. The test case is also available in 
[1].




I think this could still run into CME.  System Properties is not a 
synchronized map and the setter methods (System.setProperty or 
Properties.put method) doesn't synchronize on the Properties object.


Mandy


Thanks a lot!

[1] http://cr.openjdk.java.net/~littlee/OJDK-256/webrev.00 
<http://cr.openjdk.java.net/%7Elittlee/OJDK-256/webrev.00>





Hi Mandy,

I didn't catch you. Do you mean there are other setter methods of System 
properties in the Agent.java which are not synchronized?


Thanks a lot!

--
Best Regards,

Deven

Re: New constructor in sun.tools.java.ClassPath builds a path using File.separator instead of File.pathSeparatorChar

[Deven You]

Hi Alan,

I choose the test/sun/tools/classpath/ RMICClassPathTest.java because 
the fix is made in sun.tools.java.ClassPath.  However the test case uses 
BatchEnvironment.createClassPath to create a new ClassPath. I think 
test/sun/tools/classpath/ is more closer to sun.tools.java.ClassPath.


On 04/12/2012 04:26 PM, Alan Bateman wrote:

On 12/04/2012 08:22, Deven You wrote:


I have verified this commit. Thanks Alan and Charles!
The change-set that Charles pushed added test/sun/tools/classpath/ 
RMICClassPathTest.java but I didn't see it in the original webrev. I 
see now that your original mail had the proposed test in a separate 
webrev but I missed that. Anyway, one comment is that test has been 
added to an odd location that doesn't correspond to where the fix was 
made. I think test/sun/rmi/rmic would have been a more suitable location.


-Alan.



--
Best Regards,

Deven

Re: New constructor in sun.tools.java.ClassPath builds a path using File.separator instead of File.pathSeparatorChar

[Deven You]

I have verified this commit. Thanks Alan and Charles!

On 04/12/2012 03:12 PM, Charles Lee wrote:

On 04/11/2012 03:44 PM, Deven You wrote:

On 04/10/2012 04:12 PM, Alan Bateman wrote:

On 09/04/2012 10:32, Deven You wrote:

:

The issue is reported by one of our test team which maintains  a 
set of ORB test cases.
Okay but I guess I'm still a bit curious as it's not obvious that 
rmic (or rmic --iiop) invokes toString, maybe this is a white box 
test or a test that is using sun.tools.java.*?


-Alan.


Hi Alan,

Though I am not very sure, I guess it is a white box test. I find 
some code snippet which may cause this errors, like:


sun.misc.URLClassPath.pathToURLs(classPath.toString()));

I think if anyone use above like code, it will fail.

Thanks a lot!


Hi Deven,

The patch is committed @

Changeset: c98a013ec628
Author:youdwei
Date:  2012-04-12 15:04 +0800
URL:http://hg.openjdk.java.net/jdk8/tl/jdk/rev/c98a013ec628

6610897: New constructor in sun.tools.java.ClassPath builds a path 
using File.separator instead of File.pathSeparator

Reviewed-by: alanb

! src/share/classes/sun/tools/java/ClassPath.java
+ test/sun/tools/classpath/RMICClassPathTest.java



Please verify it.




--
Best Regards,

Deven

Re: New constructor in sun.tools.java.ClassPath builds a path using File.separator instead of File.pathSeparatorChar

[Deven You]

On 04/10/2012 04:12 PM, Alan Bateman wrote:

On 09/04/2012 10:32, Deven You wrote:

:

The issue is reported by one of our test team which maintains  a set 
of ORB test cases.
Okay but I guess I'm still a bit curious as it's not obvious that rmic 
(or rmic --iiop) invokes toString, maybe this is a white box test or a 
test that is using sun.tools.java.*?


-Alan.


Hi Alan,

Though I am not very sure, I guess it is a white box test. I find some 
code snippet which may cause this errors, like:


sun.misc.URLClassPath.pathToURLs(classPath.toString()));

I think if anyone use above like code, it will fail.

Thanks a lot!

--
Best Regards,

Deven

sun.management.Agent: the properties.putAll API may fail with ConcurrentModifcationException on multi-thread scenario

[Deven You]

Hi core-libs-devs,

I am not sure if sun.management.Agent belongs to jmx-dev mailing list, 
if so please anyone tell me.


This issue is that the sun.management.Agent.loadManagementProperties() 
will invoke properties.putAll which will throw 
ConcurrentModifcationException if there are other threads which modify 
the properties concurrently.


I have made a patch[1] which synchronize the sysProps so that putAll can 
work on multi-thread scenario. The test case is also available in [1].


Thanks a lot!

[1] http://cr.openjdk.java.net/~littlee/OJDK-256/webrev.00 



--
Best Regards,

Deven

Re: New constructor in sun.tools.java.ClassPath builds a path using File.separator instead of File.pathSeparatorChar

[Deven You]

On 04/09/2012 05:03 PM, Alan Bateman wrote:

On 09/04/2012 08:01, Deven You wrote:

Hi core-libs-devs,

There are 2 sun bugs related to this issue:

http://bugs.sun.com/view_bug.do;jsessionid=663548580556ae43f50bf92c75b5?bug_id=6695325^and 

http://bugs.sun.com/view_bug.do;jsessionid=663548580556ae43f50bf92c75b5?bug_id=6610897^ 



The basic problem is that the new constructor

public ClassPath(String[] patharray)  which calls new private method

private void init(String}[] patharray)

The init method builds a pathstr using File.separator instead of 
File.pathSeparatorChar.


Since it is really a bug and the fix is simple, I have made a 
patch[1] for this issue and a test case[2] to recreated this issue.


The patch just uses File.pathSeparatorChar to replace File.separator.

Please anyone can review it?
No issue with the proposed change but I'm curious how you ran into 
this. Is it with rmic or something else?


-Alan


Hi Alan,

The issue is reported by one of our test team which maintains  a set of 
ORB test cases.


Thanks a lot!

--
Best Regards,

Deven

New constructor in sun.tools.java.ClassPath builds a path using File.separator instead of File.pathSeparatorChar

[Deven You]

Hi core-libs-devs,

There are 2 sun bugs related to this issue:

http://bugs.sun.com/view_bug.do;jsessionid=663548580556ae43f50bf92c75b5?bug_id=6695325^and
http://bugs.sun.com/view_bug.do;jsessionid=663548580556ae43f50bf92c75b5?bug_id=6610897^

The basic problem is that the new constructor

public ClassPath(String[] patharray)  which calls new private method

private void init(String}[] patharray)

The init method builds a pathstr using File.separator instead of 
File.pathSeparatorChar.


Since it is really a bug and the fix is simple, I have made a patch[1] 
for this issue and a test case[2] to recreated this issue.


The patch just uses File.pathSeparatorChar to replace File.separator.

Please anyone can review it?

Thanks a lot!

[1] http://cr.openjdk.java.net/~youdwei/classpath/webrev.00/ 

[2] http://cr.openjdk.java.net/~youdwei/classpath/test/webrev.00/ 



--
Best Regards,

Deven