Code review request for 6998871 "Support making the Throwable.stackTrace field immutable"

[Rémi Forax]

Maybe I'm wrong but you can reuse the same clone.
I mean, it has to have a different identity from UNASSIGNED_STACK but
it can be the same for all clones.

Rémi

On 04/08/2011 08:27 PM, Joe Darcy wrote:
> 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.
>
>