Sasza Stanczew 245cbec983 Reliable - pierwszy commit

2024-05-06 11:58:30 +02:00

23 KiB

Raw Blame History

Refactoring

Old C++98/11 code

Please everyone to sit comfortably. I will show You a piece of code which I had to reactor a few years ago. This code will be simpler than those which I handled, but general ideas stayed, like: void* etc...

Let's start from Valgrind output about this piece of code:

==1667238== More than 10000000 total errors detected.  I'm not reporting any more.
==1667238== Final error counts will be inaccurate.  Go fix your program!

General idea

We want to have a RequestHandler which will store requests in the queue and handle them on a separate thread. We have 4 requests:

Download sth, for instnace an image,
Upload sth,
Remove data from cache,
Clear all cached values.

We have an API to Server class that has one method handle, which has 4 overloads, each for one request.

Requests

struct Credentail { int cert_; };

struct Download {
    enum ConnectionType { Telnet,
                          Ssh };

    std::string url_;
    Credentail credentail_;
    int maxMbps_;
    bool cache_;
    ConnectionType type_;
};

struct Upload {
    enum ConnectionType { Telnet,
                          Ssh };

    std::string url_;
    Image image_;
    Credentail credentail_;
    int maxMbps_;
    int size_;
    ConnectionType type_;
};

struct RemoveFromCache {
    std::string url_;
};

struct ClearCache {
};

___

Server class

class Server {
public:
    void handle(const Download& request, void (*callback)(bool, Image));

    void handle(const Upload& request, void (*callback)(bool, Image));

    void handle(const RemoveFromCache& request, void (*callback)(bool, Image));

    void handle(const ClearCache& request, void (*callback)(bool, Image));

private:
    bool download(const Download& request, Image* image);

    bool upload(const Upload& request, const Image* image);

    std::vector<std::pair<Image, std::string>> cache_;
};

Server class - one of handle method

void handle(const Download& request, void (*callback)(bool, Image)) {
    Image image;

    for (const auto& pair : cache_) {
        if (pair.second == request.url_) {
            image = pair.first;
            callback(true, image);
            return;
        }
    }

    if (download(request, &image)) {
        if (request.cache_) {
            cache_.push_back(std::pair<Image, std::string>(image, request.url_));
        }

        callback(true, image);
        return;
    }

    callback(false, Image{});
}

Server class - download method

bool download(const Download& request, Image* image) {
    if (request.type_ == Download::Ssh && request.credentail_.cert_ != 123) {
        return false;
    }
    if (request.type_ == Download::Telnet && request.credentail_.cert_ != 231) {
        return false;
    }

    // Simulate some other error
    if (request.maxMbps_ % 2) {
        return false;
    }
    image->bitmap_ = {97, 98, 99, 100, 101, 102};
    return true;
}

Let's stop here for a while

What's wrong with this code?
How we can improve it?
Which modern C++ feature we should use here?

void handle(const Download& request, void (*callback)(bool, Image)) {
    Image image;
    for (const auto& pair : cache_) {
        if (pair.second == request.url_) {
            image = pair.first;
            callback(true, image);
            return;
        }
    }
    if (download(request, &image)) {
        if (request.cache_) {
            cache_.push_back(std::pair<Image, std::string>(image, request.url_));
        }
        callback(true, image);
        return;
    }
    callback(false, Image{});
}

bool download(const Download& request, Image* image) {
    if (request.type_ == Download::Ssh && request.credentail_.cert_ != 123) {
        return false;
    }
    if (request.type_ == Download::Telnet && request.credentail_.cert_ != 231) {
        return false;
    }
    // Simulate some other error
    if (request.maxMbps_ % 2) {
        return false;
    }
    image->bitmap_ = {97, 98, 99, 100, 101, 102};
    return true;
}

Now will be only worse!

class RequestHandler {
public:
    enum RequestType { Download, Upload, Remove, Clear };

    void start(Server* server);

    void stop();

    void pushRequest(void* request, RequestType type, void (*callback)(bool, Image));

private:
    void run(Server* server);
    std::tuple<void*, RequestType, void (*)(bool, Image)> waitForRequest();

    bool stop_;
    std::queue<std::tuple<void*, RequestType, void (*)(bool, Image)>> requests_;
};

Implementations (1)

void start(Server* server) {
    std::thread(&RequestHandler::run, this, server).detach();
}

void stop() {
    stop_ = true;
}

void pushRequest(void* request, RequestType type, void (*callback)(bool, Image)) {
    requests_.push(std::tuple<void*, RequestType, void (*)(bool, Image)>(
        request, type, callback));
}

Implementations (2)

    void run(Server* server) {
        while (!stop_) {
            auto tuple = waitForRequest();
            switch (std::get<1>(tuple)) {
            case Download:
                server->handle(*((::Download*)(std::get<0>(tuple))), std::get<2>(tuple));
                break;
            case Upload:
                server->handle(*((::Upload*)(std::get<0>(tuple))), std::get<2>(tuple));
                break;
            case Remove:
                server->handle(*((RemoveFromCache*)(std::get<0>(tuple))), std::get<2>(tuple));
                break;
            case Clear:
                server->handle(*((ClearCache*)(std::get<0>(tuple))), std::get<2>(tuple));
                break;
            }
        }
    }

    std::tuple<void*, RequestType, void (*)(bool, Image)> waitForRequest() {
        while (requests_.empty() || !stop_) {
        }

        auto pair = requests_.front();
        requests_.pop();

        return pair;
    }

The funny part - it actually works!

int main() {
    Server server;
    RequestHandler handler;

    handler.start(&server);

    Download d{"sth.png", 123, 100, true, Download::Ssh};
    handler.pushRequest((void*)(&d),
                        RequestHandler::Download,
                        [](bool succes, Image image) {
                            if (succes) {
                                for (auto el : image.bitmap_) {
                                    std::cout << el << ' ';
                                }
                                std::cout << '\n';
                            } else {
                                std::cout << "FAILED!\n";
                            }
                        });

    std::this_thread::sleep_for(std::chrono::seconds(1));
    handler.stop();
}

Let's fix this ugly code!

For Requests we only need add enum class instead of enum the rest seems ok.

struct Credentail { int cert_; };

struct Download {
    enum class ConnectionType { Telnet,
                          Ssh };

    std::string url_;
    Credentail credentail_;
    int maxMbps_;
    bool cache_;
    ConnectionType type_;
};

struct Upload {
    enum class ConnectionType { Telnet,
                          Ssh };

    std::string url_;
    Image image_;
    Credentail credentail_;
    int maxMbps_;
    int size_;
    ConnectionType type_;
};

struct RemoveFromCache {
    std::string url_;
};

struct ClearCache {
};

Alias and status code

The bool flag is the best option for describing an error

enum class StatusCode {
    Ok,
    WrongUrl,
    CanNotConnect,
    WrongCredential,
    MaximumSizeExceeded,
    MissingImage,
};

Let's add also an aliast for callback

using CallbackType = void (*)(StatusCode, Image);

void handle(const Download& request, CallbackType callback);

Use references and STL algorithms (1)

void handle(const Download& request, CallbackType callback) {
    if (const auto it = findImage(request.url_); it != std::cend(cache_)) {
        callback(StatusCode::Ok, it->first);
        return;
    }

    Image img;
    const auto status = download(request, img);
    if (status == StatusCode::Ok) {
        if (request.cache_) {
            cache_.emplace_back(img, request.url_);
        }
    }

    callback(status, img);
}

Use references and STL algorithms (2)

std::vector<std::pair<Image, std::string>>::const_iterator findImage(const std::string& url) const {
    return std::find_if(cbegin(cache_), cend(cache_),
                        [url](const auto& pair) {
                            const auto& [_, thisUrl] = pair;
                            return thisUrl == url;
                        });
}

StatusCode download(const Download& request, Image& image) const {
    if (request.type_ == Download::ConnectionType::Ssh && request.credentail_.cert_ != 123) {
        return StatusCode::WrongCredential;
    }
    if (request.type_ == Download::ConnectionType::Telnet && request.credentail_.cert_ != 231) {
        return StatusCode::CanNotConnect;
    }

    // Simulate some other error
    if (request.maxMbps_ % 2) {
        return StatusCode::WrongUrl;
    }
    image.bitmap_ = {97, 98, 99, 100, 101, 102};
    return StatusCode::Ok;
}

Use references and STL algorithms (3)

void handle(const Upload& request, CallbackType callback) const {
    if (request.size_ > 100) {
        callback(StatusCode::MaximumSizeExceeded, Image{});
        return;
    }

    callback(upload(request), Image{});
}

void handle(const RemoveFromCache& request, CallbackType callback) {
    if (const auto it = findImage(request.url_); it != std::cend(cache_)) {
        cache_.erase(it);
        callback(StatusCode::Ok, Image{});
        return;
    }

    callback(StatusCode::MissingImage, Image{});
}

Let's fix `void*`

Could be better, but this work and is safe.

using RequestType = std::variant<Download, Upload, RemoveFromCache, ClearCache>;

void pushRequest(const RequestType& request, Server::CallbackType callback) {
    requests_.emplace(request, callback);
}

void run(Server* server) {
    const auto [request, callback] = waitForRequest();
    switch (request.index()) {
    case 0:
        server->handle(std::get<Download>(request), callback);
        break;
    case 1:
        server->handle(std::get<Upload>(request), callback);
        break;
    case 2:
        server->handle(std::get<RemoveFromCache>(request), callback);
        break;
    case 3:
        server->handle(std::get<ClearCache>(request), callback);
        break;
    }
}

Make it thread safe (1)

void start(Server* server) {
    std::thread(&RequestHandler::run, this, server).detach();
}

void stop() {
    stop_ = true;
    cv_.notify_one();
}

void pushRequest(const RequestType& request, Server::CallbackType callback) {
    {
        std::lock_guard lock(m_);
        requests_.emplace(request, callback);
    }
    cv_.notify_one();
}

using QueueType = std::pair<RequestType, Server::CallbackType>;

std::mutex m_;
std::condition_variable cv_;
std::atomic<bool> stop_{false};
std::queue<QueueType> requests_;

Make it thread safe (2)

std::optional<QueueType> waitForRequest() {
    std::unique_lock lk(m_);
    cv_.wait(lk, [&]() { return !requests_.empty() || stop_; });
    if (stop_) {
        return std::nullopt;
    }
    auto pair = requests_.front();
    requests_.pop();

    return pair;
}

Usage actually not change

Server server;
RequestHandler handler;

handler.start(&server);

handler.pushRequest(Download{"sth.png", 123, 100, true, Download::ConnectionType::Ssh},
                    [](Server::StatusCode status, Image image) {
                        if (status == Server::StatusCode::Ok) {
                            for (auto el : image.bitmap_) {
                                std::cout << el << ' ';
                            }
                            std::cout << '\n';
                        } else {
                            std::cout << "FAILED!\n";
                        }
                    });

std::this_thread::sleep_for(std::chrono::milliseconds(10));
handler.stop();

Valgird is almost satisfied

It is suspicious only for a pointer to a server that was created in another thread. It marks it as possibly lost, but we know that we didn't do any allocation, because we created it on the stack, and pass only the address.

==1678528== LEAK SUMMARY:
==1678528==    definitely lost: 0 bytes in 0 blocks
==1678528==    indirectly lost: 0 bytes in 0 blocks
==1678528==      possibly lost: 288 bytes in 1 blocks
==1678528==    still reachable: 0 bytes in 0 blocks
==1678528==         suppressed: 0 bytes in 0 blocks
==1678528== 
==1678528== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

Further improvements

We can have 2 callbacks. One returns a downloaded image and error code, and the second returns only an error code. We can change the name of function handle to download, upload etc.. because inside switch we know which function we call because we know the type of request (there is no polymorphism). So we can avoid unnecessary arguments for image, especially this is important when classes don't have default C'tor. We should only make it as a template to allow the download other things than Images. We can also use std::visit method to avoid switch case.

Better solution - use OOP

This particular example works, but what happens when we extend the program to handle also other types like Videos and audio? We need to extend server interface to handle different types.

    void handle(const DownloadVideo& request, void (*callback)(bool, Video));
    void handle(const UploadVideo& request, void (*callback)(bool, Video));

    void handle(const DownloadAudio& request, void (*callback)(bool, Audio));
    void handle(const UploadAudio& request, void (*callback)(bool, Audio));

    void handle(const DownloadCertificates& request, void (*callback)(bool, Certificates));
    void handle(const UploadCertificates& request, void (*callback)(bool, Certificates));

Server class grow and grow, and now have a lot of resposibilities and depends form implementations of Image, Video, Audio and Certificates this is bad, really bad. We should folow SOLID principles.

Command pattern

Let's create an abstraction and break the dependency between Server and Request. Command object will have only one method operator() that we can call. This operator will perform one action like: Download, Upload, RemoveFormCache, ClearCache.

class Command {
public:
    enum class StatusCode {
        Ok, WrongUrl, CanNotConnect, WrongCredential, MaximumSizeExceeded, MissingImage
    };

    // Rule of 5!
    virtual ~Command() = default;
    Command(const Command&) = default;
    Command(Command&&) = default;
    Command& operator=(const Command&) = default;
    Command& operator=(Command&&) = default;

    virtual void operator()() const = 0;
};

Delegate pattern

Because Command needs to know only how to deal with Files like Image, Video or Audio. We need to create a Delegate that delegates the rest of the actions to the Server class.

class Command {
    class Delegate {
    public:
        virtual ~Delegate() = default;
        virtual StatusCode removeFromCache(const std::string& url) = 0;
        virtual void clearCache() = 0;
        virtual void addToCache(const std::vector<uint8_t>& data, const std::string& url) = 0;
        virtual StatusCode download(std::vector<uint8_t>& data, const DownloadRequest& request) const = 0;
        virtual StatusCode upload(const std::vector<uint8_t>& data, const UploadRequest& request) const = 0;
    };

    explicit Command(Delegate* delegate)
        : delegate_(delegate) {}
    // ...
protected:
    Delegate* delegate_;
};

Server class

Now server-class depends on abstraction which is Delegate and only performs actions with connecting with network and caching values. we can also create another class that will cache values. Class Command depends on abstraction, because we don't need to know which class will implement delegate methods.

class Server : public Command::Delegate {
public:
    ~Server() override = default;

    Command::StatusCode removeFromCache(const std::string& url) override;

    void clearCache() override;

    void addToCache(const std::vector<uint8_t>& data, const std::string& url) override;

    Command::StatusCode download(std::vector<uint8_t>& data, const DownloadRequest& request) const override;

    Command::StatusCode upload(const std::vector<uint8_t>& data, const UploadRequest& request) const override;

private:
    std::vector<std::pair<std::vector<uint8_t>, std::string>>::const_iterator findFile(const std::string& url) const;

    std::vector<std::pair<std::vector<uint8_t>, std::string>> cache_;
};

Easy to extend

Now we can create whatever command we want to.

class UploadImageCommand : public Command {
public:
    using CallbackType = void (*)(StatusCode);

    ~UploadImageCommand() override = default;

    UploadImageCommand(Delegate* delegate, const Image& image, CallbackType callback, const UploadRequest& request)
        : Command(delegate), image_(image), callback_(callback), request_(request) {}

    void operator()() const override {
        callback_(delegate_->upload(image_.bitmap_, request_));
    };

private:
    Image image_;
    CallbackType callback_;
    UploadRequest request_;
};

Download Image command

We can deal with different parameters, and callbacks.

class DownloadImageCommand : public Command {
public:
    using CallbackType = void (*)(StatusCode, Image);

    ~DownloadImageCommand() override = default;

    DownloadImageCommand(Delegate* delegate, CallbackType callback, const DownloadRequest& request)
        : Command(delegate), callback_(callback), request_(request) {}

    void operator()() const override {
        std::vector<uint8_t> data;
        if (auto status = delegate_->download(data, request_); status == Command::StatusCode::Ok) {
            // Do some conversion on vector
            if (request_.cache_) {
                delegate_->addToCache(data, request_.url_);
            }
            Image image{data};
            callback_(status, std::move(image));
        } else {
            callback_(status, Image{});
        }
    };

private:
    CallbackType callback_;
    DownloadRequest request_;
};

Request handler

Request handlers also break their dependency on the server. We don't need to know about Server class anymore. We just push the command on queue and call operator().

class RequestHandler {
public:
    void start() {
        std::thread(&RequestHandler::run, this).detach();
    }

    void stop() {
        stop_ = true;
        cv_.notify_one();
    }

    void pushRequest(std::unique_ptr<Command> command) {
        {
            std::lock_guard lock(m_);
            requests_.push(std::move(command));
        }
        cv_.notify_one();
    }

private:
    void run() {
        while (!stop_) {
            const auto request = waitForRequest();
            if (!request) {
                return;
            }

            (*request)();
        }
    }

    std::unique_ptr<Command> waitForRequest() {
        std::unique_lock lk(m_);
        cv_.wait(lk, [&]() { return !requests_.empty() || stop_; });
        if (stop_) {
            return nullptr;
        }

        std::unique_ptr<Command> request = std::move(requests_.front());
        requests_.pop();

        return request;
    }

    std::mutex m_;
    std::condition_variable cv_;
    std::atomic<bool> stop_{false};
    std::queue<std::unique_ptr<Command>> requests_;
};

Usage

int main() {
    Server server;
    RequestHandler handler;

    handler.start();
    handler.pushRequest(std::make_unique<DownloadImageCommand>(
        &server,
        [](Command::StatusCode status, Image img) {
            if (status == Command::StatusCode::Ok) {
                std::copy(cbegin(img.bitmap_), cend(img.bitmap_), 
                          std::ostream_iterator<uint8_t>(std::cout, " "));
                std::cout << '\n';
            } else {
                std::cout << "Sth went wrong!\n";
            }
        },
        DownloadRequest{"Sth123", Credentail{123}, 200, true, DownloadRequest::ConnectionType::Ssh}));

    std::this_thread::sleep_for(std::chrono::milliseconds(100));
    handler.stop();
}

Valgrind

Now we have easy to extend code, which also satisfies Valgrind.

==1790927== 
==1790927== HEAP SUMMARY:
==1790927==     in use at exit: 0 bytes in 0 blocks
==1790927==   total heap usage: 11 allocs, 11 frees, 74,770 bytes allocated
==1790927== 
==1790927== All heap blocks were freed -- no leaks are possible
==1790927== 
==1790927== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

23 KiB Raw Blame History

Refactoring

Old C++98/11 code

General idea

Requests

Server class

Server class - one of handle method

Server class - download method

Let's stop here for a while

Now will be only worse!

Implementations (1)

Implementations (2)

The funny part - it actually works!

Let's fix this ugly code!

Alias and status code

Use references and STL algorithms (1)

Use references and STL algorithms (2)

Use references and STL algorithms (3)

Let's fix void*

Make it thread safe (1)

Make it thread safe (2)

Usage actually not change

Valgird is almost satisfied

Further improvements

Better solution - use OOP

Command pattern

Delegate pattern

Server class

Easy to extend

Download Image command

Request handler

Usage

Valgrind

23 KiB

Raw Blame History

Let's fix `void*`