async ops should always be identical in function to sync ops. I don't want to read code that says things like:
/* using async op here because sync does something bad */

So the async close not flushing is a bug and should be fixed.

Created attachment 160451 [details] [review]
Add flush to close_async

Proposed patch to add a flush call to close_async. I'm not too familiar with gio code (for example if there should be a check on the cancellable state in between the flush and the close). It does seem to fix the issues I had.

I think the fix is the wrong approach. With your patch, g_output_stream_close() calls the ->flush and the ->close vfuncs, while g_output_stream_close_async() does not call the ->flush_async vfunc and expects ->close_async to close things.

I think the correct fix would be to change g_output_stream_close_async() to call ->flush_async with a self-provided callback that then calls ->close_async that then calls the user-provided callback.
(Man, async code is even hard to explain.)

I understand how it's supposed to work, but I think you'd need to keep some state (like the cancellable and the io_priority) around in the output stream struct? I don't know, is there a better way?

Also, to keep it semantically the same as the sync function, it should propagate an error occuring in the flush, but still do the close (which means that the errors needs to be somehow transferred to the async close and set properly even if the async close succeeds).

Created attachment 160497 [details] [review]
Test cases for g_output_stream_close_async

The test adds some test cases for closing an output stream asynchronously. The test basicly sets up a compressor stream with a memory output stream and writes some data, then closes asynchronously (without manual flush, with sync manual flush and with async manual flush). The result is then compared to the result of closing the same setup synchronously.

Created attachment 160498 [details] [review]
Test cases for g_output_steam_close_async v2 rebased on master

Rebased on master

Created attachment 160499 [details] [review]
Preliminary patch for flush_async in close_async

This patch implements the async flush according to previous comments. With the patch, the first test (no manual flush) passes, however when doing a manual flush before closing the stream, to many bytes are written. This might be another problem that is somewhat related (or simply a bug in the patch), but I haven't looked into it.

Maybe I should try some things before commenting since I'm doing a kind of monologue here, nevertheless, it seems the converter stream might not handle flushing correctly. If I do a sync flush before a sync close, the resulting data is also not the same as when I do just the sync close.

Actually, I think the test is wrong since flushing manually will affect the compression and therefore the data is different, so the patch for the async flush seems to work correctly.

Review of attachment 160499 [details] [review]:

::: gio/goutputstream.c
@@ +595,3 @@
+        g_simple_async_result_set_from_error (G_SIMPLE_ASYNC_RESULT (res),
+                                              info->flush_error);
+

This is not right, you shouldn't mess with the GAsyncResult you get from the close operation, instead we should make our own and hand to the user in the callback.

@@ -992,3 @@
   g_object_ref (stream);
-  class->close_async (stream, io_priority, cancellable,
-		      async_ready_close_callback_wrapper, user_data);

There are some common cases where we want to avoid doing the flush and just call close_async immediately:

1) If close_async is g_output_stream_real_close_async, because then the sync close will flush
2) if flush_async is NULL
3) if flush_async is g_output_stream_real_flush_async and flush is NULL (because we'd just schedule a thread to do nothing)

(In reply to comment #11)
> Review of attachment 160499 [details] [review]:
> 
> ::: gio/goutputstream.c
> @@ +595,3 @@
> +        g_simple_async_result_set_from_error (G_SIMPLE_ASYNC_RESULT (res),
> +                                              info->flush_error);
> +
> 
> This is not right, you shouldn't mess with the GAsyncResult you get from the
> close operation, instead we should make our own and hand to the user in the
> callback.

I don't think I really understand this. I tried to mimic the sync close behavior here, propagating the error. Why should the error not be set in this async result?

> @@ -992,3 @@
>    g_object_ref (stream);
> -  class->close_async (stream, io_priority, cancellable,
> -              async_ready_close_callback_wrapper, user_data);
> 
> There are some common cases where we want to avoid doing the flush and just
> call close_async immediately:
> 
> 1) If close_async is g_output_stream_real_close_async, because then the sync
> close will flush

As far as I can see, the close async does _not_ simply call the sync close function (which is the problem), so it won't flush, right?

> 2) if flush_async is NULL
> 3) if flush_async is g_output_stream_real_flush_async and flush is NULL
> (because we'd just schedule a thread to do nothing)

IRC backlog:

<alex> however, we want to a) avoid doing flush at all when its a no-op (2-3 in the bug report)
 and b) combine it with the close if both flush and close will be run as the default implementation which just schedules an i/o thread and calls the sync versions of the calls
<jessevdk> but you also need a flush if close is not the default implementation
 or not?
<alex> jessevdk: yeah
<alex> jessevdk: so, do a direct close_async call if flush_async == NULL || (flush_async == real_flush_async && flush == NULL) || (close_async == real_close_async && flush_async == real_flush_async)
<alex> then in close_async_thread, if flush_async == real_flush_async then do a sync flush first

Created attachment 160777 [details] [review]
Flush in async close, take 3

This patch should implement according to alex's remarks and explanations.

Created attachment 160779 [details] [review]
Test cases, take 3

This is an update of the test cases because they were not correct before. This test now correctly implements comparing sync close and async close in a few cases for the GZlibCompressor stream. Maybe there could be a simpler test using some kind of custom stream testing the different cases of having (or not having) flush/flush_async.

looks good to me. please commit