Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set MQTT_PAL_MUTEX_INIT to use a recursive mutex #128

Closed
wants to merge 1 commit into from
Closed

Set MQTT_PAL_MUTEX_INIT to use a recursive mutex #128

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion include/mqtt_pal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,13 @@ extern "C" {
typedef time_t mqtt_pal_time_t;
typedef pthread_mutex_t mqtt_pal_mutex_t;

#define MQTT_PAL_MUTEX_INIT(mtx_ptr) pthread_mutex_init(mtx_ptr, NULL)
#define MQTT_PAL_MUTEX_INIT(mtx_ptr) { \
pthread_mutexattr_t attr; \
Copy link
Owner

@LiamBindle LiamBindle Mar 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

@LiamBindle LiamBindle Mar 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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*).

Copy link
Owner

@LiamBindle LiamBindle Mar 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

pthread_mutexattr_init(&attr); \
pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE); \
pthread_mutex_init(mtx_ptr, &attr); \
}
//#define MQTT_PAL_MUTEX_INIT(mtx_ptr) pthread_mutex_init(mtx_ptr, NULL)
#define MQTT_PAL_MUTEX_LOCK(mtx_ptr) pthread_mutex_lock(mtx_ptr)
#define MQTT_PAL_MUTEX_UNLOCK(mtx_ptr) pthread_mutex_unlock(mtx_ptr)

Expand Down