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, yeah I wasn’t trying to imply that currently mosquitto isn’t using native windows threads, just that to do so it requires stale code from an external source. I think there’s value in reducing the amount of external code to a project, when said code is not well maintained. In the case of OpenSSL (or libcurl) for example, it makes more sense to include it as a dependency, because this results in better work and less duplicated work across the code bases, but does make building and deploying more complicated.
> 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.
Okay, that makes sense now, thanks for the explanation. So, the best thing to do in mosquitto_destroy (assuming there isn’t any nefarious side effects to this) would be to do this sockpairW trick, as well as set the state to mosq_cs_disconnecting?
> 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.
That’s a good point, I also don’t see much difference at this point between connected and thread_running. I guess the distinction would be if there is a purpose to the thread running if it is disconnected? I can’t think of any reason, because if the client is disconnected from a broker, then it can’t really do anything, and so there wouldn’t be anything for the thread to do anyhow.
> 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.
I will have to revert my changes to the main files in this case, but that’s easy to do. Also, if you take a look at my dev branch again, I have turned all of the functions into defines, which means there needn’t be any additional .c files to compile and link against, it is all done in the preprocessor stage which is cleaner in this case. If we want to just provide definitions of pthread_start, etc. then we have even less code in this file and it becomes simpler yet. You can see said header file with the defines here :
https://github.com/anonymouse64/mosquitto/blob/dev/windows-threading/lib/mosquitto_threading.h
> 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.
I agree it’s unlikely that the pthreads interface will ever change or go away, but I still think there’s value in abstracting out which threading implementation is used. Regardless, this is a minor point of disagreement and I can certainly refactor my code to just provide definitions for the equivalent pthread functions on Windows.
> 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.
I see that now, after looking at it more closely.
> 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.
Okay. I haven’t delved into the client and broker code very much, I have been mostly focusing on the library API code, so I wasn’t sure.
> 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.
Indeed, we have some cool stuff that we want to use MQTT (and hence mosquitto) for.
Cheers,
Roger
_______________________________________________
mosquitto-dev mailing list
mosquitto-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/mosquitto-dev
