Re: SoupInputStream / trying out gio

[Alexander Larsson <alexl redhat com>]

On Mon, 2007-12-03 at 13:01 -0500, Dan Winship wrote:
> Alexander Larsson wrote:
> > On Thu, 2007-11-29 at 13:18 -0500, Dan Winship wrote:
> >> It would be cleaner if I could just do:
> >>
> >>       if (!g_input_stream_set_pending (stream, TRUE, error))
> >>         return FALSE;
> > 
> > Yeah, that would remove some duplication in other in-tree streams too.
> > Care to hack up a patch for this?
> 
> OK, three patches attached. The first gets rid of some cheating, where
> code is currently temporarily clearing the "pending" flag so it can make
> a nested call; the patch makes it use the class->foo method directly
> rather than calling the wrapper methods. (This is what
> g_input_stream_real_skip() already did, so it seemed to make sense to do
> it elsewhere as well.)

I added an extra check for ->write != NULL in the splice case (because
write() already did that) and commited.

> Then the second patch implements
> g_input_stream_set_pending/g_input_stream_clear_pending. I made the
> set_pending method also check if the stream is already closed, since
> everywhere we check for pending also checks for that (except
> g_input_stream_close() and g_output_stream_close(), which handle that
> case specially). I wondered if maybe it could also do
> "g_push_current_cancellable()" (and be renamed to something more generic
> at that point), but the async calls don't currently use
> g_push_current_cancellable(), and I couldn't figure out what that was
> even for anyway, so I didn't.

This looks very nice. Commited.

The push/pop cancellable are so that a sync call implementation does not
have to carry with it the cancellable, but can instead use
g_cancellable_get_current(). This is often needed in implementation that
use some library where the design doesn't allow you to pass some data
into the lowlevel read calls.

> >> A related issue was that with soup_input_stream_send_async, I ended up
> >> needing to have a wrapper callback to clear the pending flag before
> >> calling the real callback, just like GInputStream does for its async
> >> ops. Maybe GSimpleAsyncResult could support that idiom directly, by
> >> providing an "implementation_callback" or whatever in addition to the
> >> caller-provided callback.
> > 
> > Yeah, that would be nice... Wanna hack? :)
> 
> This ended up not being workable the way I suggested, because
> g_input_stream_read_async, etc, don't have access to the
> GSimpleAsyncResult, because it's not created until the _real_read_async
> or whatever. But I realized we could still just do the cleanup in the
> _finish methods rather than wrapping the callbacks, so I implemented
> that, which is the third patch.
> 
> The patch ended up a little ugly though, because each _finish method had
> multiple exit points, so I needed to add lots of "goto done"s to ensure
> the cleanup stuff always happened. Another possibility would be to do
> the clear_pending and g_object_unref at the top of each _finish method,
> relying on the fact that it's safe to keep using the stream after
> unreffing it because the GAsyncResult is also holding a ref on it (since
> it's the result's source_object).
> 
> g_output_stream_splice_async() was also tricky because it needs to ref
> and unref both the output stream and the source stream. In this patch I
> fixed that by changing g_output_stream_splice_finish() to take the input
> stream as a parameter as well, but maybe that's bad.

I don't like this one. There is no guarantee that the _finish() call
will be called. You might not care about the result, or it might be
called multiple times. 

Also, for the splice case, the code that calls splice_finish() doesn't
get passed the input stream, and it might not even need it. So it has to
jump through extra hoops to keep it around just to pass it to _finish().
Seems bad to me.