[DISCUSS] Change signature of Serializable::fromData on Geode-Native

[Jacob Barrett <jb...@pivotal.io>]

I would like to propose a change:
Serializable* Serializable::formData(DataInput& in)
to
void Serializable::formData(DataInput& in)

The current signature allows the object being deserialized to return an
object other than itself. The use of this trick is only done internally for
a few internal system types in what appears to have been an attempt to make
serialization more pluggable. The downside to this approach is that it
leaks this ability out to the public interface. Additionally concerning is
that the return type is a raw pointer to Serializable but typically the
object was created as a std::shared_ptr, which can lead to shard_ptr errors
if you don't property check and swap the returned raw pointer against the
original shared_ptr. Lastly the return value is a pointer to the most base
interface providing little utility and type safety.

A couple of spikes investigated changing the signature to:
std::shared_ptr<Serializable> Serializable::formData(DataInput& in)
and:
template<class T>
std::shared_ptr<T> Serializable::formData(DataInput& in)
But both approaches left some dirty things laying around. First the
templated version just caused all sorts of pain and failed when the value
was replaced on the fromData. The more generic share_ptr<Serializable>
approach uncovered a plethora of places internally that we use the
Serializable::fromData to deserialize objects as parts of protocol messages
and used as internal data where they weren't originally created as
shared_ptrs, so the opposite problem to what we were trying to solve.

The final spike investigated using void. In doing so we only had to make
small changes to the way PDX types were being deserialized. The void
signature is also more consistent with the Java DataSerializable interface.
By making it void the ambiguity of using or checking the return value goes
away.

You can see the proposed changes on my fork at
https://github.com/pivotal-jbarrett/geode-native/tree/wip/fromDataVoid.

Thoughts?

-Jake

Re: [DISCUSS] Change signature of Serializable::fromData on Geode-Native

[Michael William Dodge <md...@pivotal.io>]

+1 for the void return type

> On 14 Sep, 2017, at 13:39, Ernest Burghardt <eb...@pivotal.io> wrote:
> 
> +1      cleans up the public API and code using this as you can see in the
> proposed changes on Jake's fork
> 
> On Wed, Sep 13, 2017 at 3:30 PM, Jacob Barrett <jb...@pivotal.io> wrote:
> 
>> I would like to propose a change:
>> Serializable* Serializable::formData(DataInput& in)
>> to
>> void Serializable::formData(DataInput& in)
>> 
>> The current signature allows the object being deserialized to return an
>> object other than itself. The use of this trick is only done internally for
>> a few internal system types in what appears to have been an attempt to make
>> serialization more pluggable. The downside to this approach is that it
>> leaks this ability out to the public interface. Additionally concerning is
>> that the return type is a raw pointer to Serializable but typically the
>> object was created as a std::shared_ptr, which can lead to shard_ptr errors
>> if you don't property check and swap the returned raw pointer against the
>> original shared_ptr. Lastly the return value is a pointer to the most base
>> interface providing little utility and type safety.
>> 
>> A couple of spikes investigated changing the signature to:
>> std::shared_ptr<Serializable> Serializable::formData(DataInput& in)
>> and:
>> template<class T>
>> std::shared_ptr<T> Serializable::formData(DataInput& in)
>> But both approaches left some dirty things laying around. First the
>> templated version just caused all sorts of pain and failed when the value
>> was replaced on the fromData. The more generic share_ptr<Serializable>
>> approach uncovered a plethora of places internally that we use the
>> Serializable::fromData to deserialize objects as parts of protocol messages
>> and used as internal data where they weren't originally created as
>> shared_ptrs, so the opposite problem to what we were trying to solve.
>> 
>> The final spike investigated using void. In doing so we only had to make
>> small changes to the way PDX types were being deserialized. The void
>> signature is also more consistent with the Java DataSerializable interface.
>> By making it void the ambiguity of using or checking the return value goes
>> away.
>> 
>> You can see the proposed changes on my fork at
>> https://github.com/pivotal-jbarrett/geode-native/tree/wip/fromDataVoid.
>> 
>> Thoughts?
>> 
>> -Jake
>> 

Re: [DISCUSS] Change signature of Serializable::fromData on Geode-Native

[Ernest Burghardt <eb...@pivotal.io>]

+1      cleans up the public API and code using this as you can see in the
proposed changes on Jake's fork

On Wed, Sep 13, 2017 at 3:30 PM, Jacob Barrett <jb...@pivotal.io> wrote:

> I would like to propose a change:
> Serializable* Serializable::formData(DataInput& in)
> to
> void Serializable::formData(DataInput& in)
>
> The current signature allows the object being deserialized to return an
> object other than itself. The use of this trick is only done internally for
> a few internal system types in what appears to have been an attempt to make
> serialization more pluggable. The downside to this approach is that it
> leaks this ability out to the public interface. Additionally concerning is
> that the return type is a raw pointer to Serializable but typically the
> object was created as a std::shared_ptr, which can lead to shard_ptr errors
> if you don't property check and swap the returned raw pointer against the
> original shared_ptr. Lastly the return value is a pointer to the most base
> interface providing little utility and type safety.
>
> A couple of spikes investigated changing the signature to:
> std::shared_ptr<Serializable> Serializable::formData(DataInput& in)
> and:
> template<class T>
> std::shared_ptr<T> Serializable::formData(DataInput& in)
> But both approaches left some dirty things laying around. First the
> templated version just caused all sorts of pain and failed when the value
> was replaced on the fromData. The more generic share_ptr<Serializable>
> approach uncovered a plethora of places internally that we use the
> Serializable::fromData to deserialize objects as parts of protocol messages
> and used as internal data where they weren't originally created as
> shared_ptrs, so the opposite problem to what we were trying to solve.
>
> The final spike investigated using void. In doing so we only had to make
> small changes to the way PDX types were being deserialized. The void
> signature is also more consistent with the Java DataSerializable interface.
> By making it void the ambiguity of using or checking the return value goes
> away.
>
> You can see the proposed changes on my fork at
> https://github.com/pivotal-jbarrett/geode-native/tree/wip/fromDataVoid.
>
> Thoughts?
>
> -Jake
>