GNOME Bugzilla – Bug 157310
[patch] gdk-pixbuf thread-unsafe loader locking
Last modified: 2010-07-10 04:06:22 UTC
Hi, This patch adds transparent locking around image loaders which don't export the GDK_PIXBUF_FORMAT_THREADSAFE flag. It differs from my original proposal here: http://mail.gnome.org/archives/gtk-devel-list/2004-November/msg00007.html in in that there is only one global lock for thread-unsafe loaders, rather than one per loader. I'm not sure the extra complexity to have a lock per loader would be worth it - it would only help in the case where an application is simultaneously loading two different thread-unsafe image formats. I noticed while working on this that the tiff loader has its own internal lock for global data structures to work around the TIFF API; this patch also removes that lock as it is now duplicating the core lock. Since PNG seems to hold up well in my multithreaded torture test, this patch also adds the _THREADSAFE flag to PNG. Hopefully people more knowledgeable in the other formats can do some testing and add the flag.
Created attachment 33417 [details] [review] generic loader threadsafety
Matthias and I talked about this today, and we realized that the locking is actually insufficient (in general) in the presence of incremental loading. If the library keeps static data around relating to the current image, then one thread can start an incremental load, initialize that data, then leave gdk-pixbuf, and a second thread enters, and modifies the static data. The solution is to add a lock also in gdk-pixbuf-loader.c. Once it's determined the module for a file is thread-unsafe, we lock a global lock before the begin_load method is called, and unlock it only when the loader is closed (i.e. when priv->closed becomes TRUE). I'll update the patch tonight.
Created attachment 33453 [details] [review] generic loader threadsafety, version 2 Ok, I added locking to GdkPixbufLoader and GdkPixbufAnimation. This patch is slightly bigger in some areas because I also fixed indentation in two functions, since 70% of the function was being changed anyways.
Just a note: jpeg appears to hold up quite well under my torture test too. We can probably mark it as threadsafe.
2004-11-12 Matthias Clasen <mclasen@redhat.com> Changes to make gdk-pixbuf threadsafe (#157310, #157306, Colin Walters): * gdk-pixbuf-io.h (enum GdkPixbufFormatFlags): Add GDK_PIXBUF_FORMAT_THREADSAFE to indicate that an image loader is threadsafe. * gdk-pixbuf-io.c (get_file_formats, _gdk_pixbuf_load_module): Use a lock to make initialization of global data structures threadsafe. * gdk-pixbuf-private.h: * gdk-pixbuf-io.c (_gdk_pixbuf_lock, _gdk_pixbuf_unlock): Auxiliary functions which use another lock to protect threadunsafe image loaders. * gdk-pixbuf-io.c (gdk_pixbuf_real_save): (save_to_callback_with_tmp_file): (gdk_pixbuf_real_save_to_callback): (gdk_pixbuf_new_from_xpm_data): (_gdk_pixbuf_generic_image_load): * gdk-pixbuf-animation.c (gdk_pixbuf_animation_new_from_file): * gdk-pixbuf-loader.c (gdk_pixbuf_loader_load_module): (gdk_pixbuf_loader_close): (gdk_pixbuf_loader_finalize): Use _gdk_pixbuf_lock() and _gdk_pixbuf_unlock(). * io-ani.c, io-bmp.c, io-gif.c, io-ico.c: * io-jpeg.c, io-pcx.c, io-png.c, io-pnm.c: * io-ras.c, io-tga.c, io-wbmp.c, io-xbm.c: * io-xpm.c: Mark as threadsafe. * io-tiff.c: Remove pointless locking, mark as threadunsafe.
This code is broken for the case of loading an SVG in a gnome (or plain gtk with threads enabled) program. We strike it with gtali. gtali tries to load an SVG file (whose loader is not thread-safe) with gtk_image_new_from_file. This calls gdk_pixbuf_animation_new_from_file which locks threadunsafe_loader_lock and then calls _gdk_pixbuf_generic_image_load which also tries to lock threadunsafe_loader_lock causing a dead-lock. I'm not familiar enough the the gdk-pixbuf code to be sure what the best way to resolve this is, so there is no patch. However, I have attached a test case below (you may need to alter the filename to suit, but it should work with any thread-unsafe file format). All the test case does is initialise gnome (which intialises threads in glib) and then calls gdk_pixbuf_animation_new_from_file. Run it, and it doesn't exit immediately as it should.
Created attachment 35188 [details] A program which triggers the lock-up.
2004-12-27 Matthias Clasen <mclasen@redhat.com> * gdk-pixbuf-animation.c (gdk_pixbuf_animation_new_from_file) Avoid deadlock. Pointed out by Callum McKenzie.