Created attachment 324592 [details] [review]
codegen: Use GTask instead of GSimpleAsyncResult if 2.36 target is selected

GTask brings some differences compared to GSimpleAsyncResult. Most namely,
g_task_return*() operations perform at once the async result data asignment
and the caller's main context activation. This is something that has to be
done exactly once, so the code flow has slight changes to ensure that.

Also, the async operation data used to be attached early through
g_simple_async_result_set_op_res_gpointer, only to be maybe replaced by
the real return data. If GTask is being used, we set this data through
g_task_set_task_data().

Created attachment 324593 [details] [review]
Require and target glib >= 2.36

This enables the use of GTask by default, and gets rid of plenty of
deprecation warnings.

I btw made sure that make check runs ok both with and without the last patch

Review of attachment 324593 [details] [review]:

Thanks for your work on bringing GTask to Vala.

The patch to increase the GLib version Vala depends on has been rejected 
because support is provided for building Vala on older distributions. The
example given on IRC #vala channel was a PPA for Ubuntu 12.04. The GLib
dependency was increased from 2.24 to 2.32 in February 2016. See
https://git.gnome.org/browse/vala/commit/?id=0b297f4273a868c90b8e96adf52380839fcc38c8
It is likely this won't be changed again for the next year or more.

This means anyone reviewing the main GTask patch should use the valac
--target-glib switch to use the new GTask code.

(In reply to Al Thomas from comment #4)
> Review of attachment 324593 [details] [review] [review]:
> 
> Thanks for your work on bringing GTask to Vala.
> 
> The patch to increase the GLib version Vala depends on has been rejected 
> because support is provided for building Vala on older distributions. The
> example given on IRC #vala channel was a PPA for Ubuntu 12.04. The GLib
> dependency was increased from 2.24 to 2.32 in February 2016. See
> https://git.gnome.org/browse/vala/commit/
> ?id=0b297f4273a868c90b8e96adf52380839fcc38c8
> It is likely this won't be changed again for the next year or more.

Can't say much about this, besides: it's unfortunate that vala today chooses to use by default API that has been deprecated for ~3.5y unless some toggles are passed.

Maybe it should be possible to pass the default target glib in autogen/configure parameters, or autodetect that from installed glib at configure time? that'd make things friendly to people using vala with contemporaneous glib, while still allowing targeting older glibs for the PPA case.

I see no point in supporting ubuntu 12.04 as it is a) just maintained by ubuntu itself b) you can't even make apt-get update on it properly as repos are down.

If that's the only argument I think Vala should raise it's compatibility bar to ubuntu 14.04 which is the oldest normally supported LTS.

Carlos, Marcin, I agree with your sentiments. Please see https://bugzilla.gnome.org/show_bug.cgi?id=764420

I see their point that valac should make deterministic output regardless of GLib present in the system.

But as far as I understand discussion, they are not against doing this if --target-glib was explicitely specified. There are similar things in Vala already, e.g. you cannot call new Thread<void*>(something) unless you pass --target-glib 2.32 and this is accepted.

So maybe we should do the same with this patch? I mean that unless one explicitely sets --target-glib to the recent version, we keep the current behaviour?

I will reference this discussion in the another bug, maybe Jurg will share his thoughts.

(In reply to Marcin Lewandowski from comment #8)
> But as far as I understand discussion, they are not against doing this if
> --target-glib was explicitely specified. There are similar things in Vala
> already, e.g. you cannot call new Thread<void*>(something) unless you pass
> --target-glib 2.32 and this is accepted.

The latest stable major release of Vala, version 0.32.0, has GLib 2.32 as the default target. So once that release gets in to the distributions you will no longer need --target-glib 2.32 for the example you gave. It is good to know, though, that you didn't have a problem finding out about that feature toggle.
 
> So maybe we should do the same with this patch? I mean that unless one
> explicitely sets --target-glib to the recent version, we keep the current
> behaviour?

The patch already does this. That's what the context.require_glib_version (2, 36) checks are for.

I'm thinking now there should just be a warning that the old code path has been selected and that to use the new GTask code --target-glib 2.36 should be used. Something like:

warning: old async code generation has been selected, use --target-glib=2.36 to use the new GTask async code generation

This is similar to the test case given in https://bugzilla.gnome.org/show_bug.cgi?id=764420 for GtkTemplate. Vala gives an error:
error: glib 2.38 is required for using Gtk templates (with --target-glib=2.38)

Having a warning at least makes people aware the new code is available. If they are already using the --target-glib switch for 2.36+, because they use GtkTemplates for example, they will also get the GTask code without the option of downgrading. I don't know if that's a problem.

If a warning is agreed then an updated patch would be good. Then get a few people with good async knowledge to run the patch and say it works well, then push it out in a development release for wider testing. Hopefully the warning will get people interested.I don't know if you've seen, but someone on the mailing list has expressed an interest in doing work on async callbacks over the summer. So getting this patch tested and accepted before then would be good.

I think warning will do the job. Default can stay as it is + warning, but if new code is requested there's no reason to use deprecated one.

Created attachment 325486 [details]
Example of async methods used to create a generator

Example from https://wiki.gnome.org/Projects/Vala/AsyncSamples

Review of attachment 324592 [details] [review]:

The async generator example from https://wiki.gnome.org/Projects/Vala/AsyncSamples
compiles with this patch, but when run produces these runtime critical warnings:

Result: 0

(process:10286): GLib-GIO-CRITICAL **: g_task_return_pointer: assertion 'task->result_set == FALSE' failed
Result: 0

(process:10286): GLib-GIO-CRITICAL **: g_task_return_pointer: assertion 'task->result_set == FALSE' failed
Result: 0
Segmentation fault (core dumped)

Hmmm, AFAICT that one highly relies in vala being able to issue g_simple_async_result_complete() in the lack of a main loop.

The difference here with GTask is that it does its own checks to maybe complete the task within g_task_return_pointer(), as can be seen in:

https://git.gnome.org/browse/glib/tree/gio/gtask.c#n1154

What happens in the specific situation of that test is that:

1) g_task_new() is created when there's no thread default main context, so the task stores the global default context in:
   https://git.gnome.org/browse/glib/tree/gio/gtask.c#n696

2) In g_task_return() (the link above), it checks whether the task context is the same than the current one. As there's no current main context, the task is not returned immediately.

3) A bit farther in that function, in:
   https://git.gnome.org/browse/glib/tree/gio/gtask.c#n1171
   it schedules the task to be finished in the next main context loop, which is never dispatched.

4) Because the task never ends, the int_generator_generate_ready() function in the generated C sources is never called, so it doesnt iterate properly and ends up poking the same data and the same task.

Now, AFAICT this would all work fine if the code was driven by a main loop. As it would do if GTask detected this situation and called g_task_return_now() immediately.

I think this comes down to talking with the glib developers, and see if this non-mainloop-driven GTask usage is something they'd want to support or not.

I'm getting a crash in a _finish function where _data_ is null:

_data_ = g_task_propagate_pointer (G_TASK (_res_), NULL);
result = _data_->result;

>If the task resulted in an error, or was cancelled, then this will instead return NULL and set error.

We need to check if _data_ is NULL in the case of cancelled tasks

Created attachment 326901 [details] [review]
NULL check for cancelled GTasks

(In reply to Marcin Lewandowski from comment #0)
> Vala 0.30.0 + GLib 2.46.2 issues the following warning if async methods are
> used:
> 
> warning: 'g_simple_async_result_complete' is deprecated
> [-Wdeprecated-declarations]
>                 g_simple_async_result_complete (_data_->_async_result);

Note that if vala knows it is generating code for a specific target version of GLib, it could declare that in the generated code. Eg

  #undef GLIB_VERSION_MIN_REQUIRED
  #define GLIB_VERSION_MIN_REQUIRED GLIB_VERSION_2_44
  #include <gio/gio.h>

(In reply to Carlos Garnacho from comment #13)
> I think this comes down to talking with the glib developers, and see if this
> non-mainloop-driven GTask usage is something they'd want to support or not.

I don't think that makes sense. GTask behavior is explicitly defined in terms of GMainContext. (While the docs maybe aren't *100%* explicit about the fact that the callback is always called in a particular GMainContext, that's definitely the intention. I'm pretty sure that was entirely the intention with GSimpleAsyncResult too, it just didn't implement it automatically, so it was possible to subvert it...)

> Now, AFAICT this would all work fine if the code was driven by a main
> loop. As it would do if GTask detected this situation and called
> g_task_return_now() immediately.

GTask couldn't "detect" this, because it's indistinguishable from other cases where the current behavior is expected, like calling an _async() function from main() before starting the main loop. So if it was going to support this, it would have to be by adding "g_task_set_non_main_context_based()" or something to have the caller indicate that it wants the new behavior.

(In reply to Dan Winship from comment #18)
> (In reply to Carlos Garnacho from comment #13)
> > I think this comes down to talking with the glib developers, and see if this
> > non-mainloop-driven GTask usage is something they'd want to support or not.
> 
> I don't think that makes sense. GTask behavior is explicitly defined in
> terms of GMainContext. (While the docs maybe aren't *100%* explicit about
> the fact that the callback is always called in a particular GMainContext,
> that's definitely the intention. I'm pretty sure that was entirely the
> intention with GSimpleAsyncResult too, it just didn't implement it
> automatically, so it was possible to subvert it...)

FWIW, I suspected the answer, and I do agree. Doing async calls without a running main loop to catch the results is a bit of handwaving at best, AFAICS the failing demo badly relies on turning async into sync through yield, which is fine I guess if all your async calls do that, but doesn't seem to scale to real-life situations.

Alas, this feature has been advertised, so we can't tell that there's no running code out there that does this. Nor we can add other than runtime warnings in the case this was deemed a not-so-necessary feature.

> 
> > Now, AFAICT this would all work fine if the code was driven by a main
> > loop. As it would do if GTask detected this situation and called
> > g_task_return_now() immediately.
> 
> GTask couldn't "detect" this, because it's indistinguishable from other
> cases where the current behavior is expected, like calling an _async()
> function from main() before starting the main loop. So if it was going to
> support this, it would have to be by adding
> "g_task_set_non_main_context_based()" or something to have the caller
> indicate that it wants the new behavior.

Same goes for vala generated code. I'm attaching a patch on top of the others that forces "return now" behavior on the same situations than the old code paths by iterating on the GTask main context until the task completes.

However, I've only done this to the code path affecting this demo. The other code path in valagasyncmodule.vala invariantly used g_simple_async_result_complete(), I'd rather not take this approach for the GTask paths if it's not essential.

With this patch the generator demo works just fine, as does Tracker (a heavy vala/async user) when built with these patches. If the approach looks ok-ish I'll squash the patches together and reattach.

Created attachment 336630 [details] [review]
codegen: Force sync behavior of GTasks

There's code out there relying on immediate return here when state is
!=0. As GTask always defers the finalization to an idle in its main
context, ensure the source is dispatched and the task completed before
returning.

Comment on attachment 326901 [details] [review]
NULL check for cancelled GTasks

Thanks for the patch BTW :), I however think it makes sense to just always do the NULL check, it will just never be true if there's no error from the GTask. Seems easier than iterating through the params to find a cancellable, which will most commonly be there.

I did this change locally.

Comment on attachment 336630 [details] [review]
codegen: Force sync behavior of GTasks

seems kind of scary... Another possibility would be to just import GSimpleAsyncResult into vala (and rename it and strip out the parts you aren't using, which is 90% of it).

I admit it is... downside of that approach is that it's generated code which would need such object, so we're talking effectively about replicating an object, and possibly multiple namespaced instances of it in order to avoid function and g_type_register* clashes.

ah. well, in that case, it's not like there's any plan to ever actually remove anything from glib, so you could just continue to use GSimpleAsyncResult but emit "G_GNUC_BEGIN_IGNORE_DEPRECATIONS"/"G_GNUC_END_IGNORE_DEPRECATIONS" around the calls.

In some ways it would be nice to create a separate valagtaskasyncmodule.vala and enable that with an --enable-experimental-gtask-async-module switch to valac.

I'm not sure how practical that is. Ideally there would be an AsyncModule interface and the old and new objects would implement that to make them interchangeable.

The inheritance chain for GAsyncModule is:
Vala.GAsyncModule -> Vala.GtkModule -> Vala.GSignalModule -> Vala.GObjectModule -> Vala.GTypeModule -> Vala.GErrorModule -> Vala.CCodeDelegateModule -> Vala.CCodeArrayModule -> Vala.CCodeMethodCallModule -> Vala.CCodeAssignmentModule -> Vala.CCodeMemberAccessModule -> Vala.CCodeControlFlowModule -> Vala.CCodeMethodModule -> Vala.CCodeStructModule -> Vala.CCodeBaseModule -> Vala.CodeGenerator -> Vala.CodeVisitor

So GAsyncModule is a CodeVisitor that overrides visit_method (), visit_creation_method (), visit_yield_statement () and visit_return_statement (). It overrides a few methods from elsewhere, such as generate_method_declaration (). Vala uses the visitor pattern internally, but I haven't figured out where a single switch for --enable-experimental-gtask-async-module would work.

It's hard to tell how widespread the co-routines use case is. There is a note on Wikipedia stating "Vala implements native support for coroutines. They are designed to be used with a Gtk Main Loop, but can be used alone if care is taken to ensure that the end callback will never have to be called before doing, at least, one yield." See https://en.wikipedia.org/wiki/Coroutine#Implementations_for_Vala

As an extra data point, Geary makes heavy use of async calls and it builds and seems to run fine when built with valac master with the patches above applied.

Attachment 324592 [details] pushed as 14ca2e0 - codegen: Use GTask instead of GSimpleAsyncResult if 2.36 target is selected

@carlosg: Hmm, any ideas on how to handle "g_task_get_completed" not being available in glib 2.36, other than bumping the requirement for this feature to glib 2.44.

*** Bug 764781 has been marked as a duplicate of this bug. ***