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

[Joe Darcy]

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