karastojko / mailio

mailio is a cross platform C++ library for MIME format and SMTP, POP3, IMAP protocols. It is based on the standard C++ 17 and Boost library.
Other
380 stars 101 forks source link

Exceptions seem to lack information #56

Open tsilia opened 3 years ago

tsilia commented 3 years ago

Hello. Exceptions in mailio often don't seem to be very informative, e.g.: throw message_error("Parsing failure of the message ID."); or throw pop3_error("Parser failure."); which unfortunately don't say much about the value that caused this exception or the reason.

Perhaps you could somehow extend exception interfaces to allow saving and retrieving additional info which could be the exact value that failed to parse or at least some context that could help to track down the issue if the exact value isn't available at the time when exception is thrown? It probably would be better if there was another parameter for message_error (and the likes) constructor, like const std::string& token or const std::string& context which could also be retrieved in exception handler using a corresponding method, like get_error_token() or get_context(). This way it doesn't break anything for anyone because all who wants to have nice and clean human-readable messages will still get those with no edits required on their side. It would require editing of the library code though.

An example implementation:

class message_error : public std::runtime_error
{
    ...
    explicit message_error(const std::string& msg, const std::string& context)
        : std::runtime_error(msg)
        , context_(context)
    {
    }
    ...
    std::string get_context() const
    {
        return context_;
    }
}

...
// later in the library code
throw message_error("Parsing failure of the message ID.", header_value);

// later in the user code
try
{
    ...
}
catch (const mailio::message_error& ex)
{
    std::cerr << ex.what() << " " << ex.get_context() << std::endl;
}

Also, it would probably be better to store both the invalid token AND the context in which it appeared because there are cases when few case blocks of switch produce exceptions with the same message but for different states of the parser, or the cases like this one when you throw pop3_error and we lose the original exception reason: https://github.com/karastojko/mailio/blob/f82f7e8e0d9f1c2794a6960e409ddc4899e4881d/src/pop3.cpp#L127-L134 In such cases it probably could be better to save ex.what() as the exception context similarly to what was described above.

karastojko commented 3 years ago

You are right, I will address the issue in the future.

firedaemon-cto commented 2 years ago

In this sense I wonder what's the design decision behind catching asio (ssl) errors and rethrow them as mailio errors? Because asio is such a vital core part of the communication layer, we definitely need the underlying communication error. I personally would let asio errors through to the caller, as they are std::system_errors these days anyway.

karastojko commented 2 years ago

It was meant to move them away as low level errors. But from your and some other comments, I see there is a need to trace these errors as well the errors received from servers. Thus, I plan to extend the mailio exceptions with these additional data. In this version I am dealing with the various encoding issues, so probably I'll let the error handling for the next version.

mr-j0nes commented 1 month ago

After 2 and half years this still is very much wanted.

karastojko commented 3 weeks ago

Thanks for pushing this. There is an ongoing refactoring on the codec_folding branch regarding the header folding and policy. Once I finish it (hopefully in the following weeks), the version 0.24 will be concluded. Then I can address this one and few more topics, some of them are already in PRs. So, I put the label Enhancement, so I could prioritize it for the version 0.25. Thank you for the patience.