eclipse / mosquitto

Eclipse Mosquitto - An open source MQTT broker
https://mosquitto.org
Other
8.93k stars 2.37k forks source link

Publishing message will in callback #1175

Open JimmyBjorklund opened 5 years ago

JimmyBjorklund commented 5 years ago

Hi I am trying to publish messages directly when I am in a callback as I do not want the process to be interrupted but I want status messages to be sent during the progress. Found that the mosquitto_loop_write deadlocks in the packet__write function as it tires to lock the mosq->callback_mutex that is already locked. One solution is to add a check to se if we already is in a callback and skip this section, don't know enouth about mosquittos internals to know if this is the right way to goo. but it seams to work.

packet_mosq.c

if(((packet->command)&0xF6) == PUBLISH)
{
    G_PUB_MSGS_SENT_INC(1);
#ifndef WITH_BROKER
    if( mosq->in_callback != true ) // << New test 
    {
        pthread_mutex_lock(&mosq->callback_mutex);
        if (mosq->on_publish) 
        {
            /* This is a QoS=0 message */
            mosq->in_callback = true;
            mosq->on_publish(mosq, mosq->userdata, packet->mid);
            mosq->in_callback = false;
        }
        pthread_mutex_unlock(&mosq->callback_mutex);
   }
karlp commented 5 years ago

Could you share some example code that demonstrates the lockup? There's "quite a few" people publishing in callbacks without problems, so I'm curious what's different in your setup.

JimmyBjorklund commented 5 years ago

First of it normally works sending the message from the callback as i do not try to force the message out, then the message is send when i return out to the main loop. but in this case i want the message to be sent instantly.

This is a cut out but it shows the scenario.

 ret = mosquitto_publish(mqtt_helper_handles[handle].mosq, NULL, topic, payload_len, payload, 0, false);
  if (ret)
  {
        dbgmsg(DBG_GRP_MQTT, DBG_PRINT_ERROR, "Can't publish to Mosquitto server\n");
        exit(-1);
    }

    int rtn = MOSQ_ERR_SUCCESS;
    while( mosquitto_want_write(handle->mosq) )
    {
        rtn = mosquitto_loop_write(handle->mosq,10);
        if( rtn != MOSQ_ERR_SUCCESS ) 
        {
            dbgmsg(DBG_GRP_DEVELOP, DBG_PRINT_WARNING, "Connection lost\n");
            return rtn;
        }
    }
Laro88 commented 5 years ago

I had a similar issue with mosquittopp (c++) We ended up with a rule that we never publish from within the handling thread itself, but instead use another thread to perform any publish activity if needed. It goes well with the our general design principle of handling events as fast as possible - so there should not be any publish inside reception/handling of events.

karlp commented 5 years ago

right, so it's not just that you're calling mosuqitto_publish, it's that you're trying to call the loop() handlers as well I can't personally even imagine why you thought that would work at all :) I'll leave someone else to decide what they think should be done, but I'd suggest having a better managed main loop if you need to do "high priority" instant returns.

JimmyBjorklund commented 5 years ago

Is there any other way to get a message sent ? I am not interested in doing anything else then pushing the message out.

ralight commented 5 years ago

Not at the moment. The intention is very much that you spend as short a time as possible in the callback.