GNOME Bugzilla – Bug 741630
Add GSimpleIOStream class
Last modified: 2015-02-17 21:33:56 UTC
Add an io stream that allows to set the input and output stream. The problem from this implementation is that the GIOStream already sets the input and output stream properties and we cannot override them. Is there a way to override them and making them read/write ? Should I use a different property name?
Created attachment 292873 [details] [review] Add GSimpleStream class GSimpleIOStream represents an object that wraps an input and an output stream making easy to use them by calling the #GIOStream methods.
Created attachment 292876 [details] [review] Add GSimpleIOStream class GSimpleIOStream represents an object that wraps an input and an output stream making easy to use them by calling the #GIOStream methods.
so now it seems to be fine, though should we make it construct_only or just construct and let the user the ability to change the streams?
Review of attachment 292876 [details] [review]: ::: gio/giotypes.h @@ +135,3 @@ typedef struct _GIOStream GIOStream; +/** + * GSimpleIOStream: should move this doc annotation inside gsimpleiostream.h. ::: gio/gsimpleiostream.c @@ +19,3 @@ + + +/* Prologue {{{1 */ copy and paste issue? there's no terminator for the code folding. @@ +216,3 @@ + io_class->close_finish = g_simple_io_stream_close_finish; + + g_object_class_install_property (gobject_class, PROP_INPUT_STREAM, missing documentation. @@ +225,3 @@ + G_PARAM_CONSTRUCT_ONLY)); + + g_object_class_install_property (gobject_class, PROP_OUTPUT_STREAM, missing documentation. @@ +241,3 @@ +} + +GIOStream * missing documentation. ::: gio/gsimpleiostream.h @@ +46,3 @@ + GIOStream parent; + + GSimpleIOStreamPrivate *priv; please, don't use a priv pointer in the instance structure. we have the get_instance_private() function for that.
Created attachment 292877 [details] [review] Add GSimpleIOStream class GSimpleIOStream represents an object that wraps an input and an output stream making easy to use them by calling the #GIOStream methods.
Created attachment 292878 [details] [review] Add GSimpleIOStream class GSimpleIOStream represents an object that wraps an input and an output stream making easy to use them by calling the #GIOStream methods.
Created attachment 292879 [details] [review] Add GSimpleIOStream class GSimpleIOStream represents an object that wraps an input and an output stream making easy to use them by calling the #GIOStream methods.
Not really reviewing here, just a comment from the sidelines: "Enough streams in GIO already!"
Review of attachment 292879 [details] [review]: Missing docs section addition. ::: gio/gsimpleiostream.c @@ +51,3 @@ +}; + +G_DEFINE_TYPE_WITH_PRIVATE (GSimpleIOStream, g_simple_io_stream, G_TYPE_IO_STREAM) Sorry to hit you with this after ebassi already told you different: I'd prefer if you just moved the struct definition inside of the .c file (along with the class typedef) and avoided priv use entirely. That would effectively make the class unsubclassable, as is often the case for our "Simple" classes. See GSimpleAction, GSimpleAsyncResult, etc. for how that is done. @@ +158,3 @@ + /* If this errored out, unset error so that we don't report + further errors, but still do the following ops */ + if (error != NULL && *error != NULL) This is a bit tricky, but I think you can do better. If the other close is successful you probably still want to report the first error. For that reason I guess the better approach here would actually be to set 'error = NULL' here in order to keep the error in place and prevent the input_stream_close() from modifying it. That would definitely require a comment explaining what is happening though because it's non-standard usage and somewhat confusing. @@ +183,3 @@ + /* socket close is not blocked, just do it! */ + error = NULL; + if (!class->close_fn (stream, cancellable, &error)) I'm pretty sure I don't like whatever it is you're trying to do here.
Might be nice as well to add two small functions: #ifdef G_IS_WIN32 g_io_stream_new_for_handle() #else g_io_stream_new_for_unix_fd() #endif which do the 'obvious' thing.
Created attachment 292990 [details] [review] Add GSimpleIOStream class GSimpleIOStream represents an object that wraps an input and an output stream making easy to use them by calling the #GIOStream methods.
Review of attachment 292990 [details] [review]: ::: gio/gsimpleiostream.c @@ +33,3 @@ + * + * GSimpleIOStream represents an object that wraps an input and an output + * stream making easy to use them by calling the #GIOStream methods. The English here could use some simplification and clarification. As simply put as I can think: GSimpleIOStream creates a #GIOStream from an arbitrary #GInputStream and #GOutputStream. This allows any pair of input and output streams to be used with #GIOStream methods. And then it'd be nice to go on and show/discuss the cases of why this might be interesting, which would be good to see here. Presumably this is to make it easier to assemble some kind of shell-like stream pipeline? @@ +259,3 @@ + GIOStreamClass *io_class = G_IO_STREAM_CLASS (klass); + + gobject_class->finalize = g_simple_io_stream_dispose; Sort of surprised nobody has pointed out yet that you're passing the dispose func as your finalizer here.
Created attachment 292992 [details] [review] Add GSimpleIOStream class GSimpleIOStream represents an object that wraps an input and an output stream making easy to use them by calling the #GIOStream methods.
Thanks for the review. Nice catch with the finalize typo. This patch is still missing some better explanation as you pointed out though.
Created attachment 294852 [details] [review] Add GSimpleIOStream class GSimpleIOStream represents an object that wraps an input and an output stream making easy to use them by calling the #GIOStream methods.
Review of attachment 294852 [details] [review]: Something to consider: moving the close implementations into the interface as reasonable defaults. ::: gio/gsimpleiostream.c @@ +181,3 @@ + + /* propagate only the first error if already set */ + if (error != NULL && *error == NULL) This check is a bit confusing, but it seems correct. I'd probably have just done the check the other way and set error itself to NULL, skipping the need for the other variable. @@ +247,3 @@ + g_task_set_task_data (task, data, free_async_data); + + g_output_stream_close_async (simple_stream->output_stream, Is there a reason that you close the input before the output? Why not both at once? You already have an async data structure so you could just put a counter in it, set to 2, and use the same ready func -- whichever one gets to 0 is the one that dispatches the result.
Created attachment 294958 [details] [review] Add GSimpleIOStream class GSimpleIOStream represents an object that wraps an input and an output stream making easy to use them by calling the #GIOStream methods.
Created attachment 294977 [details] [review] streams: add private 'async close via threads' API Add an internal helper to find out if close_async() is implemented via threads using the default implementation in the base class. We will use this to decide if we should do a 'pure async' close of a GIOStream or not.
Created attachment 294978 [details] [review] GIOStream: support for unemulated async close() Add an implementation of non-thread-emulated async close of a GIOStream if either of the underlying stream objects support it. This prevents us from calling close() functions from another thread on an object that may not be expecting that. It also allows us to skip the thread entirely in case our objects support a pure async close.
Created attachment 294979 [details] [review] Add GSimpleIOStream class GSimpleIOStream represents an object that wraps an input and an output stream making easy to use them by calling the #GIOStream methods.
Created attachment 294980 [details] [review] tests: add GSimpleIOStream async close tests Just a couple of tests to make sure the two paths are working properly, without crashes or leaks.
Review of attachment 294977 [details] [review]: Looks good.
Review of attachment 294977 [details] [review]: ::: gio/ginputstream.c @@ +1254,3 @@ } +/** this will make gtk-doc pick the annotation up, but the function is private, so you'll get a warning. ::: gio/goutputstream.c @@ +1706,3 @@ } +/** same as with g_input_stream_async_close_is_via_threads(): gtk-doc will pick the annotation up.
Review of attachment 294978 [details] [review]: Looks good, just wondering whether we should use g_slice_new for consistency?
Review of attachment 294979 [details] [review]: Looks good.
Review of attachment 294978 [details] [review]: ::: gio/giostream.c @@ +547,3 @@ +{ + GError *error; + gint todo; maybe "pending_close" instead of "todo"? :-) @@ +624,3 @@ + * threadsafe. + */ + data = g_new (CloseAsyncData, 1); how are we doing with the GSlice investigation? is it worth using g_slice_new instead of g_new? it makes the free function explicit, though…
Review of attachment 294980 [details] [review]: See the comment ::: gio/tests/io-stream.c @@ +112,3 @@ +test_close_file (void) +{ +#ifdef G_OS_UNIX any reason to test it only for unix?
(In reply to comment #26) > maybe "pending_close" instead of "todo"? :-) sure > how are we doing with the GSlice investigation? is it worth using g_slice_new > instead of g_new? it makes the free function explicit, though… I just did it out of laziness for the DestroyNotify. I could just as easily free the task data when I unref the task, though, so I'll do that instead.
(In reply to comment #23) > this will make gtk-doc pick the annotation up, but the function is private, so > you'll get a warning. I copied the comment block from the existing 'read is via threads' helper -- turns out that this functions is indeed showing up in -unused :)
(In reply to comment #27) > any reason to test it only for unix? it opens /dev/null
Created attachment 295033 [details] [review] streams: de-gtkdocify internal API Remove the /** **/-style block from two internal helpers to prevent gtk-doc from picking them up. Also add the gioprivate.h header to the IGNORE_HFILES to keep the functions out of gio-unused.txt.
Created attachment 295034 [details] [review] streams: add private 'async close via threads' API Add an internal helper to find out if close_async() is implemented via threads using the default implementation in the base class. We will use this to decide if we should do a 'pure async' close of a GIOStream or not.
Created attachment 295035 [details] [review] GIOStream: support for unemulated async close() Add an implementation of non-thread-emulated async close of a GIOStream if either of the underlying stream objects support it. This prevents us from calling close() functions from another thread on an object that may not be expecting that. It also allows us to skip the thread entirely in case our objects support a pure async close.
Created attachment 295036 [details] [review] Add GSimpleIOStream class GSimpleIOStream represents an object that wraps an input and an output stream making easy to use them by calling the #GIOStream methods.
Created attachment 295037 [details] [review] tests: add GSimpleIOStream async close tests Just a couple of tests to make sure the two paths are working properly, without crashes or leaks.
Review of attachment 295036 [details] [review]: The patch looks good to me and it is pretty much the code we are running in production. Just a couple of nitpicks/questions below ::: gio/gsimpleiostream.c @@ +215,3 @@ +g_simple_io_stream_new (GInputStream *input_stream, + GOutputStream *output_stream) +{ should we add g_return_if_fail checks? ::: gio/gsimpleiostream.h @@ +32,3 @@ +#define G_TYPE_SIMPLE_IO_STREAM (g_simple_io_stream_get_type ()) +#define G_SIMPLE_IO_STREAM(obj) (G_TYPE_CHECK_INSTANCE_CAST ((obj), G_TYPE_SIMPLE_IO_STREAM, GSimpleIOStream)) +#define G_IS_SIMPLE_IO_STREAM(obj) (G_TYPE_CHECK_INSTANCE_TYPE ((obj), G_TYPE_SIMPLE_IO_STREAM)) Should we use the new G_DECLARE_FINAL_TYPE macro?
Also the rest of the patchset (preliminary patches and the tests) look ok to me (we run the tests and they pass here too)
Review of attachment 295035 [details] [review]: this patch "breaks" gdbus-close-pending test: /gdbus/close-pending: (/home/elmarco/src/gnome/glib/gio/tests/.libs/lt-gdbus-close-pending:6063): GLib-GObject-CRITICAL **: g_object_unref: assertion 'G_IS_OBJECT (object)' failed (gdb) bt ...
+ Trace 234648
It turns out continue_writing() doesn't pay enough attention to reading side, I am sending two fixes that should be applied before this patch.
Created attachment 296716 [details] [review] gdbus: delay closing stream after read finish Closing the stream on the writing side my race with a pending read. This patch ensures that closing is delayed after reading is finished.
Created attachment 296717 [details] [review] tests: fix unref of optionnal cancellable An output_stream_close_async() may take NULL cancellable. And my_slow_close_output_stream_close_async() does: df->cancellable = (cancellable != NULL ? g_object_ref (cancellable) : NULL); So, don't expect df->cancellable to be non-null.
Fwiw, the last patches look good to me, +1 this would be quite useful.
Attachment 295033 [details] pushed as f56f1ef - streams: de-gtkdocify internal API Attachment 295034 [details] pushed as cb40c55 - streams: add private 'async close via threads' API Attachment 295035 [details] pushed as c2c0a6a - GIOStream: support for unemulated async close() Attachment 295036 [details] pushed as d4e3b82 - Add GSimpleIOStream class Attachment 295037 [details] pushed as 07ae2e1 - tests: add GSimpleIOStream async close tests Also applied the two GDBus fixes as part of bug 743990.