Set MQTT_PAL_MUTEX_INIT to use a recursive mutex

[Monarda]

This is pretty clearly an alternate take on @basilaljamal 's pull request #123, and an attempt at a solution to issue #88.

I've moved all the code changes into mqtt_pal.h and made it platform-specific to POSIX platforms, by redefining MQTT_PAL_MUTEX_INIT. But I'm not sure if this is the right approach. Should MQTT_PAL_MUTEX_INIT be converted into a function instead of a multiline define? Should a define switch be used to control whether recursive mutexes are used?

#ifndef MQTT_USE_MUTEX_RECURSIVE
#define MQTT_PAL_MUTEX_INIT(mtx_ptr) pthread_mutex_init(mtx_ptr, NULL)
#else
#define MQTT_PAL_MUTEX_INIT(mtx_ptr) { ... }
#endif

Happy for feedback!

[LiamBindle]

My concern here is attr is declared in the scope of mqtt_init() or mqtt_reconnect(). I think pthread_mutexattr_t might need to come from the user's program's scope (i.e., the main function's scope, or the same scope as mqtt_client's definition) for this to be safe.

Edit: see my third suggestion below (the third comment)

What about if the UNIX/APPLE PAL defines a function like mqtt_pal_init_pthread_mutex_recursive(struct mqtt_client *, pthread_mutexattr_t*) that:

1. Destroys the mutex created by mqtt_init() (i.e., the one initialized here)
2. Creates a new mutex with &client->mutex and the pthread_mutexattr_t* passed from the user? This new mutex would be initialized as the recursive mutex.

Then users that want this functionality can do

pthread_mutexattr_t my_mutex_attr;
struct mqtt_client my_client;
mqtt_init(&client, ...);
mqtt_pal_init_pthread_mutex_recursive(&client, &my_mutex_attr);

Open to suggestions and ideas! Happy to iterate a few times.

[LiamBindle]

An alternative would be changing the MQTT_PAL_MUTEX_INIT() interface. It could take two arguments: a mqtt_pal_mutex_t* and a mqtt_pal_mutex_attr_t*. All the PAL's would need to add the new mutex attr type. Since their MQTT_PAL_MUTEX_INIT wouldn't use it, the other PALs that don't have a similar attribute type could assign it a dummy type (e.g. void*).

[LiamBindle]

A third option came to mind. What about (in the UNIX/APPLE PAL):

struct mqtt_pal_mutex_t {
    pthread_mutex_t  mutex;
    pthread_mutexattr_t attr;
};

The MQTT_PAL_MUTEX_INIT() macro is passed a pointer to mqtt_pal_mutex_t, so MQTT_PAL_MUTEX_INIT() could then be

 #define MQTT_PAL_MUTEX_INIT(mtx_ptr) { \
        pthread_mutexattr_init(&mtx_ptr->attr); \
        pthread_mutexattr_settype(&mtx_ptr->attr, PTHREAD_MUTEX_RECURSIVE); \
        pthread_mutex_init(&mtx_ptr->mutex, &mtx_ptr->attr); \
    }

This is the cleanest solution IMO.

[Monarda]

That last option does look like a very clean solution.

Is it worth including an option on whether to use recursive mutexes at all?

#ifndef MQTT_USE_MUTEX_RECURSIVE
#define MQTT_PAL_MUTEX_INIT(mtx_ptr) pthread_mutex_init(mtx_ptr, NULL)
#else
#define MQTT_PAL_MUTEX_INIT(mtx_ptr) { \
        pthread_mutexattr_init(&mtx_ptr->attr); \
        pthread_mutexattr_settype(&mtx_ptr->attr, PTHREAD_MUTEX_RECURSIVE); \
        pthread_mutex_init(&mtx_ptr->mutex, &mtx_ptr->attr); \
    }
#endif

Or alternatively the flag could be the other way round, e.g. MQTT_USE_MUTEX_DEFAULT, if the recursive mutex is the default. (Possibly a better name for the flag is needed if the default is not going to be PTHREAD_MUTEX_DEFAULT.)

[LiamBindle]

I don't see a reason to not fully switch to recursive mutexes for UNIX/APPLE. Do you have a concern about it?

I supose it could lead to confusion if people share their UNIX/APPLE message callback with a different platform. That said, I think this could be considered a "feature" of the UNIX/APPLE PAL, but not something MQTT-C supports in general.

[Monarda]

No objections from me. It was just my own relative lack of experience with these kind of mutexes.

[yamt]

does this mean to change the semantics of the pal api?

[LiamBindle]

@yamt This shouldn't change the semantics of the PAL. It would prevent a deadlock in the default Unix/APPLE PALs if one tries to call mqtt_publish() in their receive callback.

That said, I think the receive callbacks should be thought of as interrupts (fast call that essentially mark a received message) rather than heavy-duty functions, so I'm a bit hesistant to "fix" this.

I'm going to close this due to inactivity.