Introducing move semantics in my project

And then, there'll be many

A bit of a long post today, actually.

When I first started this pet project, in 2012-05, my goal was to (re)learn C++ and create a tool to automate tasks performed on several machines. Since that meant working via ssh, I turned to libssh2. When I later found boost asio, I thought it would be a good idea to use it as a framework for my work with libssh2.

At that time, I googled «libssh2 boost asio», but didn't get much. I only found one developer sharing his work with libssh2 + asio, but his design was different from what I wanted to do. So, I went ahead, step by learning step, until I finally built my first working example... which, I know today, is chock full of errors.

During this time, I've had one comment from someone who also thought this would be a good idea. And today, on libssh2's bug tracker, someone mentioned using libssh2 and boost asio together.

I'm glad to see there's more people doing this. For now, if you search google for «libssh2 boost asio», my posts will be the top results. I hope someday I'll have some company.

Lessons in movement

When I first dabbled in C++, I never really came upon move semantics. Back then, I avoided copy ctors like the plague, and passed "expensive" objects by reference. And although Scott Meyer's books didn't have the item "Prefer references to pointers", it was a mantra I held, and it was something I used when classes needed handles to something they didn't own:

class SomeHandle

{

//...

};

class SomeSuch

{

//...

private:

    SomeHandle& handle;

}; 

Fast forward to C++11 - move semantics are everywhere, which puts a wrench on my previous mantra, because a) I have to leave the "moved-from" object empty; and b) I have to reseat the "moved-to" object members on move-assignment. So, I realize I can no longer use references on classes where I want move semantics.

Do I want move semantics on my SSH classes? I can't foresee a scenario where I'd need it. Still, I figured, since this is a new subject to me, and these are a set of inter-related classes, I'll definitely learn something from trying to implement move semantics here.

And learn I did, indeed.

I already mentioned Lesson #1, above, regarding the use of references as data members (courtesy of the great folk at Stack Overflow). Lesson #2 came after I tested my move ctors, which promptly resulted in an app crash.

Moving targets

My central class is SessionConnection. Here's part of its definition:

std::unique_ptr<boost::asio::io_service> ios;

boost::asio::ip::tcp::socket sock;

SessionHandle sessionHandle;

RemoteSessionHandle remoteSessionHandle;

SessionAuthenticator sessionAuthenticator;

This order must be maintained, because sessionAuthenticator requires SSH handshake complete, which is performed by remoteSessionHandle; which requires an SSH session, which is created by sessionHandle; which requires a TCP connection, which is guaranteed by sock; which requires an io_service.

Now, if we look at class RemoteSessionHandle, we'll find this:

SocketPtr sock;

SessionHandlePtr sessionHandle;

Notice the "Ptr" suffix - both are pointers. Both are injected on construction. Since there's no ownership involved, both are naked pointers, no smarts needed here (oh, and, both used to be references).

Now, let's look at SessionConnection's current move ctor:

SessionConnection::SessionConnection(SessionConnection&& other) :

    reportStatus(move(other.reportStatus)), host(move(other.host)),  

    port(move(other.port)), user(move(other.user)), pwd(move(other.pwd)), 

    ios(move(other.ios)), sock(move(other.sock)),

    sessionHandle(move(other.sessionHandle)), 

    remoteSessionHandle(move(other.remoteSessionHandle)),

    sessionAuthenticator(move(other.sessionAuthenticator))

{

}

How's that for simplicity? Here's the test program (SSHSession is a wrapper around the SSH functionality, and owns a SessionConnection):

int main(int argc, char *argv[])

{

    try

    {

        cout << "start" << endl;

        ConnectionInfo ci;

        ci.host = "192.168.56.101";

        ci.port = "22";

        ci.user = "user";

        ci.pwd = "password";

        SSHSession sess(ci, boost::bind(ShowStatus, _1));

        SSHSession sess2(move(sess));

        sess2.ExecuteCommand("ls -l", boost::bind(ShowStatus, _1), true);

        cout << "end" << endl;

    }

    catch (SSHBaseException& ex)

    {

        cout << diagnostic_information(ex) << endl;

    }

}

As I said above, this crashes (so much for simplicity, then). It crashes when sess2's dtor runs. Actually, it crashes here, in RemoteSessionHandle::DoCloseSession():

int rc = libssh2_session_disconnect(sessionHandle->GetSession(), 

    "Disconnect SSH session");

As to why... when we do this: SSHSession sess2(move(sess)), what happens to both - the old and the new - SessionConnection::remoteSessionHandles?

We're taking sess's existing SessionConnection (let's call it oldSC) and moving it to sess2's brand new SessionConnection (let's call it newSC), which means SessionConnection's move ctor gets called:

SessionConnection::SessionConnection(SessionConnection&& other) :

    ..., remoteSessionHandle(move(other.remoteSessionHandle)),...

You'll recall remoteSessionHandle.sessionHandle is a pointer, which is set up at remoteSessionHandle construction.

Before the move, we have this:

• oldSC.remoteSessionHandle.sessionHandle points to oldSC.sessionHandle. 
• newSC.remoteSessionHandle.sessionHandle doesn't exist yet.

After the move, we get this:

• oldSC.remoteSessionHandle.sessionHandle is nullptr.
• newSC.remoteSessionHandle.sessionHandle points to what oldSC.remoteSessionHandle.sessionHandle pointed to, i.e., oldSC.sessionHandle. 

However, oldSC (as well as sess) is now a "moved-from" object, and the C++ standard requires that it's destructible and assignable, and nothing more. In fact, we have these lines in main():

SSHSession sess2(move(sess));

sess2.ExecuteCommand("ls -l", boost::bind(ShowStatus, _1), true);

And the program's output is something like this:

... 

SSHSession dtor

total 44

<ls -l result omitted>

So, I'd say gcc notices sess is moved from and not used again, and destroys it right there. And, as far as I can see, it's perfectly allowed to do so.

So, this means that, when sess2's/newSC's dtors are called, we have a whole bunch of pointers happily pointing at the memory space formerly known as sess.oldSC. Which is why sessionHandle->GetSession() crashes, because what sessionHandle points at doesn't exist anymore.

How to correct this? Like I said, these classes are very inter-related, so I'll introduce some friend relations here, along with a private interface for reseating these pointers. And, while I'm at it, I'll also make private the current public members that expose libssh2 elements.

More on this, after I test it.