diff --git a/ChangeLog b/ChangeLog index fa92485f..1c86bc98 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,6 +1,7 @@ 27/9/16 started 8.4.2 - small doc improvements - fix error message for metadata fetch type mismatch +- resolve a race condition in thread shutdown, thanks Lovell 1/5/16 started 8.4 - many more wepsave options [Felix Bünemann] diff --git a/libvips/include/vips/private.h b/libvips/include/vips/private.h index af8bfa1b..84e46977 100644 --- a/libvips/include/vips/private.h +++ b/libvips/include/vips/private.h @@ -101,17 +101,8 @@ typedef struct _VipsBufferCache { VipsBufferThread *buffer_thread; GSList *reserve; /* VipsBuffer kept in reserve */ int n_reserve; /* Number in reserve */ - struct _VipsBufferCacheProxy *proxy; } VipsBufferCache; -/* A proxy object between the image and the cache. This has the lifetime of the - * image and keeps a nullable pointer to the buffer cache. - */ -typedef struct _VipsBufferCacheProxy { - VipsBufferCache *cache; - struct _VipsImage *im; -} VipsBufferCacheProxy; - /* What we track for each pixel buffer. These can move between caches and * between threads, but not between images. */ diff --git a/libvips/include/vips/thread.h b/libvips/include/vips/thread.h index a3a552a1..3ae7f4c6 100644 --- a/libvips/include/vips/thread.h +++ b/libvips/include/vips/thread.h @@ -51,6 +51,8 @@ void vips_g_cond_free( GCond * ); */ GThread *vips_g_thread_new( const char *, GThreadFunc, gpointer ); +gboolean vips_thread_isworker( void ); + #ifdef __cplusplus } #endif /*__cplusplus*/ diff --git a/libvips/iofuncs/buffer.c b/libvips/iofuncs/buffer.c index 0e8c9740..99129adf 100644 --- a/libvips/iofuncs/buffer.c +++ b/libvips/iofuncs/buffer.c @@ -184,8 +184,8 @@ buffer_thread_free( VipsBufferThread *buffer_thread ) * * - on thread shutdown, the enclosing hash is destroyed, and that will * trigger this via GDestroyNotify. - * - if the image is closed, buffer_cache_image_postclose() fires and will call - * this + * - if the BufferCache has been allocated by the main thread, this will be + * triggered from postclose on the image * * These can happen in either order. */ @@ -223,35 +223,26 @@ buffer_cache_free( VipsBufferCache *cache ) } VIPS_FREEF( g_slist_free, cache->reserve ); - cache->proxy->cache = NULL; - cache->proxy = NULL; - g_free( cache ); } static void -buffer_cache_proxy_image_postclose( VipsImage *im, VipsBufferCacheProxy *proxy ) +buffer_cache_image_postclose( VipsImage *im, VipsBufferCache *cache ) { - g_assert( proxy->im == im ); + VipsBufferThread *buffer_thread = cache->buffer_thread; - if( proxy->cache ) { - VipsBufferCache *cache = proxy->cache; - VipsBufferThread *buffer_thread = cache->buffer_thread; - VipsImage *im = cache->im; + /* Runs to clean up main thread buffers on image close. + */ + g_assert( cache->im == im ); + g_assert( !vips_thread_isworker() ); - g_assert( cache->im == im ); - - g_hash_table_remove( buffer_thread->hash, im ); - } - - g_free( proxy ); + g_hash_table_remove( buffer_thread->hash, im ); } static VipsBufferCache * buffer_cache_new( VipsBufferThread *buffer_thread, VipsImage *im ) { VipsBufferCache *cache; - VipsBufferCacheProxy *proxy; cache = g_new( VipsBufferCache, 1 ); cache->buffers = NULL; @@ -261,17 +252,14 @@ buffer_cache_new( VipsBufferThread *buffer_thread, VipsImage *im ) cache->reserve = NULL; cache->n_reserve = 0; - proxy = g_new( VipsBufferCacheProxy, 1 ); - proxy->im = im; - proxy->cache = cache; - cache->proxy = proxy; - - /* Free memory on image close. We can't do this on thread exit since - * some buffers will be made from the main thread and that won't exit - * until program termination. + /* VipsBufferCache allocated from worker threads will be freed when + * workers shut down. This won't happen for VipsBufferCache allocated + * from the main thread, since (obviously) thread shutdown will never + * happen. In this case, we need to free resources on image close. */ - g_signal_connect( im, "postclose", - G_CALLBACK( buffer_cache_proxy_image_postclose ), proxy ); + if( !vips_thread_isworker() ) + g_signal_connect( im, "postclose", + G_CALLBACK( buffer_cache_image_postclose ), cache ); #ifdef DEBUG_CREATE g_mutex_lock( vips__global_lock ); diff --git a/libvips/iofuncs/threadpool.c b/libvips/iofuncs/threadpool.c index 559feee8..860690c6 100644 --- a/libvips/iofuncs/threadpool.c +++ b/libvips/iofuncs/threadpool.c @@ -99,6 +99,10 @@ int vips__thinstrip_height = VIPS__THINSTRIP_HEIGHT; */ int vips__concurrency = 0; +/* Set this GPrivate to indicate that this is a vips worker. + */ +static GPrivate vips_threadpool_is_worker_private; + /* Glib 2.32 revised the thread API. We need some compat functions. */ @@ -154,6 +158,16 @@ vips_g_cond_free( GCond *cond ) #endif } +/* TRUE if we are a vips worker thread. We sometimes manage resource allocation + * differently for vips workers since we can cheaply free stuff on thread + * termination. + */ +gboolean +vips_thread_isworker( void ) +{ + return( g_private_get( &vips_threadpool_is_worker_private ) != NULL ); +} + typedef struct { const char *domain; GThreadFunc func; @@ -170,6 +184,10 @@ vips_thread_run( gpointer data ) if( vips__thread_profile ) vips__thread_profile_attach( info->domain ); + /* Set this to something (anything) to tag this thread as a vips worker. + */ + g_private_set( &vips_threadpool_is_worker_private, data ); + result = info->func( info->data ); g_free( info );