RFR [9] 8071474: Better failure atomicity for default read object

[Peter Levart]

On 03/19/2015 10:26 PM, Chris Hegarty wrote:
> The current implementation of defaultReadObject sets non-primitive 
> field values one at a time, without first checking that all their 
> types are assignable. If, for example, the assignment of the last 
> non-primitive value fails, a CCE is thrown, and the previously set 
> fields remain set. The setting of field values, including primitives, 
> can be deferred until after they have been "validated", at a minimum 
> that the non-primitive types are assignable. This is achievable per 
> Class in the hierarchy up until the first user visible affect, i.e. a 
> readObject(NoData) method. Either all fields will be set, or contain 
> their default values.
>
> http://cr.openjdk.java.net/~chegar/8071474/webrev.00/webrev/
>
> I think there are further improvements that can be made in this area, 
> but I would like to move things forward incrementally.
>
> -Chris.

Hi Chris,

I'd just ask you a few questions to confirm my understanding of the code:

In ObjectInputStream:

1886         // Best effort Failure Atomicity; Each element in 
'slotFieldValues'
1887         // contains the stream field values for the same element in 
'slots',
1888         // up to the first slot with a readObject(NoData) method ( 
a user
1889         // visible effect ).
1890         int index = 1;
1891         for (; index < slots.length; index++) {
1892             ObjectStreamClass slotDesc = slots[index].desc;
1893             if (slotDesc.hasReadObjectMethod()
1894                 || slotDesc.hasReadObjectNoDataMethod()) {
1895                 break;
1896             }
1897         }
1898         // Store, and defer setting, values for index slots, ignore 
if just one.
1899         StreamFieldValues[] slotFieldValues = null;
1900         if (index > 1 && obj != null)
1901             slotFieldValues =  new StreamFieldValues[index];


...you scan slots starting from 1 and up. Is this because slots[0] is an 
uninteresting slot that belongs to j.l.Object class?

In this loop, you scan slots from 0 and up. Is this to catch corrupted 
streams that contain data for Object slot?

1971     /** Sets slot field values in the given obj. */
1972     private void setSlotFieldValues(Object obj,
1973 ObjectStreamClass.ClassDataSlot[] slots,
1974                                     StreamFieldValues[] 
slotFieldValues) {
1975         int length = slotFieldValues.length;
1976         for (int i = 0; i < length; i++) {
1977             if (slotFieldValues[i] != null)
1978                 defaultSetFieldValues(obj, slots[i].desc, 
slotFieldValues[i]);
1979         }
1980     }

I agree that this is an incremental improvement. You buffer values read 
from stream, check their types and delay setting them in particular 
class slots for classes not declaring  readObject[NoData] methods (or a 
span of slots in hierarchy ranging from Object to one before the class 
that declares readObject[NoData] method) which guarantees either success 
or atomic failure.

But have you considered a strategy that we discussed before where you 
would allow gradual building of state in object being deserialized and 
in case this fails anywhere (even in class slots declaring 
readObject[NoData] method), you roll-back the state (setting fields to 
default values) in all class slots from the one throwing an exception 
down to the 1st non-Serializable superclass (or Object). This would 
cover failure atomicity for the whole object, not just the slots not 
declaring readObject[NoData] methods. The premise is that 
deserialization starts with an uninitialized object (just class slots 
from 1st non-Serializable superclass down to Object are initialized by 
default accessible constructor). So rolling back the object to this 
state can be achieved by setting all instance fields of Serializable 
class slots to default values. A package-private 
FieldSetterContext.FieldsMap could be reused for this purpose (by adding 
a resetAllFields(Object obj) method). The FieldsMap instance is 
obtainable from ObjectStreamClass.getFieldAccessMap() which caches it 
(although internal, these type and method need renaming).

If you think this is a promising alternative to your failure atomicity 
proposal, I volunteer to add resetAllFields() method. I understand that 
this can only be added as part or on the top of FieldSetter API proposal.

Regards, Peter