Code review request for 6998871 "Support making the Throwable.stackTrace field immutable"
Joe Darcy
joe.darcy at oracle.com
Fri Apr 8 18:27:33 UTC 2011
Updated webrev described below...
Joe Darcy wrote:
> Joe Darcy wrote:
>> Hi Rémi.
>>
>> Rémi Forax wrote:
>>> On 04/07/2011 08:29 AM, Joe Darcy wrote:
>>>> Hello.
>>>>
>>>> Returning to some earlier work, I've developed a proposed fix for
>>>>
>>>> 6998871 "Support making the Throwable.stackTrace field immutable"
>>>> http://cr.openjdk.java.net/~darcy/6998871.2/
>>>>
>>>> One constructor of Throwable now takes an additional boolean
>>>> argument to make the stack trace information immutable. Analogous
>>>> constructors are added to Exception, RuntimeException, and Error.
>>>>
>>>> Mandy and David have already reviewed the change; I'm interested in
>>>> getting additional feedback on the design of the API.
>>>>
>>>> Cheers,
>>>>
>>>> -Joe
>>>
>>> Joe,
>>> I don't think you need the sentinel in the serialized form,
>>> you have only two states: an immutable stacktrace (stacktrace ==
>>> null) or
>>> a stacktrace. I think it's better to don't serialize the field
>>> "stacktrace" if
>>> the stacktrace is immutable.
>>>
>>> Also, FILLED_IN_STACK is not necessary, you can use EMPTY_STACKinstead,
>>> or if you find it hard to understand, at least FILLED_IN_STACK
>>> should be an empty array.
>>>
>>> Rémi
>>>
>>
>> Here is an explanation and rationale of the protocol in more detail.
>> As far as serialization goes, there are three cases of interest where
>> "new" means JDK 7 and "old" means any release prior to JDK 7
>>
>> 1) Object serialized on *new* platform has same semantics after being
>> deserialized on *new* platform.
>>
>> 2) Object serialized on *old* platform has same semantics after being
>> deserialized on *new* platform.
>>
>> 3) Object serialized on *new* platform has reasonable semantics after
>> being deserialized on *old* platform.
>>
>> (And of course when both source and destination platforms are old,
>> JDK 7 isn't involved.)
>>
>> For point 1), a logically equivalent object should result, which in
>> this case means the cause, stack trace, stack trace, message, etc.
>> should be the same on the old and new objects.
>>
>> For 2), if an exception object serialized prior to the stack trace
>> facility being added was deserialized in JDK 7, that is logically
>> equivalent to there being not stack trace information, so an empty
>> stack is a better result than an immutable stack. I've updated the
>> readObject method accordingly.
>>
>> For 3), since null was not a valid value for stackTrace in the past,
>> some other sentinel object should be written in out in the serial
>> form to denote such a value, which is the role played by
>> STACK_TRACE_SENTINEL as used in the writeObject method.
>>
>> A marker value other than EMPTY_STACK is needed for the following
>> reason since the stack trace information is spread amongst two
>> fields, backtrace and stackTrace. The transient backtrace field
>> pre-dates the programmatic stack facility being added; that facility
>> uses the stackTrace field. If fillInStackTrace is called *after* a
>> call to setStackTrace, the real stack information is held in the
>> backtrace field as written by fillInStackTrace. The FILLED_IN_STACK
>> value alerts the getourstacktrace method to this situation.
>>
>> Revised webrev:
>>
>> http://cr.openjdk.java.net/~darcy/6998871.3/
>>
>> Thanks,
>>
>> -Joe
>
> PS A little more detail here, exception objects can be created via
> deserialization as well as by a constructor. As currently written,
> the writeObject method uses EMPTY_STACK for both a zero-length
> incoming stack and a null stackTrace pointer (which can result from an
> throwable object serialized from a JDK release before the stack trace
> facility was added). The native fillInStackTrace method writes a null
> into the stackTrace field to indicate it is no longer valid; this goes
> against the new protocol and the Java-level of fillInStackTraces needs
> to write a non-null "dirty" value into the field to signal to
> getOurStackTrace that the stack trace array needs to be recomputed if
> requested. Therefore, if fillInStackTrace were called on one of these
> deserialized objects to logically replace its stack trace, using
> EMPTY_STACK would not allow the getOutStackTrace code to know that the
> stack trace had logically been replaced by a new value in backtrace
> since EMPY_STACK could result from serialization.
>
> The serialization code could use EMPTY_STACK.clone() instead of
> EMPTY_STACK, which would break the object equality being tested in
> getOurStackTrace. I'll consider making this change in the
> Throwable.readObject method.
>
> Cheers,
>
> -Joe
I've put the updated webrev up at
http://cr.openjdk.java.net/~darcy/6998871.4/
Diff of Throwable compared to .3 version is below. EMPTY_STACK has been
renamed to UNASSIGNED_STACK and that value (or a clone if it) is used as
the sentential and for zero-length arrays. The distinct FILLED_IN_STACK
value has been removed. The protocol around updating the fields is
better documented. In other files, ArithmeticException,
NullPointerException, and OutOfMemoryError are updated to state the VM
may created them with immutable stack too.
Thanks,
-Joe
157,163d156
< * A value indicating that the logical stack trace has been
< * populated into the backtrace field.
< */
< private static final StackTraceElement[] FILLED_IN_STACK =
< new StackTraceElement[] {new StackTraceElement("FILLED_IN",
"STACK", null, -1)};
<
< /**
166c159
< private static final StackTraceElement[] EMPTY_STACK = new
StackTraceElement[0];
---
> private static final StackTraceElement[] UNASSIGNED_STACK = new
StackTraceElement[0];
211c204,205
< * #setStackTrace()} and {@link #fillInStackTrace} will be be no-ops.
---
> * #setStackTrace(StackTraceElement[])} and {@link
> * #fillInStackTrace()} will be be no-ops.
216c210
< private StackTraceElement[] stackTrace = EMPTY_STACK;
---
> private StackTraceElement[] stackTrace = UNASSIGNED_STACK;
330c324,325
< * #fillInStackTrace()} and subsequent calls to {@code
---
> * #fillInStackTrace()}, a {@code null} will be written to the
> * {@code stackTrace} field, and subsequent calls to {@code
339,342c334,339
< * conditions under which suppression is disabled. Disabling of
< * suppression should only occur in exceptional circumstances
< * where special requirements exist, such as a virtual machine
< * reusing exception objects under low-memory situations.
---
> * conditions under which suppression is disabled and document
> * conditions under which the stack trace is not writable.
> * Disabling of suppression should only occur in exceptional
> * circumstances where special requirements exist, such as a
> * virtual machine reusing exception objects under low-memory
> * situations.
770c767
< stackTrace = FILLED_IN_STACK;
---
> stackTrace = UNASSIGNED_STACK;
807c804
< if (stackTrace == FILLED_IN_STACK) {
---
> if (stackTrace == UNASSIGNED_STACK) {
813c810
< return EMPTY_STACK;
---
> return UNASSIGNED_STACK;
886,888c883,886
< * trace elements. A single-element stack trace whose entry is
< * equal to {@code new StackTraceElement("", "", null,
< * Integer.MIN_VALUE)} results in a {@code null} {@code
---
> * trace elements. A null stack trace in the serial form results
> * in a zero-length stack element array. A single-element stack
> * trace whose entry is equal to {@code new StackTraceElement("",
> * "", null, Integer.MIN_VALUE)} results in a {@code null} {@code
918c916,924
< // Check for the marker of an immutable stack trace
---
> /*
> * For zero-length stack traces, use a clone of
> * UNASSIGNED_STACK rather than UNASSIGNED_STACK itself to
> * allow identity comparison against UNASSIGNED_STACK in
> * getOurStackTrace. The identity of UNASSIGNED_STACK in
> * stackTrace indicates to the getOurStackTrace method that
> * the stackTrace needs to be constructed from the information
> * in backtrace.
> */
920d925
< // Share zero-length stack traces
922c927
< stackTrace = EMPTY_STACK;
---
> stackTrace = UNASSIGNED_STACK.clone();
923a929
> // Check for the marker of an immutable stack
trace
937c943
< stackTrace = EMPTY_STACK;
---
> stackTrace = UNASSIGNED_STACK.clone();
976,977c982,983
< * {@linkplain #Throwable(String, Throwable, boolean) via a
< * constructor}. When suppression is disabled, this method does
---
> * {@linkplain #Throwable(String, Throwable, boolean, boolean) via
> * a constructor}. When suppression is disabled, this method does
1043,1044c1049,1050
< * #Throwable(String, Throwable, boolean) suppression is disabled},
< * an empty array is returned.
---
> * #Throwable(String, Throwable, boolean, boolean) suppression is
> * disabled}, an empty array is returned.
More information about the core-libs-dev
mailing list