RFR [9] 8071472: Add field setter API for setting final fields in readObject

[Peter Levart]

On 03/25/2015 04:49 PM, Chris Hegarty wrote:
> Peter,
>
> On 25/03/15 12:24, Peter Levart wrote:
>> On 03/23/2015 04:45 PM, Chris Hegarty wrote:
>>> Here is an updated version of the FieldSetter API, with all comments
>>> to date incorporated.
>>>
>>>
>>> http://cr.openjdk.java.net/~chegar/8071472/02/specdiff/overview-summary.html 
>>>
>>>
>>>
>>> Final spec comments welcome, after which I intend to submit a CCC
>>> request.
>>>
>>> -Chris.
>>
>> Hi Chris,
>>
>> Recent bug "JDK-8068721 - RMI-IIOP communication fails when
>> ConcurrentHashMap is passed to remote method" made me thinking...
>>
>> ObjectInputStream is an extensible API. There are subclasses of
>> ObjectInputStream out there.
>
> I did think about this previously, but thought it ok ( I assumed that 
> overridden methods would first invoke their superclass counterparts ). 
> It appears that InputStreamHook does not do this.
>
> > For example, there is an abstract
>> "com.sun.corba.se.impl.io.InputStreamHook" with concrete implementation
>> "com.sun.corba.se.impl.io.IIOPInputStream". The InputStreamHook
>> overrides ObjectInputStream.defaultReadObject() with it's own
>> implementation which doesn call ObjectInputStream.defaultReadObject(),
>> which means that FieldSetterContext.checkPersistentFinalsNotSet() and
>> FieldSetterContext.markPersistentFinalsSet() is not called for
>> InputStreamHook based subclasses when user's readObject() calls
>> defaultReadObject(), which further means that
>> FieldSetter.checkAllFinalsSet() that is eventually called as last thing
>> in user's readObject() method might find any persistent finals as not
>> set and throw InvalidObjectException.
>>
>> I have been thinking about this and I see several solutions:
>>
>> 1. provide protected final methods
>> ObjectInputStream.checkPersistentFinalsNotSet() and
>> markPersistentFinalsSet() that just delegate to FieldSetterContext for
>> ObjectInputStream classes to call as part of their own overriden
>> defaultReadObject() method. We should make sure those methods are called
>> in JDK's corba InputStreamHook and document they should be called in 3rd
>> party subclasses.
>
> Yes, I think I agree with this, but since the FieldSetterContext is 
> constructed in OIS private readSerialData, there is nothing to 
> delegate to, unless InputStreamHook/IIOPInputStream creates the context.
>
>> 2. in ObjectInputStream constructor, detect if defaultReadObject() is
>> overridden and set a flag that makes FieldSetter.checkAllFinalsSet()
>> effectively a no-op.
>
> Hmm... maybe, but we I think this could fall foul of JCK. A compliant 
> implementation should/must throw if all finals are not set, right?
>
>> 3. 1 + 2 + enable for ObjectInputStream subclasses to declare that they
>> obey the contract and that they call markPersistentFinalsSet() from
>> overridden defaultReadObject() so checkAllFinalsSet() can be enabled.
>
> Yes, again the problem is that they need to be able to create a field 
> setter context.
>
>> What do you think?
>
> I am still thinking about this. I do like your suggestions, but I 
> don't see how they will work ( regard to the construction of the 
> context ).
>
> This problem is not unique to OIS, it is a general problem with 
> evolving any concrete subclassable class, but I think we do need a 
> reasonable solution.
>
> Another possibility is that a default implementation of fieldSetter(), 
> in a subclass, could throw UOE, Yuck!
>
> Even if we drop the restriction on checking final field set, or not 
> set, there is still the issue of when/how the field context can be 
> created in a subclass. I think we may need to expose a factory method 
> for this, along with your suggestions above.
>
> -Chris.

Hi Chris,

Here's how I imagined it. Idea stolen and adapted from parallel-capable 
ClassLoaders ;-) ...

http://cr.openjdk.java.net/~plevart/jdk9-sandbox/serial-exp-branch/webrev.corba.02/
http://cr.openjdk.java.net/~plevart/jdk9-sandbox/serial-exp-branch/webrev.jdk.02/


This is a variant 3 from above. For ObjectInputStream subclasses that 
don't override defaultReadObject(), everything should behave as before 
this change. Those that do override defaultReadObject() have a choice:

- they are *not* finals-tracking capable and checkAllFinalsSet() method 
only checks that non-persistent finals have been set
- they opt-int to be finals-tracking capable by registering and 
declaring that their overridden defaultReadObject() method either 
delegates to a super method which *MUST* be finals-tracking capable or 
explicitly call exposed protected methods: 
checkPersistentFinalsNotSet()/markPersistentFinalsSet().


That's it.

Haven't tested this yet. Just an idea.

Regards, Peter

>
>
>> Regards, Peter
>>