Bug 30430

Created attachment 39017 [details] [review]
Avoid using g_simple_async_report_error_in_idle()

Avoid using g_simple_async_report_error_in_idle()

g_simple_async_report_error_in_idle() does not work with
g_simple_async_result_is_valid() because it miss the source_tag parameter.
Not using it will allow safer result check in the _finish() operation.

Created attachment 39019 [details] [review]
Keep GCancellable alive during the async calls

Keep a reference on GCancellable for the time of the async calls. This is
important to allow simple cancel and forget of cancellable by caller. Most
call where already implemented correctly, this patch fixes the remaining.

Created attachment 39020 [details] [review]
Fix wocky_implement_finish_*() macro

Fix wocky_implement_finish_*() macro

* Make the wocky_implement_*() macro safer by calling _is_valid() before
  trying to propagate errors.
* Fixed missing scope (G_STMT_START/G_STMT_END) and missing parenthesis
* Don't call copy_func if the result is NULL (enabling use of g_object_ref
  as copy_func).
* Added missing ; now required when those macro are called.

I've skim-read but not reviewed in detail yet.

(In reply to comment #0)
> 1. _is_valid() is called after _propagate_error()

I was worried about this when Guillaume re-ordered these calls, so I investigated. As far as I can tell, these are the possible situations: only the last one is problematic.

(By "invalid path", I mean "can't be reached unless the programmer is using it wrong".)

* Valid path: result is valid, error is set. Error is correctly propagated either way.

* Valid path: result is valid, error is not set. propagate_error returns FALSE, validity is checked, finish succeeds.

* Invalid path: result has been disposed already. propagate_error fails its initial g_return_if_fail, criticals and returns FALSE. As a result, is_valid is called; it *also* fails its check, and the finish function criticals and returns too.

* Invalid path: result has the wrong source tag and no error. propagate_error returns FALSE, the delayed check runs anyway, and all is good.

* Invalid path: result has the wrong source tag but has an error set. As a result of the order we use, a NULL source tag isn't diagnosed (good for g_simple_async_report_in_idle), but an incorrect non-NULL source tag isn't diagnosed either (failure to diagnose certain programmer errors, but then, this *is* C).

Avoiding g_simple_async_report_error_in_idle, just for this, seems to me to make our code less clear for very little gain?

> 2. Cancellable are not always reffed during the async calls
> 
> When cancelling a call, it is correct to do "g_cancellable_cancel(c);
> g_object_unref (c);". Some of the async functions are not safe for that since
> they don't acquire a reference on the cancellable. Patch 002 fixes all the
> calls that where not doing proper ref/unref of cancellables.

This looks good in general, and I approve of making use of the return value of g_object_ref.

> +  if (job->cancellable)
> +    g_object_unref (job->cancellable);
> +
> +  job->source_object = NULL;                 <------
> +  job->cancellable = NULL;

I'd prefer to keep all the stuff dealing with source_object together, *then* do all the stuff dealing with cancellable (in other words, move the indicated line before the "if").

> +  if (cancellable != NULL)
> +    priv->output_cancellable = g_object_ref (cancellable);
>  
> -  priv->output_cancellable = cancellable;

This does change the semantics for the case where cancellable is NULL: if output_cancellable isn't already NULL, it's not overwritten. This seems to be checked for already, but it might be worth asserting.

> 3. The macro is missing scope

This is secretly several commits...

> * Make the wocky_implement_*() macro safer by calling _is_valid() before
>   trying to propagate errors.

As explained above, I'm not sure how useful this actually is.

> * Fixed missing scope (G_STMT_START/G_STMT_END) and missing parenthesis

This bit certainly seems worthwhile to me, although I think you may mean "parentheses" (parenthesis is the singular).

> +  GSimpleAsyncResult *__simple; \

You didn't mention in the commit message that you switched to double-underscored variable names. All double-underscored names are reserved for use by the compiler/C library, so this is wrong. Use a single underscore if you want to avoid collisions with library-user-defined variable names (but to be honest, these macros are the entire body of a function, so I don't really see what they'd be colliding with).

(In reply to comment #4)
> Avoiding g_simple_async_report_error_in_idle, just for this, seems to me to
> make our code less clear for very little gain?

Looking at the patch again, I don't see any difference on the code clearness (please specify what is unclear to you). Actually, it's somewhat better since at some places we just leak or unref a GSimpleAsyncResult that we had created for the case there was no error. With that code we use it.

The goal here is to prepare for wocky_implement_finish_*() macro to get moved to GLib, but this won't happen until we flip the _is_valid() call with the propagate call. The internal GLib code analyses is wrong approach, you need to respect the way they where designed to be used.

> I'd prefer to keep all the stuff dealing with source_object together, *then* do
> all the stuff dealing with cancellable (in other words, move the indicated line
> before the "if").

Ok, will do.

> This does change the semantics for the case where cancellable is NULL: if
> output_cancellable isn't already NULL, it's not overwritten. This seems to be
> checked for already, but it might be worth asserting.

Concurrent async calls are generally not allowed in Wocky, so yes we should generally assert for cancellable and result to be NULL (we usually assert/error for only one of them). I can give a pass for that, to make sure we are consistent.

> 
> > 3. The macro is missing scope
> 
> This is secretly several commits...

I apologies, macros are a pain to edit and split properly.

> 
> > * Make the wocky_implement_*() macro safer by calling _is_valid() before
> >   trying to propagate errors.
> 
> As explained above, I'm not sure how useful this actually is.

This is by design the way it should be used, and #1 blocker to get this utility into GLib.

> 
> > +  GSimpleAsyncResult *__simple; \
> 
> You didn't mention in the commit message that you switched to
> double-underscored variable names. All double-underscored names are reserved
> for use by the compiler/C library, so this is wrong. Use a single underscore if
> you want to avoid collisions with library-user-defined variable names (but to
> be honest, these macros are the entire body of a function, so I don't really
> see what they'd be colliding with).

Aren't Wocky a library ? Honestly I just forgot about this change (was going to revert it at some point). This change was done while trying to mimic GLib macro style, which consistently use double __ inside macros. I can move to single _. I'll move __p to.

(In reply to comment #5)
> (In reply to comment #4)
> > I'd prefer to keep all the stuff dealing with source_object together, *then* do
> > all the stuff dealing with cancellable (in other words, move the indicated line
> > before the "if").
> 
> Ok, will do.

Done: http://git.collabora.co.uk/?p=user/nicolas/wocky.git;a=commitdiff;h=9afeba07b3faef08d3a4341c157da28474c835c3

> 
> > This does change the semantics for the case where cancellable is NULL: if
> > output_cancellable isn't already NULL, it's not overwritten. This seems to be
> > checked for already, but it might be worth asserting.
> 
> Concurrent async calls are generally not allowed in Wocky, so yes we should
> generally assert for cancellable and result to be NULL (we usually assert/error
> for only one of them). I can give a pass for that, to make sure we are
> consistent.

Done: http://git.collabora.co.uk/?p=user/nicolas/wocky.git;a=commitdiff;h=1f30157337330787d7a2da95e07df5cec7934524

> > 
> > > +  GSimpleAsyncResult *__simple; \
> > 
> > You didn't mention in the commit message that you switched to
> > double-underscored variable names. All double-underscored names are reserved
> > for use by the compiler/C library, so this is wrong. Use a single underscore if
> > you want to avoid collisions with library-user-defined variable names (but to
> > be honest, these macros are the entire body of a function, so I don't really
> > see what they'd be colliding with).
> 
> Aren't Wocky a library ? Honestly I just forgot about this change (was going to
> revert it at some point). This change was done while trying to mimic GLib macro
> style, which consistently use double __ inside macros. I can move to single _.
> I'll move __p to.

Done: http://git.collabora.co.uk/?p=user/nicolas/wocky.git;a=commitdiff;h=195f8353ac7a9f15956ca14bea493fa1976e87cb
(will be sqashed when this review is done)

(In reply to comment #5)
> The goal here is to prepare for wocky_implement_finish_*() macro to get moved
> to GLib, but this won't happen until we flip the _is_valid() call with the
> propagate call.

If GLib upstream are basically saying "g_simple_async_report_error_in_idle is never right" then I suppose we can do this... but which of the situations in comment #4 is actually a problem?

Would there be anything wrong with having the version in Wocky be more permissive than the version that eventually goes into GLib? If GLib is going to grow a more permissive g_simple_async_result_is_valid(), then it can get away with its version of implement_finish being in an order that relies on that.

(In reply to comment #7)
> If GLib upstream are basically saying "g_simple_async_report_error_in_idle is
> never right" then I suppose we can do this... but which of the situations in
> comment #4 is actually a problem?

The problem is more conceptual then real. So I've reworked the patch set to keep the call order, fixing the macro and the Cancellable ref.

Here's the new branch:

http://git.collabora.co.uk/?p=user/nicolas/wocky.git;a=shortlog;h=refs/heads/finish2

Created attachment 39266 [details] [review]
Keep GCancellable alive during the async calls

Keep a reference on GCancellable for the time of the async calls. This is
important to allow simple cancel and forget of cancellable by caller. Most
call where already implemented correctly, this patch fixes the remaining.

Created attachment 39267 [details] [review]
Fix wocky_implement_finish_*() macro

* Fixed missing scope (G_STMT_START/G_STMT_END) and missing parenthesis
* Don't call copy_func if the result is NULL (enabling use of g_object_ref
  as copy_func).
* Added missing ; now required when those macro are called.

Created attachment 39268 [details] [review]
Add assertions to ensure async resources are freed

Add assertions to make sure async resources GCancellable and
GSimpleAsyncResult are freed before a new async calls are attempted.

Comment on attachment 39266 [details] [review]
Keep GCancellable alive during the async calls

This was included in my leak fix branch, already upstream now.

Created attachment 41308 [details] [review]
Fix wocky_implement_finish_*() macro

Rebased/retested against current tree.

Created attachment 41309 [details] [review]
Add assertions to ensure async resources are freed

Rebased/retested against current tree.

Review of attachment 41308 [details] [review]:

Looks good apart from a small style point.

::: wocky/wocky-utils.h
@@ +90,3 @@
+    FALSE); \
+  return TRUE; \
+} G_STMT_END

Much as I hate to nit-pick, the bodies of the macros should be indented by four spaces.

Review of attachment 41309 [details] [review]:

::: wocky/wocky-porter.c
@@ +1421,3 @@
+  /* The cancellable must be NULL, otherwise it will be leaked */
+  g_assert (priv->close_cancellable == NULL);
+

If there's a comment here at all, it should be to say that, since we guarded against close_async() being called more than once, this assertion should be safe. Ditto the comment below.

Created attachment 41310 [details] [review]
Fix wocky_implement_finish_*() macro

Added 4 spaces indent to macro body. It indeed make the navigation easier since you can spot the next macro definition very quickly.

Review of attachment 41310 [details] [review]:

looks good.

Created attachment 41311 [details] [review]
Add assertions to ensure async resources are freed

Simply removed trivial comment ;)

Created attachment 41312 [details] [review]
Add assertions to ensure async resources are freed

Actually there was two trivial comments to remove. Fixed.

Merged in commit 8db5c13fe1895625a80d66ce2aef376cae6e9f8f