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
