GNOME Bugzilla – Bug 705027
GSocket GSource not threadsafe on Windows
Last modified: 2015-06-11 14:42:54 UTC
The requested_conditions list access is not threadsafe. add_condition_watch() can be called from any thread when creating a GSource, remove_condition_watch() can be called from any thread when destroying a GSource. Causes very interesting bugs when doing exactly that from a few different threads all the time. Attached patch fixes this
Created attachment 250305 [details] [review] 0001-GSocket-GSource-not-threadsafe-on-Windows.patch
Comment on attachment 250305 [details] [review] 0001-GSocket-GSource-not-threadsafe-on-Windows.patch >The requested_conditions list access is not threadsafe. add_condition_watch() >can be called from any thread when creating a GSource, remove_condition_watch() >can be called from any thread when destroying a GSource. GIO objects are generally assumed to not be threadsafe unless either (a) their use cases explicitly involve multi-threadedness (eg, GCancellable, GTlsInteraction), or (b) they're effectively global variables (eg, GSettingsBackend, GDBusConnection) At the moment, GSocket is not assumed to be threadsafe, so if these bugs only happen when using it from multiple threads simultaneously, then they're not bugs. OTOH, GSocket is really low level, yada yada yada, so maybe it would be useful if it was threadsafe. But this patch doesn't do that; there are still a handful of other non-thread-safe fields.
I'm fine with GSocket not being threadsafe, using sockets from multiple threads at once is a recipe for disaster anyway... but GSources are assumed to be threadsafe and e.g. could be created and destroyed by any thread. You're right though, the patch is not complete. update_conditions() at least has to be fixed too, and I'll also check again any other socket state used by the GSource later.
Created attachment 251045 [details] [review] 0001-GSocket-GSource-not-threadsafe-on-Windows.patch Everything else in the GSource seems to be threadsafe, it's just this Windows code.
(In reply to comment #3) > I'm fine with GSocket not being threadsafe, using sockets from multiple threads > at once is a recipe for disaster anyway... but GSources are assumed to be > threadsafe and e.g. could be created and destroyed by any thread. Certainly g_source_attach() is thread-safe, but I don't think there's any reason to believe that g_socket_create_source() is exempt from the "objects aren't thread-safe" rule. So if you create a GSocketSource in one thread while doing other socket operations in another, then you lose. Or is that not what you're doing? Can you point me to the code you're running into problems with?
My code works the following way... there's a server socket that accepts connections from a main loop running on the default main context. For each of the client sockets 1) Some bytes are read and send whenever the GSocketSource tells me that I can do that without blocking. The client tells me what it wants here. 2) After I know what the client wants the socket is passed to a GStreamer element that uses a GSocketSource too and reads/writes data from a different thread running a main loop on a different main context. My original code never touches the socket again except for destroying the GSocketSource. So what happens here is that I can easily get the first thread to destroy the first GSocketSource at the same time as the second thread creates the second GSocketSource. And then madness happens, for example the assertion at the top of remove_condition_watch().
well, you can easily *destroy* the source before creating the other thread, but it would be awkward to guarantee it gets *finalized*... ok. In that case, let's make it clear that this is just about making finalization thread-safe, and not anything else. And so in that case, you don't need to lock around ensure_event(). But other than that, the new patch looks fine. (Although the commit message should say that it's just about finalization)
(In reply to comment #7) > well, you can easily *destroy* the source before creating the other thread, but > it would be awkward to guarantee it gets *finalized*... ok. Tried that, but because I'm calling that from the GSource callback it seems that the GSource is actually finalized and destroyed after the callback has returned (although I call destroy() from inside the callback before passing it to the GStreamer element)... which is when the GStreamer element creates the new GSocketSource from the other thread :) > In that case, let's make it clear that this is just about making finalization > thread-safe, and not anything else. And so in that case, you don't need to lock > around ensure_event(). But other than that, the new patch looks fine. (Although > the commit message should say that it's just about finalization) Alright, I just added the lock around ensure_event() to make sure that nothing fails in interesting ways if the two first GSocketSources are created by two different threads at the same time too :) Just to be on the safe side. Would you prefer to have that removed?
(In reply to comment #8) > (In reply to comment #7) > > well, you can easily *destroy* the source before creating the other thread, but > > it would be awkward to guarantee it gets *finalized*... ok. > > Tried that, but because I'm calling that from the GSource callback it seems > that the GSource is actually finalized and destroyed after the callback has > returned (although I call destroy() from inside the callback Right, that's exactly what I was saying. (If you wanted to make sure that the source was finalized before the other thread used the socket, you'd have to queue an idle and start the new thread from there, rather than just doing it directly from GSocketSource callback. Which would be awkward.) > Alright, I just added the lock around ensure_event() to make sure that nothing > fails in interesting ways if the two first GSocketSources are created by two > different threads at the same time too :) Just to be on the safe side. > Would you prefer to have that removed? Yes. We're only guaranteeing that it's safe to finalize a GSocketSource while the socket is being used from another thread. If you create two GSocketSources in two different threads at once, then you lose.
Ok, I will do that then. Thanks :)
Created attachment 252632 [details] [review] 0001-GSocket-GSocketSource-finalizing-not-threadsafe-on-W.patch Like that?
Comment on attachment 252632 [details] [review] 0001-GSocket-GSocketSource-finalizing-not-threadsafe-on-W.patch Yes. Just one tiny nit: >original GSocketSource is destroyed at the same time as the new one "finalized", not "destroyed". Those are two different steps, and it's finalization that's causing the problem. Please commit with that change.
commit ab6b7dbc2efc506e43616941ac70f14fc20de574 Author: Sebastian Dröge <slomo@circular-chaos.org> Date: Sun Jul 28 16:43:44 2013 +0200 GSocket – GSocketSource finalizing not threadsafe on Windows The requested_conditions list access is not threadsafe. When passing the socket ownership from a GSource callback to another thread, which also creates a GSocketSource for the socket, it can happen that the original GSocketSource is finalized at the same time as the new one is created. This would cause inconsistencies in the requested_conditions list and can cause assertions or completely undefined behaviour. https://bugzilla.gnome.org/show_bug.cgi?id=705027