Mosquitto: Incorrect error code to on_disconnect

[igough]

In libs/loop.c, when the PINGRESP has not been received in time and the lib is not in the process of disconnecting, the the rc is being set to 1, which subsequently gets passed to on_disconnect as the disconnect reason. This corresponds to MOSQ_ERR_NOMEM, but the error has nothing to do with failed memory allocation. There does not seem to be a defined return code that corresponds to "timed out" though. Should there be?

int mosquitto_loop_misc(struct mosquitto *mosq)
{
        time_t now;
        int rc;

        if(!mosq) return MOSQ_ERR_INVAL;
        if(mosq->sock == INVALID_SOCKET) return MOSQ_ERR_NO_CONN;

        mosquitto__check_keepalive(mosq);
        now = mosquitto_time();
        if(mosq->ping_t && now - mosq->ping_t >= mosq->keepalive){
                /* mosq->ping_t != 0 means we are waiting for a pingresp.
                 * This hasn't happened in the keepalive time so we should disconnect.
                 */
                net__socket_close(mosq);
                pthread_mutex_lock(&mosq->state_mutex);
                if(mosq->state == mosq_cs_disconnecting){
                        rc = MOSQ_ERR_SUCCESS;
                }else{
                        rc = 1;
                }
                pthread_mutex_unlock(&mosq->state_mutex);
                pthread_mutex_lock(&mosq->callback_mutex);
                if(mosq->on_disconnect){
                        mosq->in_callback = true;
                        mosq->on_disconnect(mosq, mosq->userdata, rc);
                        mosq->in_callback = false;
                }
                pthread_mutex_unlock(&mosq->callback_mutex);
                return MOSQ_ERR_CONN_LOST;
        }
        return MOSQ_ERR_SUCCESS;
}

Note that there is similar incorrect code in libs/util_mosq.c in mosquitto__check_keepalive(). In fact, pretty well all place in the mosquitto code that has "rc = 1" is suspect.

[toast-uz]

It is not a bug, because "rc = 1" means an anonymous error, while MOSQ_ERR_SUCCESS = 1. The error code 1 does not always mean MOSQ_ERR_SUCCESS, while MOSQ_ERR_SUCCESS = 1. The opposite is not necessarily true.

Even if so, I truly agree with you, and I'd like to revise these ambiguous error codes, because MQTTv5 requests more accurate reason code on disconnecting.

[igough]

From mosquitto.h

/* Error values */
enum mosq_err_t {
    MOSQ_ERR_CONN_PENDING = -1,
    MOSQ_ERR_SUCCESS = 0,
    MOSQ_ERR_NOMEM = 1,
    MOSQ_ERR_PROTOCOL = 2,
    MOSQ_ERR_INVAL = 3,
    MOSQ_ERR_NO_CONN = 4,
    MOSQ_ERR_CONN_REFUSED = 5,
    MOSQ_ERR_NOT_FOUND = 6,
    MOSQ_ERR_CONN_LOST = 7,
    MOSQ_ERR_TLS = 8,
    MOSQ_ERR_PAYLOAD_SIZE = 9,
    MOSQ_ERR_NOT_SUPPORTED = 10,
    MOSQ_ERR_AUTH = 11,
    MOSQ_ERR_ACL_DENIED = 12,
    MOSQ_ERR_UNKNOWN = 13,
    MOSQ_ERR_ERRNO = 14,
    MOSQ_ERR_EAI = 15,
    MOSQ_ERR_PROXY = 16
};

I suspect you meant to say that MOSQ_ERR_NOMEM = 1 and not MOSQ_ERR_SUCCESS = 1.

I would have to disagree with your argument that 1 means an anonymous error. Their meanings are, in fact, strictly defined by the mosq_err_t enum. Now, that is not saying that 1 is not erroneously used to mean "I don't have time to look up what the actual error code is that corresponds to this error so I will just use 1 for now and fix it later". We have all been there. But that does not mean that it is not a bug.

[toast-uz]

I agree with you that these are unsofiscated codes, but I don't understand what is an actual problem. Could you show something actual problem by "rc = 1;"?

[igough]

The problem is that it caused me to waste time time tracking down the wrong problem. It led me to investigate why my client was running out of memory when the actual problem was a timeout due to taking too much time to processing a message, which stalls the single-threaded, serialized, message processing loop in mosquitto. And I cannot have been the only person it janked into a wrong direction, simply the first to report it.

So this kind of anti-pattern (https://en.wikipedia.org/wiki/Magic_number_(programming)#Unnamed_numerical_constants), what Stroustup calls "bad style" (https://www.datamation.com/columns/article.php/11079_3789981_2/Bjarne-Stroustrup-on-Educating-Software-Developers.htm) should be stomped out due to its issues with correctness, comprehension and maintainability.

Just leave the bug open and I will eventually get around to fixing it myself.

[toast-uz]

OK, I classified it is not a bug by your insistence.
My definition of a bug is not to operate as external specification. But this issue is not classified as a bug.

I recommend you close this issue and create a new pull request if you would. I say again, I agree with you that these are unsofiscated codes. Therefore, I hope you to create a new pull request.

[karlp]

I'd say it should stay open as a legitimate bug. There's already MOSQ_ERR_UNKNOWN if it's unknown, the error codes should be used . Particularly the example given where both the raw integer (1) _and_ MOSQ_ERR_xxxx codes are used, that's clearly incorrect, the 1 is _wrong_ there.

[toast-uz]

@karlp I agree with you that these are unsofiscated codes. I'd like to classified it is not a bug due to an issue influencing only internally. This place is mainly for the users who search known bugs. To treat everything as a bug fails the target. There are 200+ issues! Therefore, I'd like to concentrate the issues influencing some users actually.

I say again and again, I agree with you that these are unsofiscated codes and should be revised. This is a matter of definition of a bug for me. Please don't insist this is a bug, but insist my definition of a bug is illegal.

[toast-uz]

@igough If you have a plenty time of getting a prize of recoginization of a bug at this place, you'd better make a new pull request. Actually, I submitted a pull request #519 which includes revising a lot of error codes "rc =1" for a year ago.

[karlp]

Look, finding a technicality to just close things doesn't actually help improve quality. It's still a bug. You can mark it as low priority, or add a tag saying that it's been triaged, but I'm strongly opposed to you just closing it and pretending it doesn't exist anymore.

This is an issue tracking system. Is there an issue? yes. Has the issue been resolved? No. Do you want a method to filter this issue out of your search for new issues? sure, add a tag for it.

[toast-uz]

Sorry @karlp , I cannot understand why this issue helps improving ACTUAL quality. Of course, I agree this issue helps some enhancements in the future. But, those optimization is not a bug. Extremely speaking, by my definition of a bug, the typo of document is a more critical bug than this issue, because it influences externally.

If you think this issue is important, you can submit a new pull request. I appreciate if you will.

[toast-uz]

According to Wikipedia:
A software bug is an error, flaw, failure or fault in a computer program or system that causes it to produce an incorrect or unexpected result, or to behave in unintended ways.

[igough]

@toast-uz I will submit a pull request once I get the time to address this issue. I want this issue open to reference in the pull request, and then I can close the issue at the time I submit the pull request. I don't really understand why you want this issue closed without a pull request though. Can you articulate why that is? It is a code quality issue that will not go away without being fixed. Closing it without addressing it signals that the contributors do not care about code quality. Now, it has been my experience that SW developers not caring about code quality is rare, so is there some sort of pressure that is forcing you to want the issue closed without being resolved?

[toast-uz]

This is not the place to be discussed any "issue". This is only for a bug. Other discussion should be at mosquitto-dev ML. Remember the instruction on https://mosquitto.org/ . My recommendation is to start discussion at mosquitto-dev ML, then if the issue has clarified as a bug, move here. If you have the definitely bug, then you can start here. The word "issue" is so ambiguous that every trouble comes here, and left untouched. Therefore, I'd like to clarify it by the instruction on https://mosquitto.org/ . Sorry for the inconvenience.

Anyway, I agree to hold this issue until you can submit a new PR. Because the discussion has been here for a long time.

[ralight]

@igough I'm sorry you've had your time wasted by this confusing return value. This is something I've been gradually fixing and should definitely get round to finishing off. Please don't worry about a PR, I'll sort it.

[ralight]

I've checked over the whole library and believe this is the lot. Could you take a look at the fixes branch and close this issue if you're satisfied?

[igough]

The changes look great Roger. Thank you very much.