GNOME Bugzilla – Bug 584284
g_data_input_stream_read_until_async behaves confusingly different from sync version
Last modified: 2018-02-02 09:06:28 UTC
I just tried to wrap the function in PyGObject and in unit tests noticed the utterly confusing difference between sync and async versions. While first "eats" the stopping character out of stream (i.e. advances pointer past it), asynchronous version _doesn't_. I believe this is so confusing and just wrong (e.g. if you change your program from using synchronous to asynchronous version you need to adjust result handling), that it must be fixed even though this will break backwards compatibility. A little code from unit tests (I don't attach full code since the wrappers themselves are not pushed into the repository yet): self.assertEquals('sentence', self.data_stream.read_until('.!?')) self.assertEquals('end of line', self.data_stream.read_until('\n\r')) ... self.assertEquals('sentence', do_read_until_async('.!?')) self.assertEquals('.end of line', do_read_until_async('\n\r')) ^^^ Note that in the first (synchronous) test dot (first stop character) is eaten out of the stream and so doesn't get into the second result. In the second test dot is not eaten and so appears in the second result.
I forgot to mention that in both tests the contents of the stream is 'sentence.end of line\nthe rest'.
Python wrappers and unit tests are attached to bug 584285. Python can be used to easily to test the described behavior.
i wrote the _async version after the synchronous one already existed, so it's my fault that they are functionally different. on the other hand, the existing documentation for the synchronous function suggests that the _async() version does the right thing. specifically: there is absolutely no mention of the stop characters being consumed or 'skipped'. i agree that this should be resolved. i'm not certain which way to resolve it.
Well, a change in the sync version would be a more significant compatibility break, since the function itself is older and because (just guessing) synchronous functions are more widely used. In either case, this should be done "now or never". The longer the problem remains unadressed, the larger the compatibility break will be.
Created attachment 147604 [details] [review] change the async version for what it's worth, here's an (untested) patch that changes the async version. i'm going to write to the ML for opinions. i don't feel comfortable breaking API without some backup. probably a change to the docs should also go in.
Created attachment 156429 [details] [review] patch
Review of attachment 156429 [details] [review]: yes. this is pretty much exactly what i had in mind. missing gtk-doc symbol additions, plus small fusses noted. i'll update the patch accordingly. ::: gio/gdatainputstream.c @@ +835,3 @@ { for (i = 0; checked < available && i < peeked; i++) + stop_end = stop_chars + stop_chars_len; may as well lift this to save the compiler the trouble... @@ +862,3 @@ + * + * function consumes the stop character (and does not return it). + * Note that in contrast to g_data_input_stream_read_upto(), this a note about future deprecation would be nice @@ +889,2 @@ checked = 0; + while ((found_pos = scan_for_chars (stream, &checked, stop_chars, strlen (stop_chars))) == -1) strlen(stop_chars) here is scary. perhaps read_until() should be implemented by calling read_upto() and then consuming the separator character if it was found... @@ +1188,1 @@ g_return_if_fail (cancellable == NULL || G_IS_CANCELLABLE (cancellable)); read_until_async should have pending-deprecation note too.
Created attachment 156832 [details] [review] [PATCH] Add g_data_input_stream_read_upto{,async,finish} I think this is OK.
looks ok to me
punting to 2.26
Ryan, Any news on this?
Ya. This should have gone in ages ago.
This change added a documentation comment that g_data_input_stream_read_until() should not be used and would be deprecated in a future version. I guess we've again missed the time to do that, but shouldn't we keep this bug open so we don't forget to do that?
Sound good
Comment on attachment 156832 [details] [review] [PATCH] Add g_data_input_stream_read_upto{,async,finish} This was pushed as 2e78d07f86d70de274f126a3ff00bd4af90a5c90 in 2010.
Created attachment 367762 [details] [review] gdatainputstream: Deprecate read_until() in favour of read_upto() g_data_input_stream_read_upto() was introduced in 2.26; now it’s GLib 2.56, we can probably deprecate the old versions (since the handling of consuming the stop character differs between the sync and async versions of it). Signed-off-by: Philip Withnall <withnall@endlessm.com>
Review of attachment 367762 [details] [review]: sure
Pushed, thanks! Attachment 367762 [details] pushed as 5ed77c1 - gdatainputstream: Deprecate read_until() in favour of read_upto()