Created attachment 260126 [details] [review]
Add an asynchronous version of gdk_pixbuf_get_file_info

Review of attachment 260126 [details] [review]:

::: gdk-pixbuf/gdk-pixbuf-io.c
@@ +1981,3 @@
+        result = g_simple_async_result_new (NULL, callback, user_data, gdk_pixbuf_get_file_info_async);
+        g_simple_async_result_set_op_res_gpointer (result, data, (GDestroyNotify) get_file_info_data_async_data_free);
+        g_simple_async_result_run_in_thread (result, (GSimpleAsyncThreadFunc) get_file_info_thread, G_PRIORITY_DEFAULT, cancellable);

No, because that's not cancellable. The async code should use async functions directly, passing the same cancellable to each one of the consecutive calls.

(In reply to comment #2)
> Review of attachment 260126 [details] [review]:
> 
> ::: gdk-pixbuf/gdk-pixbuf-io.c
> @@ +1981,3 @@
> +        result = g_simple_async_result_new (NULL, callback, user_data,
> gdk_pixbuf_get_file_info_async);
> +        g_simple_async_result_set_op_res_gpointer (result, data,
> (GDestroyNotify) get_file_info_data_async_data_free);
> +        g_simple_async_result_run_in_thread (result, (GSimpleAsyncThreadFunc)
> get_file_info_thread, G_PRIORITY_DEFAULT, cancellable);
> 
> No, because that's not cancellable. The async code should use async functions
> directly, passing the same cancellable to each one of the consecutive calls.

That would result in having duplicate copies of the same code -- one for the older sync variant, one for the new cancellable async variant.

GTask (particularly see g_task_set_return_on_cancel) lets you add cancellability to uncancellable tasks. Our minimum GLib requirement is high enough to let us use GTask, so I think we should just do that.

From my reading of the code, I do not think g_simple_async_result_set_handle_cancellation lets you do such a thing. If it does, then great. Else we have GTask.

Other than that there are some mistakes in the usage of GSimpleAsyncResult in the existing code. eg., we use neither g_simple_async_result_set_check_cancellable nor g_simple_async_result_set_handle_cancellation. This is why I did not want to get into all these details in this patch, and just copied the existing format for implementing asynchronous operations. I plan to deal with these separately.

(In reply to comment #3)
> GTask (particularly see g_task_set_return_on_cancel) lets you add
> cancellability to uncancellable tasks. Our minimum GLib requirement is high
> enough to let us use GTask, so I think we should just do that.

I filed bug 712704 for the GTask porting.

> From my reading of the code, I do not think
> g_simple_async_result_set_handle_cancellation lets you do such a thing. If it
> does, then great. Else we have GTask.

I confirmed this from danw in #gtk+.

Given that we don't have any API for the async version yet, and the fact that gdk_pixbuf_get_file_info() uses fopen/fread, I'd rather have seen a _get_file_info_from_stream_async() and, yes, duplicate the 20 odd lines of code from the sync version so that even reads can be cancelled cleanly.

(In reply to comment #5)
> Given that we don't have any API for the async version yet, and the fact that
> gdk_pixbuf_get_file_info() uses fopen/fread, I'd rather have seen a
> _get_file_info_from_stream_async() and, yes, duplicate the 20 odd lines of code
> from the sync version so that even reads can be cancelled cleanly.

I guess what you are saying is to rewrite the sync version in terms of the async?

No, just that simply using GTask probably won't be enough to allow cancelling the fread calls.

(In reply to comment #7)
> No, just that simply using GTask probably won't be enough to allow cancelling
> the fread calls.

GTask will invoke the callback as soon as you cancel. The fopen/fread calls will continue to run in the thread, which will eventually be ignored. I am not too worried about that because we are not reading the whole file anyway, so the extra I/O overhead should be negligible.

If you want to cancel the fopen/fread, then we can write the async version with GIO operations, and then rewrite the sync version in terms of the async with a NULL cancellable.

Using GTask for this sounds good to me

Created attachment 283282 [details] [review]
Add an asynchronous version of gdk_pixbuf_get_file_info

(In reply to comment #9)
> Using GTask for this sounds good to me

The problem isn't using GTask, the problem is the lingering read() that doesn't actually get cancelled inside the thread.

Attachment 283282 [details] pushed as a70109b - Add an asynchronous version of gdk_pixbuf_get_file_info