GNOME Bugzilla – Bug 737451
Provide api to read_all_async
Last modified: 2014-10-21 16:11:32 UTC
This is a first version to provide a read_all async method. It is missing the docs but let me know if this is the right way to go and I will add the docs.
Created attachment 287193 [details] [review] Provide api to read_all_async
Created attachment 287195 [details] [review] Provide api to read_all_async
On this second version it is coming with docs
Created attachment 287235 [details] [review] Provide api to read_all_async
Created attachment 287243 [details] [review] Provide write_all_async method
Created attachment 287253 [details] [review] Provide write_all_async method
Review of attachment 287253 [details] [review]: Some comments below (Sunday morning pre-coffee review, so do not take them too seriously :) ::: gio/goutputstream.c @@ +895,3 @@ + else if (nwritten == 0) + { +{ why return TRUE? should it return TRUE if we did not write all the specified size? @@ +961,3 @@ + * + * Note that no copy of @buffer will be made, so it must stay valid + data->count, I am not sure if this reference to the gbytes version is useful, since we are not adding a write_all_bytes_async (for now) @@ +980,3 @@ + + task = g_task_new (stream, cancellable, callback, user_data); +/** g_slice ? (I guess you kept consistency with the rest of file, but still these kind of small structs are what gslice is made for...)
Created attachment 287275 [details] [review] Provide write_all_async method
Created attachment 287276 [details] [review] Provide api to read_all_async
Changed both patches to use g_slice. Also fixed the issues in the write_all_async that you pointed out. You were indeed right about the nwritten == 0. In that case the sync version continues but providing a g_warning. I made the same on the async version. About the read_all it does not seem to provide an error if that happens, is this a bug? should we set a EOF error for both, the async and sync versions?
Created attachment 287277 [details] [review] Provide api to read_all_async
Created attachment 287278 [details] [review] Provide write_all_async method
Review of attachment 287277 [details] [review]: ::: gio/ginputstream.c @@ -658,0 +658,8 @@ +typedef struct +{ + gchar *buffer; ... 5 more ... I do not think you need to keep the cancellable yourself, you can use g_task_get_cancellable. Maybe the same with priority (though I am not 100% sure if gio io_priority and gtask's priority are the same thing
Created attachment 287279 [details] [review] Provide api to read_all_async
Created attachment 287280 [details] [review] Provide write_all_async method
fixed, thanks. There is another open thing though. There is no: GLIB_AVAILABLE_IN_2_44 so currently it does no compile...
Created attachment 287291 [details] [review] Provide write_all_async method
Review of attachment 287279 [details] [review]: Looks nice, but I want to get some input from Vala peeps on the bizarre signature of the finish function. ::: gio/ginputstream.c @@ +659,3 @@ +{ + gchar *buffer; + gchar *p; p is redundant here if you already have bytes_read and the original buffer. @@ +694,3 @@ + else + { + data->p += nread; Might be nice to add an assert here as a sanity-check to make sure nread is not greater than our buffer allows. @@ +791,3 @@ + **/ +gboolean +g_input_stream_read_all_finish (GInputStream *stream, This function is truly and properly evil. It's a shame that we already crossed this evil bridge when adding g_input_stream_read_all()... I think it is at least worth mentioning the exceptional nature of this function (has side-effects and sets out variables even on failure) here, explicitly. I wonder what the Vala guys think of this sort of thing... @@ +807,3 @@ + AsyncReadAll *data = (AsyncReadAll *)g_task_get_task_data (task); + + *bytes_read = data->bytes_read; It might make sense to keep this optional -- not sure.
Review of attachment 287291 [details] [review]: ::: gio/goutputstream.c @@ +900,3 @@ + { + if (nwritten == 0) + g_warning ("Write returned zero without error"); This could be possible if the original count was zero. From that standpoint, it may make sense to rewrite this patch as more of a state machine approach. I often find it helpful if the initial _async() entry point just sets up the state structures and immediately calls the "progress" function (write_all_callback in this case). This function inspects the state and does the appropriate thing. In the case that the state has zero bytes in it to start with then it will return the successful result. Otherwise, it will issue the async call. This has the advantage of reducing code duplication (eg: only one call to _write_async()) and also has the advantage of producing more consistent behaviour in cases that you may not have considered (eg: @count was originally given as zero). @@ +1011,3 @@ +g_output_stream_write_all_finish (GOutputStream *stream, + GAsyncResult *result, + gsize *bytes_written, Just as in the other patch, the nature of @bytes_written bothers me quite a lot here.
(In reply to comment #19) > Review of attachment 287291 [details] [review]: > > ::: gio/goutputstream.c > @@ +900,3 @@ > + { > + if (nwritten == 0) > + g_warning ("Write returned zero without error"); > > This could be possible if the original count was zero. this is actually not true, g_output_stream_write_async already checks this case. > > From that standpoint, it may make sense to rewrite this patch as more of a > state machine approach. I often find it helpful if the initial _async() entry > point just sets up the state structures and immediately calls the "progress" > function (write_all_callback in this case). This function inspects the state > and does the appropriate thing. In the case that the state has zero bytes in > it to start with then it will return the successful result. Otherwise, it will > issue the async call. > > This has the advantage of reducing code duplication (eg: only one call to > _write_async()) and also has the advantage of producing more consistent > behaviour in cases that you may not have considered (eg: @count was originally > given as zero). > > @@ +1011,3 @@ > +g_output_stream_write_all_finish (GOutputStream *stream, > + GAsyncResult *result, > + gsize *bytes_written, > > Just as in the other patch, the nature of @bytes_written bothers me quite a lot > here. I just kept consistency with other methods around, and with what write_all does.
(In reply to comment #20) > (In reply to comment #19) > > Review of attachment 287291 [details] [review] [details]: > > > > ::: gio/goutputstream.c > > @@ +900,3 @@ > > + { > > + if (nwritten == 0) > > + g_warning ("Write returned zero without error"); > > > > This could be possible if the original count was zero. > > this is actually not true, g_output_stream_write_async already checks this > case. And what will it return in the case that zero was requested from the user?
snippet from the code: if (count == 0) { g_task_return_int (task, 0); g_object_unref (task); return; }
mmm, actually now that I think on it, relying on it would mean to have a crash when trying to get the data so that would need to be checked.
Created attachment 287348 [details] [review] Provide api to read_all_async
Created attachment 287349 [details] [review] Provide write_all_async method
So after a discussion in #vala we came to the conclusion: - most people should not care about the partial buffer read in the case of error - in error you probably just want to close and cancel whatever you were doing - from the bound languages (Vala, python, gjs, etc.) this is possible - from C, we've never committed to anything more than an (out) parameter of a function throwing an exception being in an undefined state - in this case, we can make a special exception and define that state to be equal to the number of bytes that were successfully read So more or less, the approach here (and on the original API) is suspect, but probably the best that we can do given the circumstances. We definitely want some more documentation here, though, and specifically the partial-read-before-error thing needs to be documented as a C-only feature.
Created attachment 287363 [details] [review] Add g_input_stream_read_all_async() Add an asynchronous version of _read_all(). This API is not fully consistent with the normal expectations of a non-asynchronous version. Consistency between the sync and async version is probably more important. The API will still bind correctly, but access to all functionality will not be available: specifically, in the case of an error, higher level languages will be unable to determine how many bytes were successfully read before the error. Most users will probably not want to use this information anyway, so this is OK -- and if they do need the information, then they can just write the loop for themselves. Heavily based on a patch from Ignacio Casal Quinteiro.
Created attachment 287364 [details] [review] Add g_output_stream_write_all_async() Similar to the previous patch, this commit contains a minor violation of normal API conventions. See the explanation in the previous commit message. Heavily based on a patch from Ignacio Casal Quinteiro.
Created attachment 287365 [details] [review] docs: explain inconsistency of _{read,write}_all() These functions are inconsistent with our normal conventions in that they set an output variable to a specified value, even in the case that an error is thrown. Document very clearly that this should be considered exceptional.
Still todo: we could probably use some tests. Also, we may consider making the async calls into vfuncs. If async is going to be implemented in a thread anyway then we may as well have a single thread dispatch to do all of the reading in one place... That's actually something that we could check even without adding new vfuncs: see if the _async is our own thread-based emulation and do all of the work in a single thread dispatch if that's the case.
Review of attachment 287364 [details] [review]: See the minor comments. Apart from that the patches look good to me ::: gio/goutputstream.c @@ +913,3 @@ + g_object_unref (task); + } + is this empty line on purpose? @@ +1014,3 @@ + AsyncWriteAll *data = (AsyncWriteAll *)g_task_get_task_data (task); + + if (data != NULL) this check is a leftover that should be removed
Created attachment 289046 [details] [review] Add g_input_stream_read_all_async() Add an asynchronous version of _read_all(). This API is not fully consistent with the normal expectations of a non-asynchronous version. Consistency between the sync and async version is probably more important. The API will still bind correctly, but access to all functionality will not be available: specifically, in the case of an error, higher level languages will be unable to determine how many bytes were successfully read before the error. Most users will probably not want to use this information anyway, so this is OK -- and if they do need the information, then they can just write the loop for themselves. Heavily based on a patch from Ignacio Casal Quinteiro.
Created attachment 289047 [details] [review] Add g_output_stream_write_all_async() Similar to the previous patch, this commit contains a minor violation of normal API conventions. See the explanation in the previous commit message. Heavily based on a patch from Ignacio Casal Quinteiro.
Created attachment 289048 [details] [review] Add g_input_stream_read_all_async() Fixed a broken assertion, corrected for a review comment and implemented the loop-in-thread approach.
Created attachment 289049 [details] [review] Add g_output_stream_write_all_async() (same as other)
Created attachment 289050 [details] [review] Add tests for {read,write}_all_async()
Review of attachment 289048 [details] [review]: Looks good
Review of attachment 289049 [details] [review]: see the minor nitpick apart from that looks good. ::: gio/goutputstream.c @@ +915,3 @@ + + else + g_output_stream_write_async (G_OUTPUT_STREAM(stream), space before (
Review of attachment 289050 [details] [review]: would be cool to have also the windows version of the test but I guess it is fair enough :)
Attachment 287365 [details] pushed as 223b5f7 - docs: explain inconsistency of _{read,write}_all() Attachment 289048 [details] pushed as 76b890d - Add g_input_stream_read_all_async() Attachment 289049 [details] pushed as c8d1047 - Add g_output_stream_write_all_async() Attachment 289050 [details] pushed as b768d0e - Add tests for {read,write}_all_async()