While trying to implement a simple State Pattern example from the book "Head First Design Patterns", I came across a situation which strikes me as kind of peculiar. Mind you, this question is not about implementing the pattern correctly, but about understanding the underlying mechanism that leads to the observed behavior.
The Machine "Gumball_machine" is supposed to have several possible states (No_quarter_state, Has_quarter_state, Sold_out_state, etc.), to which behavior is delegated via virtual function calls at runtime. These states are publicly inherited from an abstract base class State. Gumball_machine has an std::unique_ptr<State>, the State class itself a raw pointer to the Gumball_machine (since no ownership is assumed).
A state transition occurs when certain conditions are met, they happen via allocating a new concrete state class and transferring ownership to the Gumball_machine.
(I will post some code examples at the end of this post, since I want to "get to the point" first.)
There is one situation, where in the same function after having switched states, another function is called:
void Has_quarter_state::turn_crank()
{
std::cout << "You turned...\n";
machine_->state_ = std::make_unique<Sold_state>(machine_);
machine_->dispense(); // Invalid read!
// This works however (don't forget to comment out the above reallocation):
// Gumball_machine* ptr{machine_};
// machine_->state_ = std::make_unique<Sold_state>(machine_);
// ptr->dispense();
}
with machine_ being a pointer to the Gumball_machine, and state_ being an std::unique_ptr<State> to the concrete state, Has_quarter_state.
If I declare the temporary pointer ptr and invoke Gumball_machine::dispense(), there is no problem. However if I simply call machine_->dispense(), valgrind will show an invalid read (error message will be shown below).
And this I don't really understand. ptr and machine_ should refer to the same Gumball_machine instance, which should not be destroyed until the end of the program. Has_quarter_state (or rather the parent class "State") only has a raw pointer without ownership.
Now that I think of it, it's probably because the unique_ptr - reset will cause the memory which is occupied by the Has_quarter_state instance to be freed. That would probably mean that any subsequent action, i.e. the function call to Gumball_machine::dispense(), would result in undefined behavior. Is this assumption correct?
If the memory address (&memory_ == &ptr) doesn't change, why does it make a difference if I call ptr->dispense() or machine_->dispense()?
I feel there are some intricacies of memory management that I still don't understand. Hopefully, you will be able to help me clear things up.
Below I will post the code to reproduce this(the "incorrect" version) and the error message valgrind gives me (using --leak-check=full, --leak-kinds=all).
The code compiles via clang++ -std=c++14 -stdlib=libc++ using clang 3.6.0
Now for the actual code (greatly reduced to be more of a minimal example):
Gumball_machine.hpp:
#ifndef CLASS_GUMBALL_MACHINE_HPP_
#define CLASS_GUMBALL_MACHINE_HPP_
#include <memory>
class State;
class Gumball_machine
{
friend class Has_quarter_state;
friend class Sold_state;
public:
Gumball_machine();
~Gumball_machine();
void turn_crank();
private:
void dispense();
private:
std::unique_ptr<State> state_;
};
#endif
Gumball_machine.cpp:
#include "Gumball_machine.hpp"
#include "Has_quarter_state.hpp"
Gumball_machine::Gumball_machine() : state_{std::make_unique<Has_quarter_state>(this)} {}
Gumball_machine::~Gumball_machine() {}
void Gumball_machine::turn_crank() { state_->turn_crank(); }
void Gumball_machine::dispense() { state_->dispense(); }
State.hpp:
#ifndef CLASS_STATE_HPP_
#define CLASS_STATE_HPP_
class Gumball_machine;
class State
{
public:
explicit State(Gumball_machine* m);
virtual ~State();
virtual void turn_crank() = 0;
virtual void dispense() = 0;
protected:
Gumball_machine* machine_ = nullptr;
};
#endif
State.cpp:
#include "State.hpp"
State::State(Gumball_machine* m) : machine_{m} {}
State::~State() {}
Has_quarter_state.hpp:
#ifndef ClASS_HAS_QUARTER_STATE_HPP_
#define ClASS_HAS_QUARTER_STATE_HPP_
#include "State.hpp"
class Gumball_machine;
class Has_quarter_state : public State
{
public:
explicit Has_quarter_state(Gumball_machine*);
~Has_quarter_state() override;
void turn_crank() override;
void dispense() override;
};
#endif
Has_quarter_state.cpp:
#include "Has_quarter_state.hpp"
#include <iostream>
#include "Gumball_machine.hpp"
#include "Sold_state.hpp"
Has_quarter_state::Has_quarter_state(Gumball_machine* m) : State{m} {}
Has_quarter_state::~Has_quarter_state() {}
void Has_quarter_state::turn_crank()
{
std::cout << "You turned...\n";
machine_->state_ = std::make_unique<Sold_state>(machine_);
machine_->dispense(); // Invalid read!
// This works however (don't forget to comment out the above reallocation):
// Gumball_machine* ptr{machine_};
// machine_->state_ = std::make_unique<Sold_state>(machine_);
// ptr->dispense();
}
void Has_quarter_state::dispense()
{
std::cout << "No gumball dispensed\n";
}
Sold_state.hpp:
#ifndef ClASS_SOLD_STATE_HPP_
#define ClASS_SOLD_STATE_HPP_
#include "State.hpp"
class Gumball_machine;
class Sold_state : public State
{
public:
explicit Sold_state(Gumball_machine*);
~Sold_state() override;
void turn_crank() override;
void dispense() override;
};
#endif
Sold_state.cpp:
#include "Sold_state.hpp"
#include <iostream>
#include "Gumball_machine.hpp"
#include "Has_quarter_state.hpp"
Sold_state::Sold_state(Gumball_machine* m) : State{m} {}
Sold_state::~Sold_state() {}
void Sold_state::turn_crank()
{
std::cout << "Turning twice doesn't give you another gumball\n";
}
void Sold_state::dispense()
{
std::cout << "A gumball comes rolling out the slot\n";
// machine_->state_.reset(new No_quarter_state{machine_});
machine_->state_ = std::make_unique<Has_quarter_state>(machine_);
}
EDIT: main.cpp
int
main ()
{
Gumball_machine machine;
machine.turn_crank();
return 0;
}
And finally the valgrind output:
==12085== Memcheck, a memory error detector
==12085== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==12085== Using Valgrind-3.10.1 and LibVEX; rerun with -h for copyright info
==12085== Command: ./main
==12085==
==12085== Invalid read of size 8
==12085== at 0x401C61: Has_quarter_state::turn_crank() (in /home/mbw/Documents/Programmieren/CPP/Design_Patterns/Head_First_Design_Patterns/Chapter10_State_pattern/Example1_Revised/example_for_stackoverflow/main)
==12085== by 0x401730: Gumball_machine::turn_crank() (in /home/mbw/Documents/Programmieren/CPP/Design_Patterns/Head_First_Design_Patterns/Chapter10_State_pattern/Example1_Revised/example_for_stackoverflow/main)
==12085== by 0x402FF7: main (in /home/mbw/Documents/Programmieren/CPP/Design_Patterns/Head_First_Design_Patterns/Chapter10_State_pattern/Example1_Revised/example_for_stackoverflow/main)
==12085== Address 0x5e47048 is 8 bytes inside a block of size 16 free'd
==12085== at 0x4C2CE10: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==12085== by 0x4017B4: operator delete(void*, unsigned long) (in /home/mbw/Documents/Programmieren/CPP/Design_Patterns/Head_First_Design_Patterns/Chapter10_State_pattern/Example1_Revised/example_for_stackoverflow/main)
==12085== by 0x401858: Has_quarter_state::~Has_quarter_state() (in /home/mbw/Documents/Programmieren/CPP/Design_Patterns/Head_First_Design_Patterns/Chapter10_State_pattern/Example1_Revised/example_for_stackoverflow/main)
==12085== by 0x401B27: Has_quarter_state::turn_crank() (in /home/mbw/Documents/Programmieren/CPP/Design_Patterns/Head_First_Design_Patterns/Chapter10_State_pattern/Example1_Revised/example_for_stackoverflow/main)
==12085== by 0x401730: Gumball_machine::turn_crank() (in /home/mbw/Documents/Programmieren/CPP/Design_Patterns/Head_First_Design_Patterns/Chapter10_State_pattern/Example1_Revised/example_for_stackoverflow/main)
==12085== by 0x402FF7: main (in /home/mbw/Documents/Programmieren/CPP/Design_Patterns/Head_First_Design_Patterns/Chapter10_State_pattern/Example1_Revised/example_for_stackoverflow/main)
==12085==
==12085==
==12085== HEAP SUMMARY:
==12085== in use at exit: 0 bytes in 0 blocks
==12085== total heap usage: 3 allocs, 3 frees, 48 bytes allocated
==12085==
==12085== All heap blocks were freed -- no leaks are possible
==12085==
==12085== For counts of detected and suppressed errors, rerun with: -v
==12085== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
Thank you in advance for your help!
The problem is the fact that Has_quarter_state instance on which you are calling turn_crank is destroyed when you replace _machine->state with a new std::unique_ptr:
machine_->state_ = std::make_unique<Sold_state>(machine_);
Here you are replacing machine_->state with a new unique_ptr which contains another object. This means that ~unique_ptr<State>() is called right before constructing the new unique_ptr for the new Sold_state. But the current owned object of the unique pointer is the Has_quarter_state instance which is implicitly referred by this in the executing method.
Then what you do?
you do machine_->dispense() which is this->machine_->dispense() but machine_ is an instance variable of the object that has just been destroyed (and on which you called the current executing method) so its value is not valid anymore.
Assigning machine_ to a temporary works because you copy the content of the member field of the object before destroying it. So you can then still access the machine correctly.
Without using std::unique_ptr and by forcing each state to manage its own deallocation you see that something is wrong because the (almost) equivalent code (which would be a really BAD design) would be the following:
void Has_quarter_state::turn_crank() {
this->machine_->state_ = new Sold_state();
delete this;
this->machine_->dispense();
}
Now you see that first you delete this, then you try to access a field which is part of a deallocated object.