Whoops, after writing up and submitting this report, it actually did timeout eventually and returned!

I think the main problem is just for me to write an async API!

g_output_stream_splice_async() is probably what you want.

Would the ideal way be to do something like this:

Factor most of gdata_picasaweb_service_upload_file () into something like 
| _get_file_input_stream ()
| _get_file_output_stream ()

Then have 
| gdata_picasaweb_service_upload_file
| gdata_picasaweb_service_upload_file_async 
call those private functions, call their respective splice functions, and clean up a little.

Also provide 
| gdata_picasaweb_service_upload_file_finish
For the async's callback to obtain the new GDataPicasaWebFile entry.

Created attachment 147537 [details] [review]
Patch adding async upload method

There didn't seem much point in creating a separate get_file_input_stream () function after all.  

I'm uncertain how to handle errors of the GDATA_SERVICE_ERROR_* variety.  The other *_async () don't seem to have a GError ** param, though it makes enough sense to me to have one on gdata_picasaweb_service_upload_file_async () as well as the one on *async_finish () since the errors are provided at different times :)

Review of attachment 147537 [details] [review]:

I haven't checked the test suite changes, but the rest needs some work re. the callbacks and error handling, as you noted.

::: gdata/services/picasaweb/gdata-picasaweb-service.c
@@ +418,3 @@
+ *
+ * This should be called to obtain the result of a call to
+ * gdata_picasaweb_service_upload_file_async and check for errors.

You should put a pair of brackets ("()") after function references in documentation, to make sure the function is linked when the documentation is processed.

@@ +431,3 @@
+ */
+GDataPicasaWebFile *
+gdata_picasaweb_service_upload_file_finish (GObject *output_stream, GAsyncResult *result, GError **error)

The GObject source object here should be GDataPicasaWebService. See below for comments about the proper use of GAsyncResult.

@@ +439,3 @@
+	g_output_stream_splice_finish (G_OUTPUT_STREAM (output_stream), result, error);
+
+	if (error != NULL && *error != NULL) {

You can't do this, as it means errors could possibly not be detected if error == NULL. Use a local GError *child_error = NULL and propagate it to error if necessary.

@@ +481,3 @@
+ * %GDATA_SERVICE_ERROR_AUTHENTICATION_REQUIRED will be returned.
+ *
+ * @callback should call gdata_picasaweb_service_upload_file_finish ()

Don't leave a space before the pair of brackets in documentation; it doesn't get parsed properly.

@@ +499,3 @@
+	g_return_if_fail (callback != NULL);
+
+	/* TODO: do we want to still set an error for these cases as an out param? */

It wouldn't be an out param of gdata_picasaweb_service_upload_file_async(); the callback should be called with a GAsyncResult created with g_simple_async_result_new_from_error(). That way, the error is returned to the calling code via the callback function in the main thread. This means that all error handling code in the calling code can be on the callback side, rather than it having to have error handling code for both the initial call to gdata_picasaweb_service_upload_file_async() and the callback.

@@ +525,3 @@
+
+	/* TODO: investigate what a better iopriority other than 0 might be */
+	g_output_stream_splice_async (output_stream, input_stream, G_OUTPUT_STREAM_SPLICE_CLOSE_SOURCE | G_OUTPUT_STREAM_SPLICE_CLOSE_TARGET, 0, 

You can't piggy-back on the async result of the splice operation. You should create your own callback function, so that the user's callback function takes a GDataPicasaWebService, not a GOutputStream. Your callback function should create a GSimpleAsyncResult, set the data needed by *_upload_file_finish() on it, then call g_simple_async_result_complete() on it. See my patch in bug #559638 for an example.

@@ +528,3 @@
+				      cancellable, callback, user_data);
+
+	//g_object_unref (output_stream); // when I checked the code in git, it created its own ref ... ?

Weird object lifetimes like this are an indication that something's not quite right with your callback code. See above. :-)

Is bug 559638 the correct bug?  It seems to be for "Compile failure: missing link against libX11"

Sorry, I meant bug #559628.

Created attachment 147629 [details] [review]
Patch adding async upload method (updated, with questions)

I'm setting needs-work on this pre-emptively because of uncertainty on a couple parts. 

The example in bug #559628 was helpful; I'd been trying to delay the processing of g_output_stream_splice_finish until gdata_picasaweb_service_upload_file_finish (d'oh!) rather than in my own callback (yay!).

The parts that I'm least clear on are summarised below, but are brought up in TODO comments a bit more clearly in the code:

* ideal way to propagate errors from gdata_picasaweb_service_upload_file_async() so that they can make it to the user's callback.  I'm presuming I still want the user to get those errors through gdata_picasaweb_service_upload_file_finish(), so right now, I'm creating a g_simple_async_result_new_from_error() after each error if caught, setting its callback to be the same one that g_output_stream_splice_async() would call, and calling complete on the GSimpleAsyncResult.   Concerns are listed in a TODO comment next to them.

* in upload_file_async_cb (our callback), I end up treating a GAsyncResult like a GSimpleAsyncResult because I "need" it to handle the errors from gdata_picasaweb_service_upload_file_async(), and it works, even though I don't think the API of g_output_stream_slice_async() at all guarantees that it would. Ugh :( 

* gdata_picasaweb_service_upload_file_finish() does almost nothing but propagates errors if found in the result and try to return the GDataPicasaWebFile I set as its result pointer in upload_file_async_cb().  I'm getting turned around a bit with the referencing, though it seems to behave correctly.

Cheers!

Are you sure that's the right patch? I'm just getting a git diff summary line.

Created attachment 147852 [details] [review]
Patch adding async upload method (updated, with questions)

... doing stuff in a hurry == stupid mistakes.  D'oh.

Created attachment 147858 [details] [review]
Patch adding async upload method (updated, with questions)

Er, the last one failed to pass the correct GError along in gdata_picasaweb_service_upload_file () :|

Created attachment 147950 [details] [review]
Patch adding async upload method (updated, with questions)

Now closes the GOutputStream with our GCancellable before unrefing it, to let it know that the stream is done, as per bug 602156.

I'm not sure where to unref the GOutputStream I pass to the _async version, though :|

Review of attachment 147950 [details] [review]:

::: docs/reference/gdata-sections.txt
@@ +1181,3 @@
 GDataPicasaWebService
 GDataPicasaWebServiceClass
+UploadFileAsyncCallback

Even if you needed this, it's not correctly namespaced. Needs more "G"s! See my comments on gdata-picasaweb-service.c as to why this isn't necessary anyway.

::: gdata/services/picasaweb/gdata-picasaweb-service.c
@@ +457,3 @@
+		 which means that when someone tries to call this function to get their errors, the result,
+		 when an error had occurred, we would trip the assert, since the source_tag wouldn't have been set.
+		 I think I'm doing this wrong :( 

Checking the tag should be correct. Do it after the call to g_simple_async_result_propagate_error().

@@ +469,3 @@
+		 so I suppose need to ref it again and let the user unref it when they're done? */
+	if (file != NULL) 
+		g_object_ref (file); /* increase the ref count since GSimpleAsyncResult also wants to unref it */

I don't think you need to do that. Just use NULL instead of g_object_unref in the call to g_simple_async_result_set_op_res_gpointer(). You only need to pass a destroy function to *_set_op_res_gpointer() if you actually want the data destroyed once you're done with it.

@@ +503,3 @@
+		g_output_stream_splice_finish (output_stream, G_ASYNC_RESULT (result), &error);
+	else
+		g_simple_async_result_propagate_error (result, &error);

There's no need for this. Treat @result as a GAsyncResult and simply pass it directly to g_output_stream_splice_finish(). As explained below, you don't need to pass errors generated in *_upload_file_async() through this callback — they can go directly to the user's callback via the GAsyncResult generated by g_simple_async_result_new_from_error(). When the user's callback calls *_upload_file_finish() on this async result, the error will get propagated, and everyone will be happy.

@@ +510,3 @@
+
+	if (error == NULL && file != NULL)
+		async_result = g_simple_async_result_new (G_OBJECT (data->service), (GAsyncReadyCallback)data->callback,

Could you put a space between typecasts and the variable names please? e.g.:
(GAsyncReadyCallback) data->callback

@@ +518,3 @@
+	g_simple_async_result_set_op_res_gpointer (async_result, file, g_object_unref);
+
+	upload_file_async_data_free (data);

I think this should go below the call to g_simple_async_result_complete(), as otherwise another thread could unref @data->service after we've created @async_result with it. We then drop our reference (the final reference) to it in upload_file_async_data_free() and it is destroyed. g_simple_async_result_complete() or the user's callback attempts to access it and consequently segfaults.
Once g_simple_async_result_complete() has returned, the user's callback is guaranteed to have run, since it's a synchronous call, so we should be OK to free @data.

@@ +547,3 @@
+void
+gdata_picasaweb_service_upload_file_async (GDataPicasaWebService *self, GDataPicasaWebAlbum *album, GDataPicasaWebFile *file_entry,
+					   GFile *file_data, GCancellable *cancellable, UploadFileAsyncCallback callback, gpointer user_data)

Don't create another typedef for the callback type: just use GAsyncReadyCallback.

@@ +558,3 @@
+	g_return_if_fail (GDATA_IS_PICASAWEB_FILE (file_entry));
+	g_return_if_fail (G_IS_FILE (file_data));
+	g_return_if_fail (callback != NULL);

@callback can be NULL. You also need the following check:
g_return_if_fail (cancellable == NULL || G_IS_CANCELLABLE (cancellable));

@@ +562,3 @@
+	data = g_slice_new (UploadFileAsyncData);
+	g_object_ref (self);
+	data->service = self;

You can combine the above two lines, since g_object_ref (self) == self.

@@ +582,3 @@
+	   whether g_output_stream_splice_async's GAsyncResult called
+	   upload_file_async_cb or my GSimpleAsyncResult called it,
+	   error could be found in my data, but that seems kind of ugly.

See below. It doesn't matter that GAsyncReadyCallback takes GAsyncResults — GSimpleAsyncResult is a subclass of GAsyncResult anyway.

@@ +587,3 @@
+		g_set_error_literal (&error, GDATA_SERVICE_ERROR, GDATA_SERVICE_ERROR_ENTRY_ALREADY_INSERTED,
+				     _("The entry has already been inserted."));
+		result = g_simple_async_result_new_from_error (NULL, (GAsyncReadyCallback)upload_file_async_cb, data, error);

I was thinking something more along the lines of:
result = g_simple_async_result_new_from_error (self, callback, user_data, error);
That means you can allocate @data later as well.

@@ +600,3 @@
+	}
+
+	output_stream = get_file_output_stream (self, album, file_entry, file_data, &error);

Can we have some comments here please? :-)

@@ +603,3 @@
+	if (output_stream == NULL) {
+		result = g_simple_async_result_new_from_error (NULL, (GAsyncReadyCallback)upload_file_async_cb, data, error);
+		g_simple_async_result_complete (result);

All these identical copies of the error handling code would be best put under an "error:" label at the bottom of the function and just gotoed to in case of error.

::: gdata/services/picasaweb/gdata-picasaweb-service.h
@@ +120,3 @@
+ * Since: 0.6.0
+ */
+typedef void (*UploadFileAsyncCallback)(GDataPicasaWebService *self, GSimpleAsyncResult *result, gpointer user_data);

No need for this. See my comments in gdata-picasaweb-service.c.

Created attachment 148426 [details] [review]
Patch adding async upload method (updated, almost got it)

"Checking the tag should be correct. Do it after the call to
g_simple_async_result_propagate_error()."

The problem I run into is that the source tag for the GAsyncResult isn't set when it was created with g_simple_async_result_new_from_error(), which I do in both gdata_picasaweb_service_upload_file_async() and upload_file_async_cb() on errors.  

Now I do g_simple_async_result_propagate_error() first, and if *error != NULL, I return NULL then rather than trip the assert.  I could alternatively check in the assert that error != NULL and let it pass in that case as well (as a reason why the source tag would not be set) or I could create the GAsyncResult with result_new() and result_set_from_error() instead of result_new_from_error().  

"Don't create another typedef for the callback type: just use
GAsyncReadyCallback." 
"See below. It doesn't matter that GAsyncReadyCallback takes GAsyncResults —
GSimpleAsyncResult is a subclass of GAsyncResult anyway."

Sorry about that.  I have an aversion to assuming a subclass's methods will work on an object only guaranteed to be a superclass.  That said, I don't expect it to ultimately not be a GSimpleAsyncResult either, so I'm now using GAsyncReadyCallback and GAsyncResult despite my anxieties :)

By the way, do yo have a rough idea when 0.6.0 might be released?  I wrote a EoG plugin for uploading to PicasaWeb (bug 600190) which at least needs a fix for closing upload streams that you committed already, but which could also benefit from the upload_async () method.  (I have an alternative solution in the plugin for now which I'll be happy to rip out.)

Review of attachment 148426 [details] [review]:

(In reply to comment #14)
> Sorry about that.  I have an aversion to assuming a subclass's methods will
> work on an object only guaranteed to be a superclass.  That said, I don't
> expect it to ultimately not be a GSimpleAsyncResult either, so I'm now using
> GAsyncReadyCallback and GAsyncResult despite my anxieties :)

In what case do you think a GAsyncResult will get treated as a GSimpleAsyncResult potentially incorrectly? The GAsyncResult from the splice operation you only ever pass to *_splice_finish(), and you have control over the GAsyncResult handed between *_upload_file_async() and *_upload_file_finish().

(In reply to comment #15)
> By the way, do yo have a rough idea when 0.6.0 might be released?

Probably sometime over Christmas, as soon as all your nice patches land. I should have some time available to make a release, so keep prodding me.

::: gdata/services/picasaweb/gdata-picasaweb-service.c
@@ +456,3 @@
+
+	/* propagate any potential errors we might have encountered in g_output_stream_splice() or gdata_picasaweb_service_upload_file_async() */
+	g_simple_async_result_propagate_error (G_SIMPLE_ASYNC_RESULT (result), error);

You don't need to put the check in. If an error was set on the async result, *_propagate_error() will return TRUE, regardless of whether error == NULL. You can then just do:

if (g_simple_async_result_propagate_error (G_SIMPLE_ASYNC_RESULT (result), error))
        return NULL;

@@ +462,3 @@
+	/* TODO: in cases where the GAsyncResult was created from _new_from_error (), the source_tag == NULL.  Instead of relying on error to be
+	   set and available in those cases, should we consider allowing source_tag to == NULL in the assert?  Or should we ensure that source
+	   tag is set by using result_new () and result_set_from_error () instead of result_new_from_error () ? */

The above code I've given will always return from the function in the cases where the source_tag == NULL.

Created attachment 149186 [details] [review]
Patch adding async upload method

> In what case do you think a GAsyncResult will get treated as a
> GSimpleAsyncResult potentially incorrectly? The GAsyncResult from the splice
> operation you only ever pass to *_splice_finish(), and you have control over
> the GAsyncResult handed between *_upload_file_async() and
> *_upload_file_finish().

Just in the case that GIO finds they need some GComplicatedAsyncReady class rather than GSimpleAsyncReady, and subclass it from GAsyncReady, which their API advertises, rather than GSimpleAsyncReady, which they incidentally used internally.  However, I can write a patch if that were to happen.

> Probably sometime over Christmas, as soon as all your nice patches land. I
> should have some time available to make a release, so keep prodding me.

I got an e-mail about my account, hurrah.  I will double-check and then apply the ones you've OK'd so far. 

> The above code I've given will always return from the function in the cases
> where the source_tag == NULL.

Yah, that's much better.  I need to not rely on error being set and consider more the return values of functions :)

Review of attachment 149186 [details] [review]:

(In reply to comment #17)
> Just in the case that GIO finds they need some GComplicatedAsyncReady class
> rather than GSimpleAsyncReady, and subclass it from GAsyncReady, which their
> API advertises, rather than GSimpleAsyncReady, which they incidentally used
> internally.  However, I can write a patch if that were to happen.

You don't call any methods on the GAsyncResult for the splice operation, or examine its structure in any way, so I fail to see how this can be a problem.

Please commit the patch. :-)

Ah, I meant how in gdata_picasaweb_service_upload_file_finish() I call g_simple_async_result_* methods on the suplied GAsyncResult.  Silly me, substituting Ready for Result in the above comment, though. :|

Patch has been committed :)

(In reply to comment #19)
> Ah, I meant how in gdata_picasaweb_service_upload_file_finish() I call
> g_simple_async_result_* methods on the suplied GAsyncResult.  Silly me,
> substituting Ready for Result in the above comment, though. :|

That's not a problem, because *_upload_file_finish() should only ever be called from the GAsyncReadyCallback for *_upload_file_async(). Since the code in upload_file_async_cb() creates a GSimpleAsyncResult to pass to the GAsyncReadyCallback, *_upload_file_finish() should only ever receive a GSimpleAsyncResult. :-)

Are you still waiting for any of my patches for 0.6.0?  I think the last one you ended up committing while I was away moving again :)  I think features I have been working on which aren't quite ready are
* examples in documentation
* support for accessing/posting comments
* world domination

Are the examples blocking anything right now?

(In reply to comment #21)
> Are you still waiting for any of my patches for 0.6.0? *snip*

What's the status of bug #598748? Sorry, I haven't had time to look at it.

> Are the examples blocking anything right now?

Nope; they can be added as and when anyone feels like it.

(In future, you might want to e-mail me with this sort of thing, since it's not strictly related to this bug. :-))