Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Another shared_from_this bug... This time, invalidated shared_from_this state after SAVING (not loading) #68

Closed
Devacor opened this issue Mar 7, 2014 · 6 comments
Labels
Milestone

Comments

@Devacor
Copy link

Devacor commented Mar 7, 2014

This bug literally took me a month to write a minimal test case for. I assumed I was doing something bad, but it seems cereal's archive molests the shared_from_this state without calling a destructor!

See the line with: //BUSTED

#include <strstream>

#include "cereal/cereal.hpp"
#include "cereal/types/map.hpp"
#include "cereal/types/vector.hpp"
#include "cereal/types/memory.hpp"
#include "cereal/types/string.hpp"
#include "cereal/types/base_class.hpp"

#include "cereal/archives/json.hpp"
#include "cereal/types/polymorphic.hpp"

#include <memory>

struct Node : public std::enable_shared_from_this<Node> {
    Node(int val):data1(val){ std::cout << "Base Constructor" << std::endl; }
    virtual ~Node(){ std::cout << "Base Destructor" << std::endl; }

    template <class Archive>
    void serialize(Archive & archive){
        archive(CEREAL_NVP(data1), CEREAL_NVP(child));
    }

    template <class Archive>
    static void load_and_allocate(Archive & archive, cereal::allocate<Node> &allocate){
        int data1;
        archive(cereal::make_nvp("data1", data1));
        allocate(data1);
        archive(cereal::make_nvp("child", allocate->child));
    }

    std::shared_ptr<Node> child;

    int data1 = 0;
};

struct DerivedNode : public Node {
    DerivedNode(int val):Node(val), data2(val){ std::cout << "Derived Constructor" << std::endl; }
    ~DerivedNode(){ std::cout << "Derived Destructor" << std::endl; }

    template <class Archive>
    void serialize(Archive & archive){
        archive(CEREAL_NVP(data2), cereal::make_nvp("base", cereal::base_class<Node>(this)));
    }

    template <class Archive>
    static void load_and_allocate(Archive & archive, cereal::allocate<DerivedNode> &allocate){
        allocate(-1);
        archive(cereal::make_nvp("data2", allocate->data2), cereal::make_nvp("base", cereal::base_class<Node>(allocate.ptr())));
    }

    int data2 = 2;
};

CEREAL_REGISTER_TYPE(DerivedNode);

void saveTest(){
    int testInt = 10;
    int *pointInt = &testInt;
    std::stringstream stream;
    {
        std::shared_ptr<Node> node = std::make_shared<Node>(1);
        node->child = std::make_shared<DerivedNode>(3); //if this is just "Node" it's fine again!
        auto test1 = node->child->shared_from_this(); //Fine

        cereal::JSONOutputArchive archive(stream);
        auto test2 = node->child->shared_from_this(); //Fine

        auto nvp = cereal::make_nvp("node", node);
        auto test3 = node->child->shared_from_this(); //Fine

        archive(nvp);
        auto test4 = node->child->shared_from_this(); //BUSTED
        std::cout << "Completed Save!" << std::endl;
    }
    std::cout << stream.str() << std::endl;
    std::cout << std::endl;
    {
        std::shared_ptr<Node> node;

        cereal::JSONInputArchive archive(stream);
        archive(cereal::make_nvp("node", node));

        node->shared_from_this();
        std::cout << "We good?" << node->data1 << std::endl;

        node->child->shared_from_this();
        std::cout << "We still good?" << node->child->data1 << std::endl;
    }
    std::cout << "Done" << std::endl;
}

int main(){
    saveTest();
}
@Devacor
Copy link
Author

Devacor commented Mar 7, 2014

I am at work right now, so I cannot do further testing for a few hours, but I have a hunch it might be related to cereal::base_class.

When I get home I plan on removing the cereal::base_class call and seeing if it still breaks.

@AzothAmmo
Copy link
Contributor

Much simpler test case:

struct A : std::enable_shared_from_this<A>
{
  template <class Archive>
    void serialize( Archive & ar )
    { }

  virtual void foo() = 0;
};

struct B : A
{
  template <class Archive>
    void serialize( Archive & ar )
    { }

  void foo() {}
};

CEREAL_REGISTER_TYPE(B);

int main()
{
  {
    cereal::JSONOutputArchive ar(std::cout);
    std::shared_ptr<A> a( new B() );

    ar( a );

    auto x = a->shared_from_this();
  }
}

@AzothAmmo
Copy link
Contributor

Fairly confident the error is due to this line. In this function we are being given a void pointer to whatever your shared_ptr holds but we know its true type (the template type T, due to registering the type earlier). I think what is happening is that the temporary shared_ptr<T> we construct is messing up the internal weak_ptr.

Also this has nothing to do with base_class or any of its variants.

@AzothAmmo AzothAmmo added the bug label Mar 8, 2014
@AzothAmmo AzothAmmo added this to the v1.0.0 milestone Mar 8, 2014
@Devacor
Copy link
Author

Devacor commented Mar 8, 2014

Awesome, thanks for looking into it! Yeah, my test case was quite heavy in comparison to the true minimal example you provided. I was just so relieved to have it nailed down to something outside of my own code base. Also glad to help with another report! :)

AzothAmmo added a commit that referenced this issue Mar 9, 2014
cereal no longer permanently modifies the state of internal workings of
std::enable_shared_from_this when saving.  cereal *should* be completely
compatible with both saving and loading anything that inherits from this now.
This fixes issue #68 - note that there is still a minor issue regarding
classes declared final that will run into a bug with the way we check for
enable_shared_from_this (see issue #65).

Issue #65 will be addressed in the future by changing the way we check
for derivation from enable_shared_from_this.  In the current scheme, we
can detect this even if you use protected inheritance.  In the future, cereal
will not be able to get around protected inheritance of enable_shared_from without
befriending cereal::access.  This will come at the benefit of allowing classes
declared final to be used with polymorphic serialization.
AzothAmmo added a commit that referenced this issue Mar 9, 2014
Apparently MSVC doesn't want typename where it makes sense to use
typename (dependent type from a template).  Preprocessor verbosely
saves the day, yet again.  Relates #68
@AzothAmmo
Copy link
Contributor

Hoping this is the last of the issues related to enable_shared_from_this, which I officially deem the most annoying standard library feature to support.

@Devacor
Copy link
Author

Devacor commented Mar 9, 2014

Agreed. Love you. Want your babies. Thank god. I'll post again if I find any more bugs. My code base seems to be very useful for picking out this kind of intricate bullshit. :S

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants