IMAP client impementation on State Machine boost-ext/sml

Question

I made IMAP(IMAPS) notifier on boost-ext/sml state machine. There is an opinion that networking is best done on machines. Compile in C++20. Tested on GMail server. Uses karastojko/mailio (and therefore boost+openssl)

Just checking new emails every c_RecheckInterval_minutes minutes.

Here are two key points that concern me:

1. State machine definition in Impl.h inside struct ImapsSm;
2. State machine using in Thread.h inside void Thread::run().

Impl.h

#pragma once
namespace StateMachine {
// for action
typedef std::function< void(const std:string&) > setActiveTip_t;

// events
struct CheckAccess{};
struct Unavailable{};
struct StartLoop{};
struct HasBreak{};
struct RenewEmail{};
struct ContinueLoop{};

// dependencies
struct dependencyImaps {
    const Credentials m_credentials;
    std::chrono::seconds c_connectTimeout_sec;
};
struct dependencyEmailCount {
    setActiveTip_t fnSetActiveTip;
    unsigned long m_currentEmailCount, m_lastEmailCount;
    std::string lastFrom, lastSubject
};
struct dependencyBreakableSleep {
    const std::chrono::seconds c_RecheckInterval_sec;
    std::condition_variable *m_pConditionVariable;
    std::mutex *m_pcvMutex;
    bool m_bBreak;
};
struct dependencyAvailable {
    bool m_bAvailable;
};

namespace Action {
struct Imap { void operator()(dependencyImaps &imaps, dependencyEmailCount& counter) {
    std::unique_ptr< mailio::imaps > pmailio;
    pmailio = std::make_unique< mailio::imaps >( 
            imaps.m_credentials.host
            , imaps.m_credentials.port
            , imaps.c_connectTimeout_sec
        );
    pmailio ->authenticate(
            imaps.m_credentials.login
            , imaps.m_credentials.password
            , mailio::imaps::auth_method_t::LOGIN
        );
    const char mailbox[] = "inbox";
    pmailio ->select( mailbox );
    std::list< mailio::imap::search_condition_t > search_condition;
    // UNSEEN exclude ALL
    search_condition.emplace_back( mailio::imap::search_condition_t::UNSEEN );
    std::list<unsigned long> results;
    pmailio ->search( search_condition, results );
    unsigned long lastSequenceNumber = results.back( );

    // Find out sender and subject
    mailio::message lastMessage;
    bool header_only;
    pmailio ->fetch( mailbox, lastSequenceNumber, lastMessage, header_only = true );
    counter.m_currentEmailCount = lastSequenceNumber;
    counter.lastFrom = lastMessage.from_to_string( );
    counter.lastSubject = lastMessage.subject( );
} } Imap;

struct BreakableSleep { void operator()(dependencyBreakableSleep &dependency) {
    std::unique_lock< std::mutex > lock( *dependency.m_pcvMutex );
    std::cv_status finishedWaiting = dependency.m_pConditionVariable ->wait_until( 
            lock
            , std::chrono::system_clock::now( ) + dependency.c_RecheckInterval_sec 
        );
    if ( std::cv_status::no_timeout == finishedWaiting )
        dependency.m_bBreak = true;
} } BreakableSleep;

struct InitialEmailCount { void operator()(dependencyEmailCount& dependency) {
    dependency.m_lastEmailCount = dependency.m_currentEmailCount;
} } InitialEmailCount;

struct EmailCounter { void operator()(dependencyEmailCount& dependency) {
    if ( dependency.m_currentEmailCount <= dependency.m_lastEmailCount )
        return;
    dependency.m_lastEmailCount = dependency.m_currentEmailCount;

    dependency.fnSetActiveTip( std::string{ }
            + "SmallImapsNotifier: last email\n"
            + "from: '" + dependency.lastFrom + "'\n"
            + "subject: '" + dependency.lastSubject + "'\n"
        );
} } EmailCounter;

struct InboxAvailable { void operator()(dependencyAvailable& dependency) {
    dependency.m_bAvailable = true;
} } InboxAvailable;

} // namespace Action

struct guardAvailable { 
    bool operator()(dependencyAvailable& dependency) const { 
        return !dependency.m_bAvailable;
    }
} guardAvailable;
struct guardBreak { 
    bool operator()(dependencyBreakableSleep &dependency) const { 
        return dependency.m_bBreak;
    }
} guardBreak;

struct ImapsSm {
  auto operator()() const noexcept {
    using namespace boost::sml;
    using namespace Action;
    return make_transition_table(
        // Using `state<>` to avoid print anonymous on sml logging in MSVC
        *state<struct idle> + event<CheckAccess> / Imap = "TryInbox"_s

        , "TryInbox"_s + event<Unavailable> [guardAvailable] = X
        , "TryInbox"_s + event<StartLoop> / InitialEmailCount = "Waiting"_s
        , "TryInbox"_s + on_entry<_> / InboxAvailable

        , "Waiting"_s + event<HasBreak> [guardBreak] = X
        , "Waiting"_s + event<RenewEmail> / Imap= "RenewedEmail"_s
        , "Waiting"_s + on_entry<_> / BreakableSleep

        , "RenewedEmail"_s + event<Unavailable> [guardAvailable] = X
        , "RenewedEmail"_s + event<ContinueLoop> = "Waiting"_s
        , "RenewedEmail"_s + on_entry<_> / ( InboxAvailable, EmailCounter )

        , *("exceptions handling"_s) + exception<_> / [](dependencyAvailable& dependency) { dependency.m_bAvailable = false; } = X
    );
  }
};
} // namespace StateMachine

Thread.h

#pragma once
class Thread {
    static constexpr std::chrono::seconds c_ReconnectTry_sec{ 60 };
    static constexpr std::chrono::seconds c_ConnectTimeout_sec{ 30 };
    static constexpr std::chrono::minutes c_RecheckInterval_minutes{ 5 };

    Credentials m_credentials;
    StateMachine::setActiveTip_t m_clbSetActiveTip;

    std::condition_variable m_cv;
    std::mutex m_cvMutex;
    std::atomic_bool m_bStop;
    std::thread m_thread;

    bool breakableWaitReconnect_() {
        if ( m_bStop )
            return false;
        std::unique_lock< std::mutex > lock( m_cvMutex );
        std::cv_status waiting = m_cv.wait_until( lock, 
            std::chrono::system_clock::now( ) + c_ReconnectTry_sec );
        return std::cv_status::timeout == waiting;
    }

    // thread
    void run() {
        namespace sml = boost::sml;
        using namespace StateMachine;
        do {
            dependencyImaps imaps = { m_credentials, c_ConnectTimeout_sec };
            dependencyEmailCount counter = { m_clbSetActiveTip };
            dependencyBreakableSleep sleep = { 
                    c_RecheckInterval_minutes
                    , &m_cv
                    , &m_cvMutex
                };
            sml::sm< ImapsSm > sm{ imaps, counter, sleep };
            sm.process_event( CheckAccess{ } );
            sm.process_event( Unavailable{ } );
            sm.process_event( StartLoop{ } );
            while ( !sm.is( sml::X ) ) {
                sm.process_event( HasBreak{ } );
                sm.process_event( RenewEmail{ } );
                sm.process_event( Unavailable{ } );
                sm.process_event( ContinueLoop{ } );
            }
        } while ( breakableWaitReconnect_( ) );
    }

 public:
    Thread(
        const Credentials &credentials
        , StateMachine::setActiveTip_t clbSetActiveTip
    )
        : m_credentials( credentials )
        , m_clbSetActiveTip( clbSetActiveTip )
        , m_bStop( false )
    {
        // will be init in last order
        m_thread = { &Thread::run, this };
    }

    void stop() {
        m_bStop = true;
        m_cv.notify_all( );
        m_thread.join( );
    }
};

demo main.cpp, with invalid login and password

#include "pch.h"
#include <mailio/imap.hpp>
#include <boost/sml.hpp>
struct Credentials {
    std::string host;
    uint16_t port;
    std::string login, password;
};
#include "Impl.h"
#include "Thread.h"
void main() {
    Credentials credentials = {
            "imap.gmail.com", 993, "%login%", "%password%"
        };
    auto imapsThread = std::make_unique< Thread >( 
            credentials
            , [](const std::string&str) {
                std::cout << str;
            }
        );
    getchar( );
}

How does all this correspond to the "canons" of state machines? Not overkill?

Full code on GitHub repo

Thanks!

Answer 1

Unnecessary use of std::unique_ptr

You are using std::unique_ptr when there is no need for it. Both pmailio and imapsThread can just be stored by value:

struct Imap { void operator()(dependencyImaps &imaps, dependencyEmailCount& counter) {
    mailio::imaps pmailio(…);
    pmailio.authenticate(…);
    …
}
…
void main() {
    Credentials credentials = {…};
    Thread imapsThread(credentials, …);
    getchar();
}

Lambdas are structs

I see you write this pattern a few times:

struct Foo {
    void operator()(/* arguments */…) {
        /* body */
        …
    }
} Foo;

A lambda is just syntactic sugar for exactly that pattern! So you could also write:

auto Foo = [](/* arguments */…) {
    /* body */
    …
};

Incorrect use of Hungarian notation

I see you are using some form of Hungarian notation. The problem with Hungarian notation is that nothing checks whether it is applied correctly. If you make mistakes, the whole benefit of Hungarian notation is kind of gone. Consider for example c_connectTimeout_sec. The c_ prefix indicates that it's a const variable, however it is not. At the same time, m_credentials is const but that's not reflected in the prefix. You also use cv for condition variables, but then why is it m_cvMutex, when that variable is not a condition variable?

My advice is to stop using Hungarian notation, except for maybe using the prefix m_ to indicate that something is a class member variable, but only because the latter is useful to avoid name conflicts with parameters and local variables.

Don't mix atomics and mutexes

Make m_bStop a regular bool, and read and modify it only when holding a lock. Consider for example that breakableWaitReconnect_() has just checked m_bStop, and its value was false. The before it does anything else, stop() is called, which sets m_bStop to true (which does nothing), and which then calls m_cv.notify_all()(which also does nothing because no other thread iswait()ing yet). Then breakableWaitReconnect_() continues, locks the mutex and callsl m_cv.wait_until(…). Now you have to wait at least 60 seconds before that thread stops.

The correct way to handle this is to use a variant of wait() that uses a predicate, and to only modify m_bStop while holding the mutex:

bool breakableWaitReconnect_() {
    std::lock_guard lock(m_cvMutex);
    auto waiting = m_cv.wait_for(lock, c_ReconnectTry_sec, []{
        return m_bStop;
    });
    return std::cv_status::timeout == waiting;
}
…
void stop() {
    {
        std::lock_guard lock(m_cvMutex);
        m_bStop = true;
    }
    m_cv.notify_all( );
    m_thread.join( );
}

Use wait_for() instead of wait_until()

If you only need to wait for a given period, then use wait_for(). Apart from not needing the manual calculation of the point in time to wait until, wait_for() also uses a steady clock, which is not affected by things like daylight savings time changes, or just someone adjusting their computer's clock. This avoids your code accidentally waiting for hours instead of seconds.

About the use of state machines

There are several ways in which you could have approached this problem. The use of an explicit state machine is one way. The drawback is that you have to explicitly define all the states and their relationships with each other. Coroutines are another approach, which allows you to write code in a more natural way. Boost has coroutine libraries as well, and C++20 added native supported for coroutines, but you still need some external library at the moment to make effective use of them.

I don't think your use of Boost SML looks wrong, but I have never used it myself.

Naming things

Some of your class names are very generic, and don't describe what they actually do. Consider Thread for example. That's way more than just being something like std::thread. You could name it IMAPThread. However, while it is using a thread, that might not be the most important thing to know about it. More important would be to know if this implements a client or a server. So perhaps IMAPClient is a better name? The same goes for Credentials: perhaps IMAPClientCredentials is more accurate.

Also consider using a namespace to group your classes together, and/or use the fact that you can nest class declarations. Since everything is related to IMAP, maybe use a hierarchy like:

namespace IMAP {

class Client {
public:
    class Credentials {
        …
    };
    …
};

}

Then your main() could look like:

void main() {
    using IMAP;
    Client::Credentials credentials = {…};
    Client client(credentials, …);
    getchar();
}