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

[Chris Hegarty]

Peter,

Some more thought is needed in that area of the field setter API. If there are no strong objections, then I’d like to proceed with this version of better failure atomicity, and follow up as needed.

-Chris.

On 20 Mar 2015, at 16:30, Chris Hegarty <chris.hegarty at oracle.com> wrote:

> Hi Peter,
> 
> On 20 Mar 2015, at 14:06, Peter Levart <peter.levart at gmail.com> wrote:
> 
>> 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?
> 
> No. I should have explained.
> 
> From ObjectStreamClass.getClassDataLayout:
>    * Returns array of ClassDataSlot instances representing the data layout
>    * (including superclass data) for serialized objects described by this
>    * class descriptor.  ClassDataSlots are ordered by inheritance with those
>    * containing "higher" superclasses appearing first.  The final
>    * ClassDataSlot contains a reference to this descriptor.
> 
> … so slots contains entries for only Serializable types in the hierarchy, and from superclass down.
> 
> The first entry can be skipped as its stream field values will always be read, checked, and then set, regardless of anything else in the hierarchy ( as you can see I split defaultReadFields so that is now only reads stream field vales, and they can then be “checked”, and set separately). It only becomes “interesting" when there are more than one level of Serializable type in the hierarchy.
> 
>> 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     }
> 
> All slots can contain field values, so they will need to be set. Note to self: I need to verify that this is ok when a slot has no data ( let me check ).
> 
>> 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.
> 
> Right. Almost everything can be delayed until the first observable effect, i.e. a readObject[NoData].  Fields from non-Serializable types in the hierarchy will remain set, as per the default no-args constructor.
> 
>> 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).
> 
> I agree with this. I think it could be built on top of this work. In many cases the reset/rollback can be completely avoided, but in the case where a readObject throws an Exception then the rollback could be triggered.  It will really come down to code complexity whether this proposal adds value, when the rollback/reset is implemented.
> 
>> 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.
> 
> I certainly think it is worth exploring, and I will help with that myself. We can explore this in the sandbox ( where the FieldSetter API exists ), maybe a branch off serial-exp-branch ?
> 
> I would like to move the FieldSetter API forward separate to this effort. I will send mail on that later today to see if we can finalize/agree final API wording.
> 
> -Chris.
> 
>> Regards, Peter
>> 
>