Re: [gtk-list] thread safety broken? (and Changes-1.2.txt)

[Owen Taylor <otaylor redhat com>]

"Charles 'Buck' Krasic" <krasic@cse.ogi.edu> writes:

> Disclaimer: please don't take any of this as ragging on developers.I'm
>             grateful for your efforts.
> 
> I'm working on a multithreaded application, an mpeg video player, that
> I've been migrating to gtk for some time now.
> 
> About four or five months ago, I started out with gtk 1.0.  I'm not an
> experienced X programmer, so when I started getting dreaded 'Xlib:
> async reply error', I rapidly retreated to the most conservative
> approach that various faqs, newsgroup postings, etc. described: I put
> all gui activity into a single thread, and I had other threads use
> that thread as an intermediary for any gui work.  This was reliable,
> but added a lot of ugliness to my code.
> 
> A few weeks ago, I read something that indicated the 1.1 series was
> incorporating better thread support.  Then I read Changes-1.2.txt.  I
> thought "great! this will let me really clean up my code".  So I
> proceeded to try and move to 1.1 with threads.
> 
> My experience so far has not been good.  I desperately wanted to make
> this work.  I spent a lot of time pulling my hair out searching
> mailing mailing list archives, FAQs, source, etc.  I've been stubborn
> about reverting back to my 1.0 model because it's truly a pain in the
> butt.  .

Please realize that thread support is new, somewhat experimental,
and will only become stable if people test it. If you
have problems, please don't pull out your hair - instead
post here, and help make GTK+ better for everyone.
 
> I often got deadlocks because the glib main loop lock and the gdk
> global mutex were not acquired in the same order.  I've concluded for
> now that thread safety in gtk+ simply isn't there yet.

Well, the deadlocks were a small and known-about problem; 
I thought we had fixed it last week, but it turned out
that all the necessary changes never got into CVS. I
think think that things should be OK now
 
> I found out about XThreadsInit, which seems to work well in covering
> the XAsyncReply problems.

If you are getting XAsyncReply problems, then you are not 
using gdk_threads_enter()/leave() properly. Since GTK+/GDK has 
non-thread safe internals, and since normally all access to X is
through GDK, if you get an XAsyncReply, then you are also
risking corrupted data structures in GTK+ or GDK.
 
> For internal thread safety of gtk+/gdk/glib, I resort to using a mutex
> to guard all access to gtk+/gdk/glib, and having only a single thread
> invoke gtk_main.  
> 
> I do *not* call g_thread_init(), because doing so means that gtk+ will
> deadlock even if I protect all my gui access with a mutex.  (This is
> because interaction between gtk_main, which accesses the gdk and glib
> mutex, and whatever gtk function my other threads might happen to
> run).  By not calling g_thread_init(), the gdk mutex is not activated.
> So far I'm OK, although I expect there are still nasty races between
> the thread that invokes gtk_main and the rest.

Note that this type of "simple-minded" threading has various
problems. The biggest problem is that idles queued from 
anything but the main thread will not wake up the main
thread. This causes redraws and resizes not to work properly, 
since they are done in idle queues.
 
> I'm curious about whether I've made a mistake in drawing these
> conclusions.  If not, I'd suggest amending Changes-1.2.txt with a
> warning to prevent other newbies from wasting time in finding out the
> same kind of stuff.

Yes and no. Yes, you've found some problems, but, no I don't
think they are as fundemental as you seem to think they
are. I hope that the hour I spent on it today fixed these 
problems.
 
> I also noticed the following errors in the Changes-1.2.txt:
> 
> >   - You must call g_thread_init(), then gtk_thread_init()
> >     in a threaded GTK+ program.
> 
> gtk_thread_init doesn't exist anymore, right?  It looks like gdk_init
> (invoked by gtk_init) automatically detects threads when g_thread_init 
> has been called first.

Yes. gtk_thread_init() never existed. I wrote that line before
the thread support was finalized, and never updated it.
 
> >   - Idles, timeouts, and input functions are executed outside 
> >     of the main GTK+ lock. So, if you need to call GTK+ 
> >     inside of such a callback, you must surround the callback
> >    with a gtk_thread_enter()/gtk_thread_leave() pair.
> 
> It's gdk_thread_enter() and gdk_thread_leave(), right?

Yes. Thanks for pointing it out.
 
> >      b) Link with the libraries returned by:
> > 
> >           gtk-config --libs gthread
>    
> 
> My gtk-config (from 1.1.11-1.1.13) doesn't understand anything about
> gthread, although glib-config does.

It will in 1.1.15. 
 
> Also, given the above observations, I tried to use testthreads.c and
> found that it confirmed that gtk and threads are broken.  

Unfortunately, it is hard for us to supply a working
testthreads.c, because glib does not supply a thread
creation primitive. (We felt that since this was
not needed for GLib or GTK+ thread-safety, we'd initially
avoid portability problems by not providing one)

The following is a working version of testthreads.c
for pthreads systems. Compile it like:

gcc -o testthreads `gtk-config --libs gthread` testthreads.c \
       `gtk-config --cflags gthread`

Thanks for your report. I hope that threading in GTK+
will be a more pleasant experience for you in the
future.:-)
                                        Owen

=====
/* GTK - The GIMP Toolkit
 * Copyright (C) 1995-1997 Peter Mattis, Spencer Kimball and Josh MacDonald
 *
 * This library is free software; you can redistribute it and/or
 * modify it under the terms of the GNU Library General Public
 * License as published by the Free Software Foundation; either
 * version 2 of the License, or (at your option) any later version.
 *
 * This library is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
 * Library General Public License for more details.
 *
 * You should have received a copy of the GNU Library General Public
 * License along with this library; if not, write to the
 * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
 * Boston, MA 02111-1307, USA.
 */
#include <stdio.h>
#include <unistd.h>
#include <gtk/gtk.h>

#include <pthread.h>

static int nthreads = 0;
static pthread_mutex_t nthreads_mutex = PTHREAD_MUTEX_INITIALIZER;

void
close_cb (GtkWidget *w, gint *flag)
{
  *flag = 1;
}

gint
delete_cb (GtkWidget *w, GdkEvent *event, gint *flag)
{
  *flag = 1;
  return TRUE;
}

void *
counter (void *data)
{
  gchar *name = data;
  gint flag = 0;
  gint counter = 0;
  gchar buffer[32];
  
  GtkWidget *window;
  GtkWidget *vbox;
  GtkWidget *label;
  GtkWidget *button;

  gdk_threads_enter();

  window = gtk_window_new (GTK_WINDOW_TOPLEVEL);
  gtk_window_set_title (GTK_WINDOW (window), name);
  gtk_widget_set_usize (window, 100, 50);

  vbox = gtk_vbox_new (FALSE, 0);

  gtk_signal_connect (GTK_OBJECT (window), "delete_event",
		      GTK_SIGNAL_FUNC (delete_cb), &flag);

  gtk_container_add (GTK_CONTAINER (window), vbox);

  label = gtk_label_new ("0");
  gtk_box_pack_start (GTK_BOX (vbox), label, TRUE, FALSE, 0);

  button = gtk_button_new_with_label ("Close");
  gtk_signal_connect (GTK_OBJECT (button), "clicked",
		      GTK_SIGNAL_FUNC (close_cb), &flag);
  gtk_box_pack_start (GTK_BOX (vbox), button, FALSE, FALSE, 0);

  gtk_widget_show_all (window);

  /* Since flag is only checked or set inside the GTK lock,
   * we don't have to worry about locking it explicitly
   */
  while (!flag)
    {
      sprintf(buffer, "%d", counter);
      gtk_label_set (GTK_LABEL (label), buffer);
      gdk_threads_leave();
      counter++;
      /* Give someone else a chance to get the lock next time.
       * Only necessary because we don't do anything else while
       * releasing the lock.
       */
      sleep(0);
      
      gdk_threads_enter();
    }

  gtk_widget_destroy (window);

  pthread_mutex_lock (&nthreads_mutex);
  nthreads--;
  if (nthreads == 0)
    gtk_main_quit();
  pthread_mutex_unlock (&nthreads_mutex);

  gdk_threads_leave();

  return NULL;
}

int 
main (int argc, char **argv)
{
  int i;

  g_thread_init (NULL);
  gtk_init (&argc, &argv);

  pthread_mutex_lock (&nthreads_mutex);

  for (i=0; i<5; i++)
    {
      char buffer[5][10];
      pthread_t thread;
      
      sprintf(buffer[i], "Thread %i", i);
      if (pthread_create (&thread, NULL, counter, buffer[i]))
	{
	  fprintf(stderr, "Couldn't create thread\n");
	  exit(1);
	}
      nthreads++;
    }

  pthread_mutex_unlock (&nthreads_mutex);

  gdk_threads_enter();
  gtk_main();
  gdk_threads_leave();
  fprintf(stderr, "Done\n");

  return 0;
}