From c9b87ce4f5407082108fab0a424180677172d767 Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Mon, 8 Jan 2018 11:47:47 +0000 Subject: [PATCH] fix a thread leak in sink_screen we were not joining the bg render thread when it exited instead, keep the thread around with a semaphore for it to wait on --- libvips/iofuncs/image.c | 19 +++- libvips/iofuncs/sinkscreen.c | 210 +++++++++++++++++++---------------- 2 files changed, 128 insertions(+), 101 deletions(-) diff --git a/libvips/iofuncs/image.c b/libvips/iofuncs/image.c index 827ca99a..6e964dca 100644 --- a/libvips/iofuncs/image.c +++ b/libvips/iofuncs/image.c @@ -2517,30 +2517,39 @@ vips_image_write_gen( VipsRegion *or, int vips_image_write( VipsImage *image, VipsImage *out ) { + /* image needs to stay alive for this call. It can be unreffed during + * the generate. + */ + g_object_ref( image ); + if( vips_image_pio_input( image ) || vips_image_pipelinev( out, - VIPS_DEMAND_STYLE_THINSTRIP, image, NULL ) ) + VIPS_DEMAND_STYLE_THINSTRIP, image, NULL ) ) { + g_object_unref( image ); return( -1 ); + } if( vips_image_generate( out, vips_start_one, vips_image_write_gen, vips_stop_one, - image, NULL ) ) + image, NULL ) ) { + g_object_unref( image ); return( -1 ); + } - /* If @out is a partial image, we need to make sure that @image stays - * alive as long as @out is alive. + /* If @out is a partial image, we need to unref @image when out is + * unreffed. * * If it's not partial, perhaps a file we write to or a memory image, * we need to break any links between @image and @out created by * vips_image_pipelinev(). */ if( vips_image_ispartial( out ) ) { - g_object_ref( image ); vips_object_local( out, image ); } else { vips__reorder_clear( image ); vips__link_break_all( out ); + g_object_unref( image ); } return( 0 ); diff --git a/libvips/iofuncs/sinkscreen.c b/libvips/iofuncs/sinkscreen.c index f003feab..4d57e9be 100644 --- a/libvips/iofuncs/sinkscreen.c +++ b/libvips/iofuncs/sinkscreen.c @@ -162,7 +162,8 @@ typedef struct _RenderThreadStateClass { G_DEFINE_TYPE( RenderThreadState, render_thread_state, VIPS_TYPE_THREAD_STATE ); -/* The BG thread which sits waiting to do some calculations. +/* The BG thread which sits waiting to do some calculations, and the semaphore + * it waits on holding the number of renders with dirty tiles. */ static GThread *render_thread = NULL; @@ -170,10 +171,12 @@ static GThread *render_thread = NULL; */ static gboolean render_kill = FALSE; -/* All the renders with dirty tiles. +/* All the renders with dirty tiles, and a semaphore that the bg render thread + * waits on. */ static GMutex *render_dirty_lock = NULL; static GSList *render_dirty_all = NULL; +static VipsSemaphore n_render_dirty_sem; /* Set this to make the bg thread stop and reschedule. */ @@ -221,8 +224,14 @@ render_free( Render *render ) g_assert( render->ref_count == 0 ); g_mutex_lock( render_dirty_lock ); - if( g_slist_find( render_dirty_all, render ) ) + if( g_slist_find( render_dirty_all, render ) ) { render_dirty_all = g_slist_remove( render_dirty_all, render ); + + /* We don't need to adjust the semaphore: if it's too high, + * the render thread will just loop and decrement next time + * render_dirty_all is NULL. + */ + } g_mutex_unlock( render_dirty_lock ); vips_g_mutex_free( render->ref_count_lock ); @@ -275,34 +284,6 @@ render_unref( Render *render ) return( 0 ); } -/* Get the first render with dirty tiles. - */ -static Render * -render_dirty_get( void ) -{ - Render *render; - - g_mutex_lock( render_dirty_lock ); - - /* Just take the head of the jobs list ... we sort when we add. - */ - render = NULL; - if( render_dirty_all ) { - render = (Render *) render_dirty_all->data; - - /* Ref the render to make sure it can't die while we're - * working on it. - */ - render_ref( render ); - - render_dirty_all = g_slist_remove( render_dirty_all, render ); - } - - g_mutex_unlock( render_dirty_lock ); - - return( render ); -} - /* Get the next tile to paint off the dirty list. */ static Tile * @@ -444,49 +425,6 @@ render_work( VipsThreadState *state, void *a ) static void render_dirty_put( Render *render ); -/* Main loop for RenderThreads. - */ -static void * -render_thread_main( void *client ) -{ - Render *render; - - while( (render = render_dirty_get()) && - !render_kill ) { - VIPS_DEBUG_MSG_GREEN( "render_thread_main: " - "threadpool start\n" ); - - render_reschedule = FALSE; - if( vips_threadpool_run( render->in, - render_thread_state_new, - render_allocate, - render_work, - NULL, - render ) ) - VIPS_DEBUG_MSG_RED( "render_thread_main: " - "threadpool_run failed\n" ); - - VIPS_DEBUG_MSG_GREEN( "render_thread_main: " - "threadpool return\n" ); - - /* Add back to the jobs list, if we need to. - */ - render_dirty_put( render ); - - /* _get() does a ref to make sure we keep the render - * alive during processing ... unref before we loop. - * This can kill off the render. - */ - render_unref( render ); - } - - /* We are exiting, so render_thread must now be NULL. - */ - render_thread = NULL; - - return( NULL ); -} - /* Called from vips_shutdown(). */ void @@ -494,24 +432,25 @@ vips__render_shutdown( void ) { /* We may come here without having inited. */ - if( !render_dirty_lock ) - return; + if( render_dirty_lock ) { + g_mutex_lock( render_dirty_lock ); - g_mutex_lock( render_dirty_lock ); + if( render_thread ) { + GThread *thread; - if( render_thread ) { - GThread *thread; + thread = render_thread; + render_reschedule = TRUE; + render_kill = TRUE; - thread = render_thread; + g_mutex_unlock( render_dirty_lock ); - g_mutex_unlock( render_dirty_lock ); + vips_semaphore_up( &n_render_dirty_sem ); - render_reschedule = TRUE; - render_kill = TRUE; - (void) vips_g_thread_join( thread ); + (void) vips_g_thread_join( thread ); + } + else + g_mutex_unlock( render_dirty_lock ); } - else - g_mutex_unlock( render_dirty_lock ); } static int @@ -534,17 +473,10 @@ render_dirty_put( Render *render ) render_dirty_all = g_slist_sort( render_dirty_all, (GCompareFunc) render_dirty_sort ); - /* Make sure there is a bg render thread, and get it to - * reschedule. + /* Tell the bg render thread we have one more dirty + * render on there. */ - if( !render_thread ) { - render_thread = vips_g_thread_new( "sink_screen", - render_thread_main, NULL ); - g_assert( render_thread ); - } - VIPS_DEBUG_MSG_GREEN( "render_dirty_put: " - "reschedule\n" ); - render_reschedule = TRUE; + vips_semaphore_up( &n_render_dirty_sem ); } } @@ -1020,10 +952,96 @@ mask_fill( VipsRegion *out, void *seq, void *a, void *b, gboolean *stop ) return( 0 ); } +/* Get the first render with dirty tiles. + */ +static Render * +render_dirty_get( void ) +{ + Render *render; + + /* Wait for a render with dirty tiles. + */ + vips_semaphore_down( &n_render_dirty_sem ); + + g_mutex_lock( render_dirty_lock ); + + /* Just take the head of the jobs list ... we sort when we add. + */ + render = NULL; + if( render_dirty_all ) { + render = (Render *) render_dirty_all->data; + + /* Ref the render to make sure it can't die while we're + * working on it. + */ + render_ref( render ); + + render_dirty_all = g_slist_remove( render_dirty_all, render ); + } + + g_mutex_unlock( render_dirty_lock ); + + return( render ); +} + +/* Loop for the background render mananger thread. + */ +static void * +render_thread_main( void *client ) +{ + Render *render; + + while( !render_kill ) { + VIPS_DEBUG_MSG_GREEN( "render_thread_main: " + "threadpool start\n" ); + + render_reschedule = FALSE; + + if( (render = render_dirty_get()) ) { + if( vips_threadpool_run( render->in, + render_thread_state_new, + render_allocate, + render_work, + NULL, + render ) ) + VIPS_DEBUG_MSG_RED( "render_thread_main: " + "threadpool_run failed\n" ); + + VIPS_DEBUG_MSG_GREEN( "render_thread_main: " + "threadpool return\n" ); + + /* Add back to the jobs list, if we need to. + */ + render_dirty_put( render ); + + /* _get() does a ref to make sure we keep the render + * alive during processing ... unref before we loop. + * This can kill off the render. + */ + render_unref( render ); + } + } + + /* We are exiting, so render_thread must now be NULL. + */ + render_thread = NULL; + + return( NULL ); +} + static void vips_sink_screen_init( void ) { + g_assert( !render_thread ); + g_assert( !render_dirty_lock ); + render_dirty_lock = vips_g_mutex_new(); + render_thread = vips_g_thread_new( "sink_screen", + render_thread_main, NULL ); + vips_semaphore_init( &n_render_dirty_sem, 0, "n_render_dirty" ); + + g_assert( render_thread ); + g_assert( render_dirty_lock ); } /**