Back to libssh2/boost asio

I'm coming back to my libssh2/boost asio code, after taking a (long) break.

Why the break? I didn't like some parts of the design, but I wasn't able to find alternatives. And, more importantly, while I got to a working design in a short time, as I began improving that code, I found my knowledge of the language lacking, so I've decided to pursue other problems, in order to broaden that knowledge.

Now, as I return, my first task is getting a cleaner design. Next, I will be finishing the logging to put the code on GitHub.

I'll be discussing the classes SessionHandle and RemoteSessionHandle in this post. These classes are a bit different now, but the main concepts remain. I'll also talk about the class SessionConnection, which I haven't presented yet.

My first stumbling block appeared when I added move semantics to my SSH session classes.

RemoteSessionHandle has a reference (a pointer, actually) to an instance of SessionHandle. As I said at the time, that's "because the libssh2 calls need the LIBSSH2_SESSION* stored in SessionHandle", i.e., in order to do its work, RemoteSessionHandle needs a LIBSSH2_SESSION*, and this is owned by SessionHandle. Therefore, a RemoteSessionHandle instance needs a SessionHandle instance from which to get the LIBSSH2_SESSION*.

When creating an SSH session on a remote host, I create an instance of SessionConnection, which has, among its data members, an instance of SessionHandle and an instance of RemoteSessionHandle.

After the SessionHandle is successfully constructed, RemoteSessionHandle will be constructed, taking SessionHandle as an argument, and storing its address in a pointer.

So, SessionConnection is performing a "sort-of" dependency injection here. Since RemoteSessionHandle doesn't own this SessionHandle instance (so, no delete necessary), and it needs to be nullable (for the move operation), I've decide to use a naked pointer.

However, I ran into a problem with move. I'l try to demonstrate using my amazing artistic skills. Here's what we have before the move happens:

movedFromSessionConnection           movedToSessionConnection
+------------------------------+     +----------------------------+
| movedFromSessionHandle       |     | movedToSessionHandle       |
|            ^                 |     |            ^               |
|            |                 |     |            |               |
| movedFromRemoteSessionHandle |     | movedToRemoteSessionHandle |
+------------------------------+     +----------------------------+

Both SessionHandles and both RemoteSessionHandles are members of SessionConnection, and each RemoteSessionHandle points to its own SessionHandle instance, and all's well and jolly. Now, this is what we get with a "regular" move ctor, i.e., a move ctor that moves all the data members:

movedFromSessionConnection --- mv ---> movedToSessionConnection
+------------------------------+     +----------------------------+
| movedFromSessionHandle ---- mv ----> movedToSessionHandle       |
|            ^ -----------------------------------+               |
|                                                 |               |
| movedFromRemoteSessionHandle - mv -> movedToRemoteSessionHandle |
+------------------------------+     +----------------------------+

movedToRemoteSessionHandle received the contents of movedFromRemoteSessionHandle, including the pointer to movedFromSessionHandle. Which, after being moved from, is in an unknown state. So, the way this is designed, the move operation shouldn't move the pointer. Which is the bit I don't like very much.

I have considered several alternatives, and I've asked here, trying to get some more. However, I believe I was looking at the problem from the wrong angle - while there are several ways to perform this injection, in the end, it all leads to the same question:

How is RemoteSessionHandle going to store its dependency?

There are two options:

• Store it as a pointer

The current problem doesn't come from storing a pointer, but rather from the scope of what's being pointed to. If SessionHandle's scope was decoupled from SessionConnection's (meaning, if SessionHandle could outlive SessionConnection), I wouldn't have this problem. OTOH, since these are auxiliary classes for SessionConnection, I like the fact that their scopes are coupled.

• Store it as a value

None of these classes are copyable. Initially, I considered I'd never implement copy; now, I believe I may, someday, implement it. Still, "no copy" means the only way for RemoteSessionHandle to store its dependency by value is to remove sessionHandle from SessionConnection and place it in RemoteSessionHandle, as a data member. This also means that RemoteSessionHandle becomes the provider of the LIBSSH2_SESSION*, e.g., for creating the channel when executing commands.

For now, I believe the second option is the cleanest, even though I don't like having to use RemoteSessionHandle to get to the LIBSSH2_SESSION*. However, since I can't really come up with a rational reason for it, this is what went for.

I've left an issue to address at a later date: We're not supposed to perform a move while a command is executing. The channel classes don't have either copy nor move ctor/assignment. I may change this in the future.

Let's keep moving (will the puns ever end?)

So, how is my first Adventure in Moveland going?

The solution for my classes went according to planned, which was nice. SessionHandle, being the class that holds LIBSSH2_SESSION*, found itself suddenly surrounded by many new friends:

friend class SessionConnection;

friend class RemoteSessionHandle;

friend class SessionAuthenticator;

friend class RemoteChannelHandle;

SessionHandle::GetSession(), which provides access to said LIBSSH2_SESSION*, went from public to private. And both SessionHandle and RemoteSessionHandle (which also gained SessionConnection as friend) now have private Reseat() functions, to allow SessionConnection to reseat the pointers on move:

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))

{

    sessionHandle.Reseat(reportStatus);

    remoteSessionHandle.Reseat(reportStatus, sock, sessionHandle);

}

So, everything went smoothly, right? Well, not quite. When I tested my move ctor, I was still getting a SegFault. It was now happening during the socket's dtor, more exactly here (boost's scoped_lock.hpp):

// Constructor acquires the lock.

scoped_lock(Mutex& m)

  : mutex_(m)

{

  mutex_.lock();

  ...

}

So, it looked like I was still going to learn something more about Boost.Asio. I began tracing the socket's dtor. Here's boost::asio::basic_stream_socket's definition:

template <typename Protocol,

    typename StreamSocketService = stream_socket_service<Protocol> >

class basic_stream_socket

  : public basic_socket<Protocol, StreamSocketService>

Notice the stream_socket_service<>? Let's take a look at it (relevant info only):

template <typename Protocol>

class stream_socket_service

#if defined(GENERATING_DOCUMENTATION)

  : public boost::asio::io_service::service

#else

  : public boost::asio::detail::service_base<stream_socket_service<Protocol> >

#endif

{ 

... 

  // The type of the platform-specific implementation.

#if defined(BOOST_ASIO_HAS_IOCP)

  typedef detail::win_iocp_socket_service<Protocol> service_impl_type;

#else

  typedef detail::reactive_socket_service<Protocol> service_impl_type;

#endif

...

  // The platform-specific implementation.

  service_impl_type service_impl_;

};

Since we're on Windows and have BOOST_ASIO_HAS_IOCP #defined, let's take a look at win_iocp_socket_service:

template <typename Protocol> 

class win_iocp_socket_service : public win_iocp_socket_service_base

There is actually little of interest here, so we move on to win_iocp_socket_service_base:

class win_iocp_socket_service_base

{

...

// Mutex to protect access to the linked list of implementations.

boost::asio::detail::mutex mutex_;

...

};

Holy Shmoly, Duck Dodgers! It has a mutex! And it's neither a reference nor a pointer!

So, what does it hold? In our case (Windows), it's an instance of boost::asio::detail::win_mutex, and it encapsulates an OS critical section object. And it performs RAII, too; win_mutex's dtor looks like this:

~win_mutex()

{

  ::DeleteCriticalSection(&crit_section_);

}

We can read here what DeleteCriticalSection() does, but this is the important bit:

After a critical section object has been deleted, do not reference the object in any function that operates on critical sections (such as EnterCriticalSection, TryEnterCriticalSection, and LeaveCriticalSection) other than InitializeCriticalSection and InitializeCriticalSectionAndSpinCount. If you attempt to do so, memory corruption and other unexpected errors can occur.

Memory corruption and other unexpected errors? Check, eager young space cadet.

So, basically, even though Boost.Asio's socket is moveable (according to std::is_move_constructible<> and std::is_move_assignable<>), it may hold objects that aren't. In this case, we were left with two instances of win_iocp_socket_service_base, each with its instance of win_mutex that pointed to the same OS critical section object.

When the first socket ("moved to") is destroyed, ~win_mutex() is called, and the OS is told to get rid of that pesky critical section object pointed by crit_section_. Which it dutifully does.

Then, the second socket ("moved from") is destroyed. During which, this is called:

void win_iocp_socket_service_base::destroy( 

  win_iocp_socket_service_base::base_implementation_type& impl)

{

  ...

  // Remove implementation from linked list of all implementations.

  boost::asio::detail::mutex::scoped_lock lock(mutex_);

  ...

Obviously, you can guess what mutex_ is holding, right? A lovely pointer to an ex-pesky ex-critical (and probably ex-)section. So when scoped_lock's ctor does this: mutex_.lock(), this gets called:

void lock()

{

  ::EnterCriticalSection(&crit_section_);

}

Windows looks at it and says "Why, how quaint! Didn't you just tell me, a few nano-seconds ago, to get rid of this? You're probably having memory problems. But I've got just the thing to solve that. Here, have a SegFault! Oh, and a nice day, too, of course". Or, as the duck said "And brother, when it disintegrates, it disintegrates".

So, now what? Well, I've decided to apply to socket the same treatment I gave io_service, namely, it has gone from tcp::socket sock to unique_ptr<tcp::socket> sock.

Next time, the final part of this moving adventure - adding move semantics to the channel classes. And deciding whether I want to allow move when a command is executing.

That won't be the end of it, naturally. I'll have to make this thread-safe, and then I'll have to revisit all this. But, one step at a time.

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.