geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jacob Barrett <jbarr...@pivotal.io>
Subject [DISCUSS] Change signature of Serializable::fromData on Geode-Native
Date Wed, 13 Sep 2017 21:30:58 GMT
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

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message