[BUG] when returning a primitive type from a C callback for a task task::propagate expects a GValue

[Ytsejam76]

given a C code callback returning a primitive typed result (such as a boolean) with, e.g., g_task_return_boolean the Gtk-rs bindings fail to propagate the result, since the generic ffi::g_task_propagate_value() (or ffi::g_task_propagate_pointer`, depending on the version) is called under the hood by Task::propagate() and the call fails at the:

  g_return_val_if_fail (task->result_destroy == value_free, FALSE);

The workaround is to use g_task_return_value() even for primitive types:

  void my_task_callback(....) {
            GValue ret_value = { 0 };

/* My callback logic */

            g_value_init(&ret_value, G_TYPE_BOOLEAN);
            g_value_set_boolean(&ret_value, TRUE);
            g_task_return_value(task, &ret_value);
            g_value_unset(&ret_value);
}

[sdroege]

Can you provide a runnable and minimal testcase that demonstrates the problem?

[Ytsejam76]

Do you really need it?
If you check task.rs you see that propagate() either calls task.rs:287: ffi::g_task_propagate_value(), or task.rs:302: ffi::g_task_propagate_pointer()

Both expect the gtask.c: 2305: task->result_destroy not to be NULL, which it is, if a primitive value is returned, as is the case of gtask.c:2007: g_task_return_boolean()

[sdroege]

Not "necessary" but as you already have the code it would allow me to spend less time on writing a testcase (and maybe moving that into an actual test) and more time on actually solving it.

[Ytsejam76]

There we go:

https://github.com/Ytsejam76/gtk_rs_minimal_example.git

running cargo test should be sufficient.

[Ytsejam76]

This is an attempt at a solution for the boolean case only. If deemed fine I could follow up along the same lines for integers.
Ytsejam76:gtk-rs-core:wip/task. I use any::TypeId to identify if we are dealing with a bool because I couldn't do it some other way (e.g. with static_type() which doesn't seem to be implemented for glib::Value if I am not mistaken). Also I only tested the not(feature="v2_64") case.

[carlosmn]

If I understand this right, fundamental issue is that glib expects you to know what kind of result was set to be returned, down to the type, and those can be boolean, int, value or error (though the error gets handled more like a Result. And if you call g_task_return_boolean, then it stores that as a boolean rather than a value, and g_task_propagate_value wants you to have set the return as a value. The docs for g_task_propagate_value starts with Gets the result of task as a GValue which sure sounds to me like it's saying it will convert the returned value into a GValue.

It seems like hard-coding, as pointed to by the previous comment, has the same kinds of issues in the other direction, as is pointed out by the tests in that branch. If I call let res: Result<bool, _> = task.propagate() then we have a different flavour of the same issue, as that would change so the task cannot generically return a value for bool (or int). Given you're using the same version of gtk-rs-core on both that would be fine (but it's fine already, though it would be slightly more efficient), but if something is generically returning a GValue from C code even though it's a boolean then you would be forced to change the types so you get the Value and then you have to convert it yourself instead of letting Task::propagate do it for you.

AFAICT there is no way to figure out what kind of GTask has inside, so an external caller (and even internally?) isn't able to be generic/dynamic on the return value. Presumably the answer from glib to this is that you can, and you should be using a GValue if that's what you want to do (which is indeed what gtk-rs-core is doing).

Given the specialization of this for calls that would explicitly use g_task_return_{boolean,int}, and that there are only two of these, maybe it's better to explicitly expose Task::propagate_{bool,isize}() (or use the same names from glib) that call the appropriate value? You would want to document why you probably don't want to use them, and unfortunately you still cannot know if you're calling them in error as you're just asking to use the untagged union.

[carlosmn]

Depending on the caller of the task, would we also need to have the explicit Task::return_{bool,isize}? If you are passing in a task to some C library, it might expect to be given a boolean or int that way rather than via a GValue, no?

[Ytsejam76]

Yeah, I agree on all you say and, generally speaking, I tend to like the motto explicit is better than implicit, so I am not against having specialized methods for the sake of clarity and also for uniformity with glib.
However, that was also the reason why I preferred (or I disliked less) to have "the same kind of issues in the other direction".
That said I am perfectly fine using your Task::propagate_{bool,isize}() (although, again for the sake of consistency with glib naming, I'd prefer int rather than isize to name the latter).

[carlosmn]

I've marked #1762 ready. If we're messing around with this then I think we should keep it to what glib is dong and call out the danger of mixing the setter and getters for the result.

A better option is to just not do any of this and use futures but obviously that doesn't work if you need to interop.

[sdroege]

Fixed by #1762