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.

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.

Success with the new design! And RemoteSessionHandle

It's still early days, there's still a lot to polish, but it connects and disconnects successfully.

On this post, I'll introduce RemoteSessionHandle, the class responsible for implementing RAII on the SSH session remote "resources", i.e., the SSH session on the remote host.

Once again, the interface is simple:

class RemoteSessionHandle

{

public:

    RemoteSessionHandle(boost::function<void(std::string)>& pReportStatus,  

        boost::asio::ip::tcp::socket& pSock, SessionHandle& pSessionHandle);

    ~RemoteSessionHandle();

    void CloseSession();

    // WE MAY ADD MOVE IN THE FUTURE, BUT NOT COPY

    RemoteSessionHandle(RemoteSessionHandle&&) = delete;

    RemoteSessionHandle& operator=(RemoteSessionHandle&&) = delete;

    RemoteSessionHandle(RemoteSessionHandle const&) = delete;

    RemoteSessionHandle& operator=(RemoteSessionHandle const&) = delete;

private:

    utils::TaskState DoHandshake();

    utils::TaskState DoCloseSession();

    utils::CleanupState cleanupState;

    boost::function<void(std::string)>& reportStatus;

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

    SessionHandle& sessionHandle;

};

Ctor, dtor, and a method to explicitly close the session on the remote host. The private methods are the handlers for the loop functions.

We're storing references to the report callback (to let the UI know what we're doing), to the socket (used in the ctor to create the session), and to an instance of SessionHandle (because the libssh2 calls need the LIBSSH2_SESSION* stored in SessionHandle).

RemoteSessionHandle::RemoteSessionHandle( 

    boost::function<void(std::string)>& pReportStatus,

    boost::asio::ip::tcp::socket& pSock, SessionHandle& pSessionHandle) :

    cleanupState(utils::CleanupState::NoCleanup), 

    reportStatus(pReportStatus), sock(pSock), sessionHandle(pSessionHandle)

{

    if (!reportStatus.empty())

        reportStatus("SSH Handshake");

    io_service& ios = sock.get_io_service();

    ios.reset();

    sock.async_read_some(boost::asio::null_buffers(), bind(AsyncLoopSocket,

        protect(bind(&RemoteSessionHandle::DoHandshake, this)), 

        boost::ref(sock), true));

    ios.run();

}

The ctor contains the SSH handshake. We get the io_service from the socket, and use it to drive our loop.

RemoteSessionHandle::~RemoteSessionHandle()

{

    if (!reportStatus.empty())

        reportStatus("Cleanup: SSH remote session.");

    // WE'LL ONLY TRY TO CLEANUP IF NO ATTEMPT HAS BEEN MADE YET

    if (cleanupState == utils::CleanupState::NoCleanup)

    {

        try

        {

            CloseSession();

        }

        catch (...)

        { }

    }

}

The dtor's behaviour is similar to SessionHandle's dtor. We only perform the cleanup if no attempt has been previously made.

void RemoteSessionHandle::CloseSession()

{

    io_service ios;

    deadline_timer dt(ios);

    dt.expires_from_now(milliseconds(10));

    dt.async_wait(bind(AsyncLoopTimer, 

        protect(bind(&RemoteSessionHandle::DoCloseSession, this)), 

        boost::ref(dt), 10));

    cleanupState = utils::CleanupState::CleanupInProgress;

    ios.run();

    cleanupState = utils::CleanupState::CleanupDone;

}

CloseSession() sets up the loop with a timer, and manages the cleanup state.

utils::TaskState RemoteSessionHandle::DoHandshake()

{

    int rc = libssh2_session_handshake(sessionHandle.GetSession(),  

        sock.native_handle());

    if (rc == LIBSSH2_ERROR_EAGAIN)

    {

        return utils::TASK_WORKING;

    }

    if (rc)

    {

        BOOST_THROW_EXCEPTION(SSHHandshakeError() <<

            ssh_error_string("Error " + lexical_cast<string>(rc) +  

            " during SSH Handshake.") << ssh_error_id(rc));

    }

    return utils::TASK_DONE;

}

utils::TaskState RemoteSessionHandle::DoCloseSession()

{

    int rc = libssh2_session_disconnect(sessionHandle.GetSession(),  

        "Disconnect SSH session");

    if (rc == LIBSSH2_ERROR_EAGAIN)

    {

        return utils::TASK_WORKING;

    }

    if (rc)

    {

        BOOST_THROW_EXCEPTION(SSHDisconnectSessionError() <<

            ssh_error_string("Error " + lexical_cast<string>(rc) +  

            " disconnecting SSH session.") << ssh_error_id(rc));

    }

    return utils::TASK_DONE;

}

DoHandshake() and DoCloseSession() follow the same pattern. There's not much to add here.

This post is very short on words, mostly just stating the obvious. I believe that's a good thing - it means the class is simple.

The code is here.

SSH Session - Class SessionHandle

And... I'm back. Work has gone back to manageable levels, and I've taken a few days off C++, to give my head a chance to catch up.

I also took the opportunity to get some feedback on my design. It was quite satisfying to hear that I've been heading in the right direction all this time. And, once again, while writing (and rewriting) that question, I've hit yet another design change, one that I believe will simplify my work when I introduce other forms of authentication (for now, it's just login/password).

So, let's get moving.

Today, I'll introduce SessionHandle, the class responsible for implementing RAII on the local SSH session resources.

The interface is pretty simple:

class SessionHandle

{

public:

    SessionHandle();

    ~SessionHandle();

    void CloseSession();

    LIBSSH2_SESSION* GetSession() { return session; }

    // WE MAY ADD MOVE IN THE FUTURE, BUT NOT COPY

    SessionHandle(SessionHandle&&) = delete;

    SessionHandle& operator=(SessionHandle&&) = delete;

    SessionHandle(SessionHandle const&) = delete;

    SessionHandle& operator=(SessionHandle const&) = delete;

private:    

    utils::TaskState DoCloseSession();

    utils::CleanupState cleanupState;

    LIBSSH2_SESSION *session;

};

We define a ctor/dtor. All other special operations are deleted. I suppose I may have to implement move semantics, but I won't worry about that now. CloseSession() allows the client to explicitly release the resources, in which case the dtor won't do anything. GetSession() is necessary for the other classes that need the LIBSSH2_SESSION*, although I may review this and make these classes friend, thus removing the need to expose it to the outside world.

DoCloseSession() is the handler called by the loop functions, and cleanupState is a flag to tells us if we need to cleanup on the dtor.

The code itself is also simple, which is good, since this was one of the goals of this redesign.

SessionHandle::SessionHandle() : 

    cleanupState(utils::CleanupState::NoCleanup), 

    session(nullptr)

{

    session = libssh2_session_init();

    if (!session)

    {

        BOOST_THROW_EXCEPTION(SSHCreateSessionError() << 

            ssh_error_string("Error creating SSH session."));

    }

    libssh2_session_set_blocking(session, 0);

}

There's no need for a loop here, because creating the session is always a blocking operation. Speaking of which, for now blocking on the session is hard-coded. I may make it configurable in a future version.

SessionHandle::~SessionHandle()

{

    // WE'LL ONLY TRY TO CLEANUP 

    // IF NO ATTEMPT HAS BEEN MADE YET

    if ((session != nullptr) && 

        (cleanupState == utils::CleanupState::NoCleanup))

    {

        try

        {

            CloseSession();

        }

        catch (...)

        { }

    }

}

The dtor calls CloseSession() to perform the cleanup, but only if no attempt has been previously made; meaning, if the user didn't call CloseSession() explicitly. If such an attempt was made, we won't try it again (even if it failed).

Also on the to-do list is applying boost's current_exception() to the catch block.

void SessionHandle::CloseSession()

{

    io_service ios;

    deadline_timer dt(ios);

    dt.expires_from_now(milliseconds(10));

    dt.async_wait(bind(AsyncLoopTimer, 

        protect(bind(&SessionHandle::DoCloseSession, this)), 

        boost::ref(dt), 10));

    cleanupState = utils::CleanupState::CleanupInProgress;

    ios.run();

    session = nullptr;

    cleanupState = utils::CleanupState::CleanupDone;

}

The use of a timer and a local io_service here avoids the whole outstanding handlers issue. Once cleanup is completed, we set the state accordingly, to make sure our dtor won't try to cleanup again.

utils::TaskState SessionHandle::DoCloseSession()

{

    int rc = libssh2_session_free(session);

    if (rc == LIBSSH2_ERROR_EAGAIN)

    {

        return utils::TASK_WORKING;

    }

    if (rc)

    {

        BOOST_THROW_EXCEPTION(SSHFreeSessionError() <<

            ssh_error_string("Error " + lexical_cast<string>(rc) + 

            " freeing SSH session"));

    }

    return utils::TASK_DONE;

}

Finally, the handler follows the pattern we've setup a couple of months ago. So, nothing new here.

Incidentally, I said in that post that "This is equivalent to libssh2's ssh2_exec example, and I'll be discussing it here in the following posts". I haven't discussed the example here, as such, because as I started writing the posts about it (and actually thinking about its design), I became aware of its failings, and that's when the redesign began. However, the functionality is still identical, it's only the organization that changed.

This class is the first result of this redesign. It's simple, it's focused. And I suppose I'll learn at some point in the future it's not entirely right. But that's a good thing. Otherwise, how would we learn?

You can get all the code here, namely:

• exception.h, which contains the base exception definitions.
• sshexception.h, which contains the exception definitions for the ssh classes.
• misc.h, which contains the enum with our cleanup state.
• sessionhandle.h and sessionhandle.cpp. The class we've presented above.

And you'll also need this.

Yes, I know. I need to setup a place somewhere to store this code.