Re: Patch for 78542 - change thumbnails to use a thread

[Alex Larsson <alexl redhat com>]

On 21 May 2002, Damon Chaplin wrote:

> 
> I've added a monster patch to:
> 
> http://bugzilla.gnome.org/show_bug.cgi?id=78542
> 
> 
> Nautilus currently does a fork() to create each thumbnail, which I don't
> think is safe when using gnome-vfs, and it is also inefficient.
> 
> The patch changes it so it uses a thread to create all the thumbnails in
> a directory. (I assume using pthread functions is OK in nautilus. They
> are used elsewhere.) Though it still forks/execs 'convert' if it has to.
> The patch also removes code related to public metadata, which we don't
> use any more.
> 
> 
> I don't know if this fixes the particular bug. But convert works for me
> now, and it didn't before.

In general this looks pretty good. I'm way to tired to review it fully 
now, but if you change it according to the comments below and do a fair 
amount of testing of it you can check it in. Then I'll do some more 
reviewing in cvs later (with a large change like this, reading the final 
code is easier anyway).

Comments from a quick read:

Race condition on thumbnails_to_make:

+	info = g_new0 (NautilusThumbnailInfo, 1);
+	info->image_uri = file_uri;
+	info->mime_type = nautilus_file_get_mime_type (file);
+	info->original_file_mtime = file_mtime;
+		
+	/* Check if it is already in the list of thumbnails to make. */
+	if (g_list_find_custom (thumbnails_to_make, info,
+				compare_thumbnail_info) == NULL) {
+#ifdef DEBUG_THUMBNAILS
+		g_message ("(Main Thread) Locking mutex\n");
+#endif
+		pthread_mutex_lock (&thumbnails_mutex);
 
You need to grab the lock before searching the list. (Or you could walk of 
the list if you accidentally read it while the thread was modifying it.)

If convert gets stuck it blocks the thumbnailing thread. Do we care about 
that? I think the old code had a timeout check.

thumbnail_thread_notify_file_changed() doesn't really need to do 
GDK_THREADS_ENTER/LEAVE, since none of nautilus uses gdk threads. But it's 
good practice, so lets leave it there.

I think thumbnail_thread_is_running and thumbnails_to_make need to be volatile.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander Larsson                                            Red Hat, Inc 
                   alexl redhat com    alla lysator liu se 
He's an immortal voodoo paranormal investigator haunted by an iconic dead 
American confidante She's a mistrustful African-American detective with the 
power to see death. They fight crime!