Re: RFR 9: 8034903: Add Logging of Process.start arguments and resulting pid

[Florian Weimer]

On 02/13/2014 10:26 PM, roger riggs wrote:


Having folks stumbling over process creation and problems of quoting,
especially on windows, it seems useful to log the native commands and
arguments.
They are proposed to be logged using the PlatformLogger at Level.FINE
which will not be logged by default. The environment is useful in some
cases,
but verbose, that it should be Logged at FINER.  The pid of the spawned
process
logged as well for traceability.


It may make sense to expose something like getProcessID() to the public 
because it is independently useful on systems with /proc.


In the src/solaris version, Arrays::toString(Object[]) does not quote 
the arguments, so the argument boundary is lost in the logging.  I think 
something that performs quoting (using the Java language rules, not 
shell) would be helpful in this context.  Although on non-Windows 
platforms, this information can easily be obtained with tools such as 
strace or truss.


--
Florian Weimer / Red Hat Product Security Team

Re: RFR 9: 8034903: Add Logging of Process.start arguments and resulting pid

[Alan Bateman]

On 14/02/2014 10:26, Florian Weimer wrote:


It may make sense to expose something like getProcessID() to the 
public because it is independently useful on systems with /proc.
Yes, this has been asked for many times. We have JEP 102 [1] that lists 
this and other Process topics that we'd like to get funded for JDK 9.


-Alan

[1] http://openjdk.java.net/jeps/102

Re: RFR 9: 8034903: Add Logging of Process.start arguments and resulting pid

[Daniel Fuchs]

Hi Roger,

I agree with Paul that using Objects.toString(environment) would be
cleaner.

Concerning the test, you may want to modify CheckHandler.publish to
filter out any log record that doesn't come from your ProcessImpl
classes. Something like:

 175 @Override
 176 public void publish(LogRecord lr) {
 // Only look at the records emitted by the
 // "java.lang.Process" logger
 if ("java.lang.Process".equals(lr.getLoggerName()) {
 177logRec = lr;
 }
 178 }

Also, since you're changing the Level of the root logger, you may
want to either run your test in /othervm mode, or make sure that it
puts back the root logger level to its original value and also
removes the CheckHandler() instance from the root handlers with a try {
} finally { } to avoid polluting the other tests that will follow.

best regards,

-- daniel

On 2/13/14 10:26 PM, roger riggs wrote:

Hi,

Having folks stumbling over process creation and problems of quoting,
especially on windows, it seems useful to log the native commands and
arguments.
They are proposed to be logged using the PlatformLogger at Level.FINE
which will not be logged by default. The environment is useful in some
cases,
but verbose, that it should be Logged at FINER.  The pid of the spawned
process
logged as well for traceability.

Please take a look at this first draft and comment on whether it is useful,
a good idea and any improvements needed.

Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-log-processcreate/

Thanks, Roger

Re: (8034912) Approval request : RFR: (8031737) CHECK_NULL and CHECK_EXCEPTION macros cleanup

[Seán Coffey]

Approved for pushing to jdk8u-dev.

regards,
Sean.

On 14/02/14 01:38, Phil Race wrote:
Yeah sorry about that. It built on Windows and Mac but not Linux as i 
found out today.


-phil.

On 2/13/14 5:36 PM, Mandy Chung wrote:
Looks good.   Sorry I didn't catch this earlier and I was counting on 
the test build :)


Mandy
[1] http://hg.openjdk.java.net/jdk9/dev/jdk/rev/c58c6b0fbe34

On 2/13/2014 1:50 PM, Phil Race wrote:
That worked on Mac but I just found it doesn't build on Linux 
because a macro-redefinition

 warning is treated as an error there.

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

The early/original fix had removed the duplicate definition in
src/share/native/java/net/net_util.h

So the fix is to do the same in 8u :-

~/jdk8u-dev/jdk$ hg diff src/share/native/java/net/net_util.h
diff --git a/src/share/native/java/net/net_util.h 
b/src/share/native/java/net/net_util.h

--- a/src/share/native/java/net/net_util.h
+++ b/src/share/native/java/net/net_util.h
@@ -42,9 +42,6 @@
 #define NET_ERROR(env, ex, msg) \
 { if (!(*env)->ExceptionOccurred(env)) JNU_ThrowByName(env, ex, msg) }

-#define CHECK_NULL(x) if ((x) == NULL) return;
-#define CHECK_NULL_RETURN(x, y) if ((x) == NULL) return y;
-
 / 


  * Cached field IDs
  *

A little surprised no else else found this already  (did they?)
Anyway I need a review and Ok to push 8034912  to JDk 8 u

-Phil.

On 02/12/2014 10:21 AM, Mike Duigou wrote:

This looks fine.


On Feb 11 2014, at 15:42 , Phil Race  wrote:


Here's a JDk8u webrev : -http://cr.openjdk.java.net/~prr/8031737.8u/

-phil.

On 2/11/14 2:28 PM, Phil Race wrote:

So since hg export/import doesn't apply cleanly and the dependency
chain seems, long and in order to have some consistency across 
the releases,
I think I should prepare a webrev which essentially backports 
8031737

including its small changes to Version.c, if only because otherwise
I'd have to have a new bug ID that would not be forwarded ported
(one source of confusion) or even worse re-use 8031737 but not 
fully implement it


Agreed ?

-phil.

Why isEmpty(), retainAll() and containsAll() not supported with default implementations in JDK8?

[Roman Leventov]

I'm sure there was a discussion somewhere in JDK mailing lists, but I couldn't 
find. Please, give me a link.

I've tried to ask on SO: 
http://stackoverflow.com/questions/21758081/why-many-methods-in-jcf-interfaces-not-made-default-in-java-8,
 proved that JDK developers don't read SO :)

Re: Why isEmpty(), retainAll() and containsAll() not supported with default implementations in JDK8?

[Paul Sandoz]

On Feb 14, 2014, at 2:13 PM, Roman Leventov  wrote:
> I'm sure there was a discussion somewhere in JDK mailing lists, but I 
> couldn't find. Please, give me a link.
> 

I cannot recall such discussion.

Generally we have only converted existing abstract methods on an interface to 
non-abstract if there was a compelling reason to aid implementations, such as 
Iterator.remove.

These are not new methods on Collection, and there are already implementations 
in AbstractCollection. The advantage of converting these abstract into 
non-abstract methods its not particularly compelling given one is most likely 
to inherit from AbstractCollection or provide more efficient implementations.

It would be possible to move all non-abstract methods on AbstractCollection to 
Collection. If we were starting from a blank sheet of paper that is what we 
might have done. (Note one cannot do this with all non-abstract methods on 
AbstractList.)


> I've tried to ask on SO: 
> http://stackoverflow.com/questions/21758081/why-many-methods-in-jcf-interfaces-not-made-default-in-java-8,
>  proved that JDK developers don't read SO :)


I don't :-) i know others do, and Stuart replied:

  http://stackoverflow.com/a/21774137

Paul.

Re: A hole in the serialization spec

[David M. Lloyd]

On 02/13/2014 11:38 AM, David M. Lloyd wrote:

On 02/13/2014 10:29 AM, Chris Hegarty wrote:

On 12 Feb 2014, at 15:24, David M. Lloyd 
wrote:


That's a quote from the serialization spec.  I take it to mean,
"Don't write fields and everything might go to hell".  In practice,
if the reading side doesn't read fields, things end up more or less
OK, as evidenced by various classes in the wild.  But it's not hard
to imagine a scenario in which a class change could cause protocol
corruption.

I think the specifics of the quote relate to this kind of class
change; in particular, if a class is deleted from the hierarchy on
the read side, and that class corresponds to the class that had the
misbehaving writeObject, I suspect that things will break at that
point as the read side will probably try to consume and discard the
field information for that class, which will be missing (it will
start reading the next class' fields instead I think).


Yes, possibly. And who knows what fields/values may be read and
mistaken for the wrong object in the hierarchy. So ‘undefined'
behaviour seems right to me.


I think the behavior is well-defined, just "bad", which is my point.  If
the exact current is spec'd out as-is then at least we can be assured of
the same bad behavior across implementations.  If the behavior is
changed such that fields are read/written but discarded, without
updating the spec, then the "undefined" behavior at least becomes safer.
  If the behavior is changed, *and* the spec is updated, then we get
both benefits, but at the cost that all previous implementations will
not be compliant with the spec.

All options seem to have a cost though.


In the JDK, java.util.Date does not read/write fields.  Perhaps others 
as well.  Given that the behavior is presently undefined, that means the 
serialized representation of java.util.Date (and any other such 
non-conforming classes) are also undefined.


--
- DML

Re: RFR 9: 8034903: Add Logging of Process.start arguments and resulting pid

[roger riggs]

Thanks Paul, Daniel,

Thanks for the suggestions,  I updated the webrev.

/othervm does seem to be needed to prevent this test from interfering or
being interfered wit.

Roger


On 2/14/2014 6:06 AM, Daniel Fuchs wrote:

Hi Roger,

I agree with Paul that using Objects.toString(environment) would be
cleaner.

Concerning the test, you may want to modify CheckHandler.publish to
filter out any log record that doesn't come from your ProcessImpl
classes. Something like:

 175 @Override
 176 public void publish(LogRecord lr) {
 // Only look at the records emitted by the
 // "java.lang.Process" logger
 if ("java.lang.Process".equals(lr.getLoggerName()) {
 177logRec = lr;
 }
 178 }

Also, since you're changing the Level of the root logger, you may
want to either run your test in /othervm mode, or make sure that it
puts back the root logger level to its original value and also
removes the CheckHandler() instance from the root handlers with a try {
} finally { } to avoid polluting the other tests that will follow.

best regards,

-- daniel

On 2/13/14 10:26 PM, roger riggs wrote:

Hi,

Having folks stumbling over process creation and problems of quoting,
especially on windows, it seems useful to log the native commands and
arguments.
They are proposed to be logged using the PlatformLogger at Level.FINE
which will not be logged by default. The environment is useful in some
cases,
but verbose, that it should be Logged at FINER.  The pid of the spawned
process
logged as well for traceability.

Please take a look at this first draft and comment on whether it is 
useful,

a good idea and any improvements needed.

Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-log-processcreate/

Thanks, Roger

Re: RFR 9: 8034903: Add Logging of Process.start arguments and resulting pid

[roger riggs]

Hi Florian,

As for escaping, is there an existing API for encoding and decoding those?
A human won't have trouble figuring out the few cases where there might be
some ambiguity due to "," in the string.
The test will be more complicated as well and I think added that the 
extra complexity

is not a good investment.

Thanks, Roger


On 2/14/2014 5:26 AM, Florian Weimer wrote:

On 02/13/2014 10:26 PM, roger riggs wrote:


Having folks stumbling over process creation and problems of quoting,
especially on windows, it seems useful to log the native commands and
arguments.
They are proposed to be logged using the PlatformLogger at Level.FINE
which will not be logged by default. The environment is useful in some
cases,
but verbose, that it should be Logged at FINER.  The pid of the spawned
process
logged as well for traceability.


It may make sense to expose something like getProcessID() to the 
public because it is independently useful on systems with /proc.


In the src/solaris version, Arrays::toString(Object[]) does not quote 
the arguments, so the argument boundary is lost in the logging.  I 
think something that performs quoting (using the Java language rules, 
not shell) would be helpful in this context. Although on non-Windows 
platforms, this information can easily be obtained with tools such as 
strace or truss.

Re: A hole in the serialization spec

[roger riggs]

Hi David,

I would quibble that the serialized form of java.util.Date is defined.
The behavior is defined by the writeObject and readObject methods.
They write and read a time computed from and restored to the state of 
the object.


Roger

On 2/14/2014 10:56 AM, David M. Lloyd wrote:

On 02/13/2014 11:38 AM, David M. Lloyd wrote:

On 02/13/2014 10:29 AM, Chris Hegarty wrote:

On 12 Feb 2014, at 15:24, David M. Lloyd 
wrote:


That's a quote from the serialization spec.  I take it to mean,
"Don't write fields and everything might go to hell".  In practice,
if the reading side doesn't read fields, things end up more or less
OK, as evidenced by various classes in the wild.  But it's not hard
to imagine a scenario in which a class change could cause protocol
corruption.

I think the specifics of the quote relate to this kind of class
change; in particular, if a class is deleted from the hierarchy on
the read side, and that class corresponds to the class that had the
misbehaving writeObject, I suspect that things will break at that
point as the read side will probably try to consume and discard the
field information for that class, which will be missing (it will
start reading the next class' fields instead I think).


Yes, possibly. And who knows what fields/values may be read and
mistaken for the wrong object in the hierarchy. So ‘undefined'
behaviour seems right to me.


I think the behavior is well-defined, just "bad", which is my point.  If
the exact current is spec'd out as-is then at least we can be assured of
the same bad behavior across implementations.  If the behavior is
changed such that fields are read/written but discarded, without
updating the spec, then the "undefined" behavior at least becomes safer.
  If the behavior is changed, *and* the spec is updated, then we get
both benefits, but at the cost that all previous implementations will
not be compliant with the spec.

All options seem to have a cost though.


In the JDK, java.util.Date does not read/write fields.  Perhaps others 
as well.  Given that the behavior is presently undefined, that means 
the serialized representation of java.util.Date (and any other such 
non-conforming classes) are also undefined.

Re: A hole in the serialization spec

[Chris Hegarty]

On 14/02/14 15:56, David M. Lloyd wrote:

On 02/13/2014 11:38 AM, David M. Lloyd wrote:

On 02/13/2014 10:29 AM, Chris Hegarty wrote:

On 12 Feb 2014, at 15:24, David M. Lloyd 
wrote:


That's a quote from the serialization spec.  I take it to mean,
"Don't write fields and everything might go to hell".  In practice,
if the reading side doesn't read fields, things end up more or less
OK, as evidenced by various classes in the wild.  But it's not hard
to imagine a scenario in which a class change could cause protocol
corruption.

I think the specifics of the quote relate to this kind of class
change; in particular, if a class is deleted from the hierarchy on
the read side, and that class corresponds to the class that had the
misbehaving writeObject, I suspect that things will break at that
point as the read side will probably try to consume and discard the
field information for that class, which will be missing (it will
start reading the next class' fields instead I think).


Yes, possibly. And who knows what fields/values may be read and
mistaken for the wrong object in the hierarchy. So ‘undefined'
behaviour seems right to me.


I think the behavior is well-defined, just "bad", which is my point.  If
the exact current is spec'd out as-is then at least we can be assured of
the same bad behavior across implementations.  If the behavior is
changed such that fields are read/written but discarded, without
updating the spec, then the "undefined" behavior at least becomes safer.
  If the behavior is changed, *and* the spec is updated, then we get
both benefits, but at the cost that all previous implementations will
not be compliant with the spec.

All options seem to have a cost though.


In the JDK, java.util.Date does not read/write fields.  Perhaps others
as well.  Given that the behavior is presently undefined, that means the
serialized representation of java.util.Date (and any other such
non-conforming classes) are also undefined.


True. j.u.Date is playing with fire ( and it should be fixed ). But 
since it has no serializable state itself, it doesn't fall foul ( at 
least on Oracles JDK ) of any problems. If it were to evolve in the 
future it may run into trouble.


-Chris.

Re: A hole in the serialization spec

[Chris Hegarty]

On 14/02/14 16:31, roger riggs wrote:

Hi David,

I would quibble that the serialized form of java.util.Date is defined.
The behavior is defined by the writeObject and readObject methods.
They write and read a time computed from and restored to the state of
the object.


Yes, but it falls foul if the MUST invoke default read/write fields in 
the Serialization spec if it has a writeObect. Evolving this class in 
the future ( if it were to happen ) would be problematic. If it were to 
add serializable state, for example.


-Chris.



Roger

On 2/14/2014 10:56 AM, David M. Lloyd wrote:

On 02/13/2014 11:38 AM, David M. Lloyd wrote:

On 02/13/2014 10:29 AM, Chris Hegarty wrote:

On 12 Feb 2014, at 15:24, David M. Lloyd 
wrote:


That's a quote from the serialization spec.  I take it to mean,
"Don't write fields and everything might go to hell".  In practice,
if the reading side doesn't read fields, things end up more or less
OK, as evidenced by various classes in the wild.  But it's not hard
to imagine a scenario in which a class change could cause protocol
corruption.

I think the specifics of the quote relate to this kind of class
change; in particular, if a class is deleted from the hierarchy on
the read side, and that class corresponds to the class that had the
misbehaving writeObject, I suspect that things will break at that
point as the read side will probably try to consume and discard the
field information for that class, which will be missing (it will
start reading the next class' fields instead I think).


Yes, possibly. And who knows what fields/values may be read and
mistaken for the wrong object in the hierarchy. So ‘undefined'
behaviour seems right to me.


I think the behavior is well-defined, just "bad", which is my point.  If
the exact current is spec'd out as-is then at least we can be assured of
the same bad behavior across implementations.  If the behavior is
changed such that fields are read/written but discarded, without
updating the spec, then the "undefined" behavior at least becomes safer.
  If the behavior is changed, *and* the spec is updated, then we get
both benefits, but at the cost that all previous implementations will
not be compliant with the spec.

All options seem to have a cost though.


In the JDK, java.util.Date does not read/write fields.  Perhaps others
as well.  Given that the behavior is presently undefined, that means
the serialized representation of java.util.Date (and any other such
non-conforming classes) are also undefined.

Re: RFR (JAXP): 8033980 : Xerces Update: datatype XMLGregorianCalendarImpl and DurationImpl

[huizhe wang]

Hi All,

I added a SerializationTest. The test contains a helper that can 
generate serialization files for XMLGregorianCalendar and Duration. I've 
created such files for jdk6,7,8 and 9, and manually run the test, that 
is, read them back with JDK6, 7, 8 and 9. The test worked fine. In the 
JDK9(or 10 in the future) repo and for an auto-run, it would use the 
current JDK9/10 build and test against JDK6, 7, 8 and 9. Past JDK10, we 
could consider add serialization files for JDK10.


The new fields did not affect serialization compatibility. The above 
tests passed with/without the new fields being transient. But I added 
transient since it's the right thing to do.


Adding fields is a compatible change in accordance with Java Object 
Serialization Spec 
.


Thanks,
Joe

On 2/13/2014 6:23 AM, Alan Bateman wrote:

On 13/02/2014 08:18, huizhe wang wrote:

Hi Alan, Lance, and Daniel,

The Xerces serialization revision meant to create a serialization 
form that would help maintain future serialization compatibility. But 
in reality it itself is causing significant incompatibility as Alan 
pointed out below and we discussed previously. I've removed the 
revision from the patch as a result.


Please see the new webrev here:
http://cr.openjdk.java.net/~joehw/jdk9/8033980/webrev/
Thanks for dropping the serialization change as it was just not going 
to work the way you had intended.


I agree with Daniel's comment about all the new fields added to 
XMLGregorianCalendarImpl as it's not clear why they aren't transient.


I have not studied the rest of the changes but I think Daniel and 
Lance are.


-Alan

RFR: 8034853 remove sun.misc.ClassLoaderUtil

[Michael McMahon]

Could I get the following change reviewed please?

http://cr.openjdk.java.net/~michaelm/8034853/webrev.1/

The change is to remove the class sun.misc.ClassLoaderUtil.
The functionality provided by this class is now in the public
java.net.URLClassloader.close() method.

Thanks
Michael

Re: A hole in the serialization spec

[David M. Lloyd]

On 02/14/2014 09:56 AM, David M. Lloyd wrote:

On 02/13/2014 11:38 AM, David M. Lloyd wrote:

On 02/13/2014 10:29 AM, Chris Hegarty wrote:

On 12 Feb 2014, at 15:24, David M. Lloyd 
wrote:


That's a quote from the serialization spec.  I take it to mean,
"Don't write fields and everything might go to hell".  In practice,
if the reading side doesn't read fields, things end up more or less
OK, as evidenced by various classes in the wild.  But it's not hard
to imagine a scenario in which a class change could cause protocol
corruption.

I think the specifics of the quote relate to this kind of class
change; in particular, if a class is deleted from the hierarchy on
the read side, and that class corresponds to the class that had the
misbehaving writeObject, I suspect that things will break at that
point as the read side will probably try to consume and discard the
field information for that class, which will be missing (it will
start reading the next class' fields instead I think).


Yes, possibly. And who knows what fields/values may be read and
mistaken for the wrong object in the hierarchy. So ‘undefined'
behaviour seems right to me.


I think the behavior is well-defined, just "bad", which is my point.  If
the exact current is spec'd out as-is then at least we can be assured of
the same bad behavior across implementations.  If the behavior is
changed such that fields are read/written but discarded, without
updating the spec, then the "undefined" behavior at least becomes safer.
  If the behavior is changed, *and* the spec is updated, then we get
both benefits, but at the cost that all previous implementations will
not be compliant with the spec.

All options seem to have a cost though.


In the JDK, java.util.Date does not read/write fields.  Perhaps others
as well.  Given that the behavior is presently undefined, that means the
serialized representation of java.util.Date (and any other such
non-conforming classes) are also undefined.


An interesting detail here - since Date doesn't have any non-transient 
fields, this happens to work out OK for a second reason (that 
defaultReadFields() would read nothing anyway) - however it still would 
break if a non-transient field were to be added.


--
- DML

Re: A hole in the serialization spec

[David M. Lloyd]

Yes, however given that the lack of reading/writing fields makes it 
undefined *in general*, you can only say "the behavior is defined as 
long as this undefined behavior is actually defined in this (risky) manner".


Put another way, since the behavior of not reading/writing fields is not 
defined, this class is then by spec not portable between JVMs (after 
all, undefined behavior could mean anything at all).  That's why I was 
saying that the spec should define exactly what will physically happen 
if you do this (from both the perspective of the "other end" of the 
serialization API, *and* the wire protocol), even if it's considered 
poor practice - because then at least you have guaranteed 
interoperability (which becomes even more important with the knowledge 
that JDK classes have this issue).


On 02/14/2014 10:31 AM, roger riggs wrote:

Hi David,

I would quibble that the serialized form of java.util.Date is defined.
The behavior is defined by the writeObject and readObject methods.
They write and read a time computed from and restored to the state of
the object.

Roger

On 2/14/2014 10:56 AM, David M. Lloyd wrote:

On 02/13/2014 11:38 AM, David M. Lloyd wrote:

On 02/13/2014 10:29 AM, Chris Hegarty wrote:

On 12 Feb 2014, at 15:24, David M. Lloyd 
wrote:


That's a quote from the serialization spec.  I take it to mean,
"Don't write fields and everything might go to hell".  In practice,
if the reading side doesn't read fields, things end up more or less
OK, as evidenced by various classes in the wild.  But it's not hard
to imagine a scenario in which a class change could cause protocol
corruption.

I think the specifics of the quote relate to this kind of class
change; in particular, if a class is deleted from the hierarchy on
the read side, and that class corresponds to the class that had the
misbehaving writeObject, I suspect that things will break at that
point as the read side will probably try to consume and discard the
field information for that class, which will be missing (it will
start reading the next class' fields instead I think).


Yes, possibly. And who knows what fields/values may be read and
mistaken for the wrong object in the hierarchy. So ‘undefined'
behaviour seems right to me.


I think the behavior is well-defined, just "bad", which is my point.  If
the exact current is spec'd out as-is then at least we can be assured of
the same bad behavior across implementations.  If the behavior is
changed such that fields are read/written but discarded, without
updating the spec, then the "undefined" behavior at least becomes safer.
  If the behavior is changed, *and* the spec is updated, then we get
both benefits, but at the cost that all previous implementations will
not be compliant with the spec.

All options seem to have a cost though.


In the JDK, java.util.Date does not read/write fields.  Perhaps others
as well.  Given that the behavior is presently undefined, that means
the serialized representation of java.util.Date (and any other such
non-conforming classes) are also undefined.






--
- DML

Re: RFR: 8034853 remove sun.misc.ClassLoaderUtil

[Joe Darcy]

Hi Michael,

Good to see more of sun.misc go away :-)

For the test, I recommend updating the @summary and removing the comment 
about the old API.


Thanks,

-Joe

On 02/14/2014 09:42 AM, Michael McMahon wrote:

Could I get the following change reviewed please?

http://cr.openjdk.java.net/~michaelm/8034853/webrev.1/

The change is to remove the class sun.misc.ClassLoaderUtil.
The functionality provided by this class is now in the public
java.net.URLClassloader.close() method.

Thanks
Michael

Re: RFR: 8034853 remove sun.misc.ClassLoaderUtil

[Michael McMahon]

Thanks Joe. Will do that.

Michael

On 14/02/14 18:09, Joe Darcy wrote:

Hi Michael,

Good to see more of sun.misc go away :-)

For the test, I recommend updating the @summary and removing the 
comment about the old API.


Thanks,

-Joe

On 02/14/2014 09:42 AM, Michael McMahon wrote:

Could I get the following change reviewed please?

http://cr.openjdk.java.net/~michaelm/8034853/webrev.1/

The change is to remove the class sun.misc.ClassLoaderUtil.
The functionality provided by this class is now in the public
java.net.URLClassloader.close() method.

Thanks
Michael

Re: RFR: 8034853 remove sun.misc.ClassLoaderUtil

[Alan Bateman]

On 14/02/2014 17:42, Michael McMahon wrote:

Could I get the following change reviewed please?

http://cr.openjdk.java.net/~michaelm/8034853/webrev.1/

The change is to remove the class sun.misc.ClassLoaderUtil.
The functionality provided by this class is now in the public
java.net.URLClassloader.close() method.
The removal is good. Maybe the test should be moved to URLClassLoader as 
it doesn't make sense to have it in ClassLoaderUtil anymore.


-Alan.

Re: RFR: 8034853 remove sun.misc.ClassLoaderUtil

[Michael McMahon]

On 14/02/14 18:20, Alan Bateman wrote:

On 14/02/2014 17:42, Michael McMahon wrote:

Could I get the following change reviewed please?

http://cr.openjdk.java.net/~michaelm/8034853/webrev.1/

The change is to remove the class sun.misc.ClassLoaderUtil.
The functionality provided by this class is now in the public
java.net.URLClassloader.close() method.
The removal is good. Maybe the test should be moved to URLClassLoader 
as it doesn't make sense to have it in ClassLoaderUtil anymore.


-Alan.


I think I'd probably just delete the test since URLClassLoader.close has 
its own tests,

I thought it might be useful as a point of reference
in case there could be someone still using the sun.misc API.
Also, I think mercurial won't delete the empty directory after the file 
is gone - not that that matters very much.


Thanks
Michael

Re: A hole in the serialization spec

[David M. Lloyd]

Interestingly, if the behavior in this case was to write empty values on 
the write side and read and discard on the read side, both the behavior 
and the serialized format of java.util.Date would be identical to today; 
though other pathological classes might be incompatible, they were 
already in undefined-land.


On 02/14/2014 12:01 PM, David M. Lloyd wrote:

Yes, however given that the lack of reading/writing fields makes it
undefined *in general*, you can only say "the behavior is defined as
long as this undefined behavior is actually defined in this (risky)
manner".

Put another way, since the behavior of not reading/writing fields is not
defined, this class is then by spec not portable between JVMs (after
all, undefined behavior could mean anything at all).  That's why I was
saying that the spec should define exactly what will physically happen
if you do this (from both the perspective of the "other end" of the
serialization API, *and* the wire protocol), even if it's considered
poor practice - because then at least you have guaranteed
interoperability (which becomes even more important with the knowledge
that JDK classes have this issue).

On 02/14/2014 10:31 AM, roger riggs wrote:

Hi David,

I would quibble that the serialized form of java.util.Date is defined.
The behavior is defined by the writeObject and readObject methods.
They write and read a time computed from and restored to the state of
the object.

Roger

On 2/14/2014 10:56 AM, David M. Lloyd wrote:

On 02/13/2014 11:38 AM, David M. Lloyd wrote:

On 02/13/2014 10:29 AM, Chris Hegarty wrote:

On 12 Feb 2014, at 15:24, David M. Lloyd 
wrote:


That's a quote from the serialization spec.  I take it to mean,
"Don't write fields and everything might go to hell".  In practice,
if the reading side doesn't read fields, things end up more or less
OK, as evidenced by various classes in the wild.  But it's not hard
to imagine a scenario in which a class change could cause protocol
corruption.

I think the specifics of the quote relate to this kind of class
change; in particular, if a class is deleted from the hierarchy on
the read side, and that class corresponds to the class that had the
misbehaving writeObject, I suspect that things will break at that
point as the read side will probably try to consume and discard the
field information for that class, which will be missing (it will
start reading the next class' fields instead I think).


Yes, possibly. And who knows what fields/values may be read and
mistaken for the wrong object in the hierarchy. So ‘undefined'
behaviour seems right to me.


I think the behavior is well-defined, just "bad", which is my
point.  If
the exact current is spec'd out as-is then at least we can be
assured of
the same bad behavior across implementations.  If the behavior is
changed such that fields are read/written but discarded, without
updating the spec, then the "undefined" behavior at least becomes
safer.
  If the behavior is changed, *and* the spec is updated, then we get
both benefits, but at the cost that all previous implementations will
not be compliant with the spec.

All options seem to have a cost though.


In the JDK, java.util.Date does not read/write fields.  Perhaps others
as well.  Given that the behavior is presently undefined, that means
the serialized representation of java.util.Date (and any other such
non-conforming classes) are also undefined.









--
- DML

Re: RFR: 8034853 remove sun.misc.ClassLoaderUtil

[Chris Hegarty]

Looks good.

-Chris.

> On 14 Feb 2014, at 17:42, Michael McMahon  
> wrote:
> 
> Could I get the following change reviewed please?
> 
> http://cr.openjdk.java.net/~michaelm/8034853/webrev.1/
> 
> The change is to remove the class sun.misc.ClassLoaderUtil.
> The functionality provided by this class is now in the public
> java.net.URLClassloader.close() method.
> 
> Thanks
> Michael

RFR [4682009] Typo in javadocs in javax/naming

[Ivan Gerasimov]

Hello!

May I please have a review of the fix?

It's not meant to be a proof reading, I only fixed some obvious typos.
Some of them were reported at the time of java 1.4, so it is probably 
the time to have them fixed.


BUG: https://bugs.openjdk.java.net/browse/JDK-4682009
WEBREV: http://cr.openjdk.java.net/~igerasim/4682009/0/webrev/

Sincerely yours,
Ivan

Re: RFR [4682009] Typo in javadocs in javax/naming

[Lance Andersen - Oracle]

Looks fine.  assume the  will be addressed as part of a full sweep 
of javax/naming
On Feb 14, 2014, at 2:48 PM, Ivan Gerasimov wrote:

> Hello!
> 
> May I please have a review of the fix?
> 
> It's not meant to be a proof reading, I only fixed some obvious typos.
> Some of them were reported at the time of java 1.4, so it is probably the 
> time to have them fixed.
> 
> BUG: https://bugs.openjdk.java.net/browse/JDK-4682009
> WEBREV: http://cr.openjdk.java.net/~igerasim/4682009/0/webrev/
> 
> Sincerely yours,
> Ivan


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

Re: RFR 9: 8034903: Add Logging of Process.start arguments and resulting pid

[Alan Bateman]

On 14/02/2014 18:10, Martin Buchholz wrote:

I'm not excited about having logging statements added throughout the jdk
implementation.  PlatformLogger is an implementation detail, so real users
cannot benefit.  Your OS should already provide facilities to let you trace
process creation.  E.g. on Linux you can do this with strace (and I have
done this myself in the past to debug subprocess creation).  So I'd say
adding logging just to provide a small rarely used convenience for jdk
developers is Not Worthwhile.

Just on PlatformLogger then it just forwards to j.u.logging when it is 
present.


-Alan.

Re: RFR: 8034853 remove sun.misc.ClassLoaderUtil

[Alan Bateman]

On 14/02/2014 18:31, Michael McMahon wrote:


I think I'd probably just delete the test since URLClassLoader.close 
has its own tests,

I thought it might be useful as a point of reference
in case there could be someone still using the sun.misc API.
Also, I think mercurial won't delete the empty directory after the 
file is gone - not that that matters very much.
If the updated test just duplicate the URLClassLoader tests then 
deleting the old test seems reasonable to me. I don't see a need to 
leave a test in a location that no longer corresponds to anything.


-Alan

Re: RFR [4682009] Typo in javadocs in javax/naming

[Alan Bateman]

On 14/02/2014 19:48, Ivan Gerasimov wrote:

Hello!

May I please have a review of the fix?

It's not meant to be a proof reading, I only fixed some obvious typos.
Some of them were reported at the time of java 1.4, so it is probably 
the time to have them fixed.


BUG: https://bugs.openjdk.java.net/browse/JDK-4682009
WEBREV: http://cr.openjdk.java.net/~igerasim/4682009/0/webrev/

Looks good and nice to get these fixed after all these years.

-Alan

Re: RFR [4682009] Typo in javadocs in javax/naming

[Ivan Gerasimov]

On 15.02.2014 0:13, Lance Andersen - Oracle wrote:

Looks fine.  assume the  will be addressed as part of a full sweep 
of javax/naming
On Feb 14, 2014, at 2:48 PM, Ivan Gerasimov wrote:

Thanks!
Yes, I didn't touch formatting, only typos.


Hello!

May I please have a review of the fix?

It's not meant to be a proof reading, I only fixed some obvious typos.
Some of them were reported at the time of java 1.4, so it is probably the time 
to have them fixed.

BUG: https://bugs.openjdk.java.net/browse/JDK-4682009
WEBREV: http://cr.openjdk.java.net/~igerasim/4682009/0/webrev/

Sincerely yours,
Ivan



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

Re: RFR: 8034853 remove sun.misc.ClassLoaderUtil

[Mandy Chung]

On 2/14/14 10:31 AM, Michael McMahon wrote:

On 14/02/14 18:20, Alan Bateman wrote:

On 14/02/2014 17:42, Michael McMahon wrote:

Could I get the following change reviewed please?

http://cr.openjdk.java.net/~michaelm/8034853/webrev.1/

The change is to remove the class sun.misc.ClassLoaderUtil.
The functionality provided by this class is now in the public
java.net.URLClassloader.close() method.
The removal is good. Maybe the test should be moved to URLClassLoader 
as it doesn't make sense to have it in ClassLoaderUtil anymore.


-Alan.


I think I'd probably just delete the test since URLClassLoader.close 
has its own tests,


The change looks good to me.  I think URLClassLoader/closetest test 
covers this case already.  I agree with Alan that you can simply delete 
this old test.


Mandy

Re: RFR: 8034853 remove sun.misc.ClassLoaderUtil

[Stuart Marks]

On 2/14/14 10:31 AM, Michael McMahon wrote:

Also, I think mercurial won't delete the empty directory after the file is gone
- not that that matters very much.


If you delete the last file in a directory, the empty dir might stay around in 
your working copy. But the directory will be absent from new clones; Mercurial 
doesn't track empty directories. Go ahead and delete!


s'marks

Re: RFR: JDK-8032050: TEST_BUG: java/rmi/activation/Activatable/shutdownGracefully/ShutdownGracefully.java fails intermittently

[Stuart Marks]

Hi Tristan,

OK, we're getting close. Just a couple things about ShutdownGracefully.java.

It's a bit clumsy to have both a boolean and a message string to keep track of 
the state of the test, but by itself this isn't such a big deal.


A bigger deal is the lack of exception chaining. If we catch an unexpected 
exception at line 162, its class and message are printed out, but its stack 
trace isn't. If this were to occur it might be fiendishly difficult to debug.


The TimeoutException isn't chained up either, but this isn't so bad, since 
there's only one place the timeout can occur. Still, it's good to chain 
exceptions where possible.


There's another failure case that doesn't have an exception at all, which is 
when the registration request we're expecting to fail actually succeeds. This 
doesn't have an exception, but we should consider creating one.


Here's an approach to getting rid of the boolean+string and also chaining up 
exceptions. The key insight is that an exception can be created in a different 
place from where it's thrown.


Instead of the boolean+stream, have a variable of type Exception that's 
initialized to null. Anyplace where we catch an exception that indicates 
failure, fill in this variable. The idea is that if this exception variable is 
non-null, a failure has occurred. The exception being non-null replaces the 
boolean, and the exception message replaces the failure string. In addition, the 
exception has a stack trace, which is essential for debugging.


For the case where we don't get the expected exception (i.e., registration 
succeeds unexpectedly), simply set the exception variable to a newly created 
exception, but don't throw it yet. For example,


exception = new RuntimeException(
"The registration request succeeded unexpectedly");

At the place where we catch an unexpected exception, wrap the caught exception 
in a new RuntimeException with a message like "caught unexpected exception". The 
reason to do this is so we can add an additional message. The stack trace will 
also be a bit easier to follow.


For the timeout, just assign the TimeoutException to the exception variable.

Also, at each location where the exception variable is assigned to, print out a 
message at that point. It will at least show the state of the test to be a 
failure. The reason is that, if rmid.destroy() were to throw an exception from 
the finally-block, our carefully recorded exception state will be thrown away. 
(An alternative would be to put this into its own try-block, and add any 
exceptions caught from it to the suppressed exception list of the exception 
variable, but it's not clear to me that this is worth it.)


At the end of the test, if the exception variable is non-null, call 
TestLibrary.bomb() with it to fail the test.


Finally, one typo: "prcoess".

Thanks for updating this webrev again.

s'marks

On 2/13/14 12:25 AM, Tristan Yan wrote:

Thank you Stuart
I have fixed comment in JavaVM.java. Dealing with different cases in
ShutdownGracefully.java, two variables were added. One is a flag indicate test
passed or not. Other variable keeps the error message when test failed. I put
TestLibrary.bomb in the bottom of the main method which only shows test fail
message.
Could you review it again
http://cr.openjdk.java.net/~tyan/JDK-8032050/webrev.04/
Tristan

On 02/13/2014 05:29 AM, Stuart Marks wrote:

Hi Tristan,

JavaVM.waitFor looks mostly fine. The indentation of the start of the
waitFor(timeout) javadoc comment is a bit off, though; please fix.

There are still some adjustments that need to be made in
ShutdownGracefully.java. Both have to do with the case where the last
registration succeeds unexpectedly -- this is the one that we expect to fail.

First, if this registration has succeeded unexpectedly, that means rmid is
still running. If that occurs, the call to rmid.waitFor(timeout) will
inevitably time out. It may be worth calling rmid.destroy() directly at this
point.

Second, still in the succeeded-unexpectedly case, at line 154
TestLibrary.bomb() is called. This throws an exception, but it's caught by the
catch-block at lines 157-158, which calls TestLibrary.bomb() again, saying
"unexpected exception". Except that this is kind of expected, since it was
thrown from an earlier call to TestLibrary.bomb(). This is quite confusing.

There are several cases that need to be handled.

1. Normal case. Registration fails as expected, rmid has terminated
gracefully. Test passes.

2. Rmid is still running and has processed the registration request
successfully. Need to kill rmid and fail the test.

3. Rmid is still running but is in some bad state where the registration
request failed. Need to kill rmid and fail the test.

4. Some other unexpected failure. This is what the catch and finally blocks at
lines 157-161 are for.

These four cases need to be handled independently. Ideally they should be
separated from the cleanup code. As noted above, you don't want to throw a

Re: RFR: JDK-8032050: TEST_BUG: java/rmi/activation/Activatable/shutdownGracefully/ShutdownGracefully.java fails intermittently

[Tristan Yan]

Thank you Stuart
This is nice. I thought two variables were weird but I didn’t figure out the 
right solution.  Also I wasn’t so sure why we print out so many messages now 
it’s clear to me.
http://cr.openjdk.java.net/~tyan/JDK-8032050/webrev.05/
I’m sorry I have to ask you review this again.
regards
Tristan

On Feb 15, 2014, at 9:47 AM, Stuart Marks  wrote:

> Hi Tristan,
> 
> OK, we're getting close. Just a couple things about ShutdownGracefully.java.
> 
> It's a bit clumsy to have both a boolean and a message string to keep track 
> of the state of the test, but by itself this isn't such a big deal.
> 
> A bigger deal is the lack of exception chaining. If we catch an unexpected 
> exception at line 162, its class and message are printed out, but its stack 
> trace isn't. If this were to occur it might be fiendishly difficult to debug.
> 
> The TimeoutException isn't chained up either, but this isn't so bad, since 
> there's only one place the timeout can occur. Still, it's good to chain 
> exceptions where possible.
> 
> There's another failure case that doesn't have an exception at all, which is 
> when the registration request we're expecting to fail actually succeeds. This 
> doesn't have an exception, but we should consider creating one.
> 
> Here's an approach to getting rid of the boolean+string and also chaining up 
> exceptions. The key insight is that an exception can be created in a 
> different place from where it's thrown.
> 
> Instead of the boolean+stream, have a variable of type Exception that's 
> initialized to null. Anyplace where we catch an exception that indicates 
> failure, fill in this variable. The idea is that if this exception variable 
> is non-null, a failure has occurred. The exception being non-null replaces 
> the boolean, and the exception message replaces the failure string. In 
> addition, the exception has a stack trace, which is essential for debugging.
> 
> For the case where we don't get the expected exception (i.e., registration 
> succeeds unexpectedly), simply set the exception variable to a newly created 
> exception, but don't throw it yet. For example,
> 
>exception = new RuntimeException(
>"The registration request succeeded unexpectedly");
> 
> At the place where we catch an unexpected exception, wrap the caught 
> exception in a new RuntimeException with a message like "caught unexpected 
> exception". The reason to do this is so we can add an additional message. The 
> stack trace will also be a bit easier to follow.
> 
> For the timeout, just assign the TimeoutException to the exception variable.
> 
> Also, at each location where the exception variable is assigned to, print out 
> a message at that point. It will at least show the state of the test to be a 
> failure. The reason is that, if rmid.destroy() were to throw an exception 
> from the finally-block, our carefully recorded exception state will be thrown 
> away. (An alternative would be to put this into its own try-block, and add 
> any exceptions caught from it to the suppressed exception list of the 
> exception variable, but it's not clear to me that this is worth it.)
> 
> At the end of the test, if the exception variable is non-null, call 
> TestLibrary.bomb() with it to fail the test.
> 
> Finally, one typo: "prcoess".
> 
> Thanks for updating this webrev again.
> 
> s'marks
> 
> On 2/13/14 12:25 AM, Tristan Yan wrote:
>> Thank you Stuart
>> I have fixed comment in JavaVM.java. Dealing with different cases in
>> ShutdownGracefully.java, two variables were added. One is a flag indicate 
>> test
>> passed or not. Other variable keeps the error message when test failed. I put
>> TestLibrary.bomb in the bottom of the main method which only shows test fail
>> message.
>> Could you review it again
>> http://cr.openjdk.java.net/~tyan/JDK-8032050/webrev.04/
>> Tristan
>> 
>> On 02/13/2014 05:29 AM, Stuart Marks wrote:
>>> Hi Tristan,
>>> 
>>> JavaVM.waitFor looks mostly fine. The indentation of the start of the
>>> waitFor(timeout) javadoc comment is a bit off, though; please fix.
>>> 
>>> There are still some adjustments that need to be made in
>>> ShutdownGracefully.java. Both have to do with the case where the last
>>> registration succeeds unexpectedly -- this is the one that we expect to 
>>> fail.
>>> 
>>> First, if this registration has succeeded unexpectedly, that means rmid is
>>> still running. If that occurs, the call to rmid.waitFor(timeout) will
>>> inevitably time out. It may be worth calling rmid.destroy() directly at this
>>> point.
>>> 
>>> Second, still in the succeeded-unexpectedly case, at line 154
>>> TestLibrary.bomb() is called. This throws an exception, but it's caught by 
>>> the
>>> catch-block at lines 157-158, which calls TestLibrary.bomb() again, saying
>>> "unexpected exception". Except that this is kind of expected, since it was
>>> thrown from an earlier call to TestLibrary.bomb(). This is quite confusing.
>>> 
>>> There are several cases that need to be