After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 617937 - output_stream_close vs output_stream_close_async semantics
output_stream_close vs output_stream_close_async semantics
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gio
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks: 413220
 
 
Reported: 2010-05-06 17:14 UTC by jessevdk@gmail.com
Modified: 2010-05-12 07:24 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add flush to close_async (1.28 KB, patch)
2010-05-06 18:01 UTC, jessevdk@gmail.com
none Details | Review
Test cases for g_output_stream_close_async (8.52 KB, patch)
2010-05-07 09:59 UTC, jessevdk@gmail.com
none Details | Review
Test cases for g_output_steam_close_async v2 rebased on master (8.52 KB, patch)
2010-05-07 10:03 UTC, jessevdk@gmail.com
none Details | Review
Preliminary patch for flush_async in close_async (3.73 KB, patch)
2010-05-07 10:11 UTC, jessevdk@gmail.com
none Details | Review
Flush in async close, take 3 (5.94 KB, patch)
2010-05-10 22:06 UTC, jessevdk@gmail.com
committed Details | Review
Test cases, take 3 (8.83 KB, patch)
2010-05-10 22:08 UTC, jessevdk@gmail.com
committed Details | Review

Description jessevdk@gmail.com 2010-05-06 17:14:06 UTC
Experimenting a bit with converter streams, I noticed that there is quite a difference between closing a stream sync and closing a stream async. Looking at the code, it seems the async close does not flush pending output. I'm not sure if this is intentional, but if it is it should at least be mentioned in the documentation. In addition, if you try to do the flush manually, it still doesn't work because the converter streams are never flushed while the base stream is in the closing state (which causes the converter stream to not finalize the output, at least in the case of GZlibCompressor as a converter this causes problems).

So either way, it seems it's not possible to correctly close the stream asynchronously in case of converter output streams.
Comment 1 Benjamin Otte (Company) 2010-05-06 17:31:12 UTC
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.
Comment 2 jessevdk@gmail.com 2010-05-06 18:01:58 UTC
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.
Comment 3 Benjamin Otte (Company) 2010-05-06 19:00:47 UTC
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.)
Comment 4 jessevdk@gmail.com 2010-05-06 21:45:06 UTC
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?
Comment 5 jessevdk@gmail.com 2010-05-06 22:09:16 UTC
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).
Comment 6 jessevdk@gmail.com 2010-05-07 09:59:37 UTC
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.
Comment 7 jessevdk@gmail.com 2010-05-07 10:03:15 UTC
Created attachment 160498 [details] [review]
Test cases for g_output_steam_close_async v2 rebased on master

Rebased on master
Comment 8 jessevdk@gmail.com 2010-05-07 10:11:03 UTC
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.
Comment 9 jessevdk@gmail.com 2010-05-07 10:15:16 UTC
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.
Comment 10 jessevdk@gmail.com 2010-05-09 15:01:38 UTC
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.
Comment 11 Alexander Larsson 2010-05-10 14:10:26 UTC
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)
Comment 12 jessevdk@gmail.com 2010-05-10 14:17:07 UTC
(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)
Comment 13 jessevdk@gmail.com 2010-05-10 22:05:42 UTC
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
Comment 14 jessevdk@gmail.com 2010-05-10 22:06:53 UTC
Created attachment 160777 [details] [review]
Flush in async close, take 3

This patch should implement according to alex's remarks and explanations.
Comment 15 jessevdk@gmail.com 2010-05-10 22:08:47 UTC
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.
Comment 16 Alexander Larsson 2010-05-12 07:11:41 UTC
looks good to me. please commit