Re: [mosquitto-dev] Windows Threading support for libmosquitto

[Roger Light <roger@xxxxxxxxxx>]

On Mon, Feb 29, 2016 at 1:47 AM, Ian Johnson <ijohnson@xxxxxxxxxxx> wrote:

> The main problem I am trying to solve is removing the dependency on pthreads
> on Windows platforms. In my opinion, not having to link against another
> library, pthread-win32 in this case, is preferable when there is support for
> threading provided natively. Additionally, most of the pthread
> implementations are quite old code bases and not well maintained.

The final point is the most important bit and I do agree. I'm quite
surprised that there isn't more effort to keep them up to date.

Ultimately the pthreads-win32 library and the approach that you are
proposing are doing the same thing: providing a wrapper to native
Windows threading. Whether the code to support threading is internal
to this project or linked from another library is almost neither here
nor there. Put another way, mosquitto already uses native threading on
Windows, just through the pthreads-win32 wrapper.

> Right, as I mentioned I only see pthread_cancel used in two functions. One,
> in mosquitto_loop_stop, with the force variable set to true, and in
> mosquitto_destroy. I understand that you do not want to directly call
> mosquitto_disconnect inside mosquitto_destroy, but what about just setting
>
> mosq->state = mosq_cs_disconnecting;
>
> inside mosquitto_destroy?

This does look like a possibility - it would need some testing to make
sure nothing untoward happened, and would need to use the sockpairW
trick to make sure there wasn't a big delay in select().

> I also notice that
> right above the call to pthread_cancel in mosquitto_thread_stop is this
> block of code:
>
>     /* Write a single byte to sockpairW (connected to sockpairR) to break
> out
>      * of select() if in threaded mode. */
>     if(mosq->sockpairW != INVALID_SOCKET){
> #ifndef WIN32
>         if(write(mosq->sockpairW, &sockpair_data, 1)){
>         }
> #else
>        send(mosq->sockpairW, &sockpair_data, 1, 0);
> #endif
>      }
>
> I’m wondering what this does?

sockpairR and W are connected to one another, so writing to sockpairW
sends data to sockpairR. select() is called with a large delay (less
than keepalive) so that there is no need to process the network loop
when there is no data to send/receive. This is fine for incoming data
because as soon as data arrives it triggers the select() call to exit.
For outgoing messages though, the other thread can call publish() at
any point - and could be waiting for up to keepalive seconds until the
network thread wakes up. sockpairR is included in the set of sockets
that select() inspects, so when we want to break out of the select()
call we just write something to sockpairW.

> Regardless of how that works, it makes me wonder if a plausible
> implementation strategy for this might just be to add another state to
> mosq->state, that’s thread_running or something, and if true it continues to
> run the thread, if not it exits the thread safely without doing any kind of
> disconnect sent to the broker, or any other kind of network activity. It
> could be a similar kind of check as to this at the end of
> mosquitto_loop_forever :
> https://github.com/anonymouse64/mosquitto/blob/master/lib/mosquitto.c#L1058.

Adding some sort of flag is probably a better approach than using
pthread_cancel anyway. I'm not entirely sure what the difference
between the "connected" state and "thread_running" state would be.
Overall, I think there's mileage here it just needs a bit more
thinking about.

> So you would rather I have a “pthreads.h” and “pthreads.c” file and not
> define a new type, but rather on windows just provide implementations for
> pthreads?

Yes that is correct.

> That’s fine, but personally I view it as more maintainable to be
> able to change all threading implementations by just changing the
> implementations of a few functions in one file than a couple hundred lines
> across a bunch of files iif for some reason down the line there ever becomes
> a need for some other threading implementation. This also ties the library
> to the threading model that pthreads uses, so again if further down the line
> when someone else is maintaining the project and wants to use a different
> threading model they have to recode lots of parts, rather than just one
> file.

My view is that pthreads is a well understood interface, so why not
use it. I've had plenty of situations where I've expended effort
coding for lots of possible future scenarios and they mostly just
don't come true. If pthreads falls out of favour then the code can be
rewritten at that point.

> This to me is the same reason why one would have _mosquitto_malloc and
> such functions.

This is a slightly different scenario - the memory functions do more
than normal malloc.

> If there are changes necessary for the broker side, I don’t see much
> difficulty in configuring it to use mosquitto_thread_t and such, so perhaps
> if that’s a concern I can contribute changes to that as well.

The broker doesn't use threading yet so any changes should break the
build. Using pthreaddummy.h help there.

> Yes, I am indeed working on this for work, we are working on integrating
> MQTT support into our main software product Mathematica.

Ace, that's very interesting.

Cheers,

Roger