why is this simple test program not thread-safe?

[Paul Davis]

the following simple test program will randomly crash (always in the
same place). the backtrace makes it seem reasonably likely that
sigc::signal::emit() is not thread safe - i.e. an application must
ensure that only a single thread is emitting the same signal at one
time.

if true, this is a major setback to my understanding of sigc++. i knew
that it was not threadsafe to connect new slots to the signal without
mutexes, but i was under the impression that emission was safe.

--p

compile with: cc -g -o sigctest sigctest.cc `pkg-config --cflags --libs
glibmm-2.4` `pkg-config --cflags --libs gthread`



------------------------

#include <sigc++/signal.h>
#include <pthread.h>
#include <stdio.h>
#include <stdlib.h>
#include <glibmm/thread.h>

sigc::signal<void,std::string,int> FireMe;
Glib::StaticMutex SyncObject;

void
listener (std::string str, int i)
{
        static int firings;
        Glib::Mutex::Lock lm (SyncObject);
        firings++;
}

void*
signaller (void* arg)
{
        std::string str = (char*) arg;

        while (1) {
                usleep (rand()%100);
                FireMe (str, 1);
        }
        return 0;
}

int
main (int argc, char* argv[])
{
        const char* first_arg = "first";
        const char* second_arg = "second";
        pthread_t first_thread;
        pthread_t second_thread;

        g_thread_init (NULL);

        FireMe.connect (sigc::ptr_fun (listener));

        pthread_create (&first_thread, 0, signaller, (void *) first_arg);
        pthread_create (&second_thread, 0, signaller, (void *) second_arg);

        sleep (-1);
}


_______________________________________________
libsigc-list mailing list
libsigc-list@...
http://mail.gnome.org/mailman/listinfo/libsigc-list

[Chris Vine]

On Thu, 06 Nov 2008 22:44:12 +0100
Paul Davis <paul@...> wrote:

>
> the following simple test program will randomly crash (always in the
> same place). the backtrace makes it seem reasonably likely that
> sigc::signal::emit() is not thread safe - i.e. an application must
> ensure that only a single thread is emitting the same signal at one
> time.
>
> if true, this is a major setback to my understanding of sigc++. i knew
> that it was not threadsafe to connect new slots to the signal without
> mutexes, but i was under the impression that emission was safe.

[snip]

You have a static (shared) signal object.  It couldn't be thread safe,
since it doesn't have any locking.  It is just like any other class
object with data members (ie which operates on non-local data).
sigc::signal<>::emit() will amongst other things access the std::list
object maintained by the signal object to call operator() on the slot
object the list object holds (the slot object representing listener()).

The other issue (discussed in earlier exchanges of postings) is that
signaller() has a C++ linkage specification but is passed to
pthread_create( ) which has a C linkage specification for its function
pointer argument - but this is not a problem with g++.

Chris

_______________________________________________
libsigc-list mailing list
libsigc-list@...
http://mail.gnome.org/mailman/listinfo/libsigc-list

[Paul Davis]

On Fri, 2008-11-07 at 00:20 +0000, Chris Vine wrote:

>
> You have a static (shared) signal object.  It couldn't be thread safe,
> since it doesn't have any locking.  It is just like any other class
> object with data members (ie which operates on non-local data).
> sigc::signal<>::emit() will amongst other things access the std::list
> object maintained by the signal object to call operator() on the slot
> object the list object holds (the slot object representing listener()).

this much i understood :) however, my assumption was that anything which
manipulated the slot list would be thread-unsafe, but that read-only
access (ie. traversing the list and invoking the slots contained
therein) would be thread-safe to the extent that the code invoked by the
slots is (in my real program and this test, that part is true). i do not
understand why emit() would need to modify the list of slots in any way,
and thus i don't see why it could not be thread-safe on the condition
that the slot list is not modified during the "period in question".

it is certainly possible to have two threads traverse a std::list<>
simultaneously without any issues at all - its not clear why emit has to
do anything more than this. i'd really like to understand why it
apparently does.

--p



_______________________________________________
libsigc-list mailing list
libsigc-list@...
http://mail.gnome.org/mailman/listinfo/libsigc-list

[Chris Vine]

On Fri, 07 Nov 2008 07:35:52 +0100
Paul Davis <paul@...> wrote:

> On Fri, 2008-11-07 at 00:20 +0000, Chris Vine wrote:
>
> >
> > You have a static (shared) signal object.  It couldn't be thread
> > safe, since it doesn't have any locking.  It is just like any other
> > class object with data members (ie which operates on non-local
> > data). sigc::signal<>::emit() will amongst other things access the
> > std::list object maintained by the signal object to call operator()
> > on the slot object the list object holds (the slot object
> > representing listener()).
>
> this much i understood :) however, my assumption was that anything
> which manipulated the slot list would be thread-unsafe, but that
> read-only access (ie. traversing the list and invoking the slots
> contained therein) would be thread-safe to the extent that the code
> invoked by the slots is (in my real program and this test, that part
> is true). i do not understand why emit() would need to modify the
> list of slots in any way, and thus i don't see why it could not be
> thread-safe on the condition that the slot list is not modified
> during the "period in question".
>
> it is certainly possible to have two threads traverse a std::list<>
> simultaneously without any issues at all - its not clear why emit has
> to do anything more than this. i'd really like to understand why it
> apparently does.

The C++ standard doesn't guarantee you can traverse a std::list<>
object simultaneously in two different threads, though, although
I agree usually you can. The signal_impl struct also has other data
members than just the slot list, and it has its own iterator type which
possibly causes something somewhere to keep state (it is a while since
I looked at it).

I cannot now even recall whether operator()() for any one slot is thread
safe as between different threads invoking that method on the same
slot (which your example does). There is no particular reason to think
it would not be, but I would want to look at the code before assuming
that it is. Slot creation and destruction is definitely problematic
though if the slot represents a method of an object derived from
sigc::trackable, as sigc::trackable is very much not thread safe - that
would be an issue if the sigc::signal<> object keeps its slot_base
objects by value, which I think it does, because every time a slot is
extracted from the std::list object copies are created and destroyed.
However, again that is not relevant to your example as you used global
functions as callbacks.

Basically I always assume that any class object which maintains data
members is not thread safe until I have looked at the code and found
that it is (or it is reliably documented as thread safe).

Chris

_______________________________________________
libsigc-list mailing list
libsigc-list@...
http://mail.gnome.org/mailman/listinfo/libsigc-list