GNOME Bugzilla – Bug 732960
gtask could help make "pending" easier
Last modified: 2015-06-25 09:34:29 UTC
It's currently nearly impossible to properly implement "one at a time" restrictions on async operations using GTask. For the sake of argument, everything is single-threaded here, under a single main context. The duration of an operation in progress goes from the time of the user's _async() call to the time that the user gets their callback entered. Note that the user may not call _finish() from their callback. Note also that it is valid to issue another async operation from the callback. As a class implementing an async method I have a problem: I don't know when the user's callback will be called, so I have no way to update my internal state about this. I do know when I request the user's callback to be scheduled (via g_task_return_*) but there could be other things scheduled on the mainloop that run before the actual dispatch, which is where the trouble lies. A proper solution would see new operations rejected during this interim period. There are a couple of approaches we could take here. One of the more obvious is to try and get GTask to track the pending status of operations internally. We could have some new GTask constructor API that takes a "uniqueness key" and prevents creation of two GTask objects on the same source object with the same uniqueness key For example: my_stream_read_async() { task = g_task_new_unique (stream, "read", cancellable, ...); which would prevent two "read" operations existing on 'stream'. In the event that you try to create a second one, it could automatically send a G_IO_ERROR_PENDING to the caller. We could also have a g_task_check_pending (stream, "read") to check if any read operation was pending. We could use this during sync/blocking operations to make sure they don't conflict with any pending async operations. We could even do it like g_task_ensure_not_pending (stream, "read", &error) to get the #GError filled in for the user. On the other hand, this could very well result in the "pending" error arriving _after_ the successful result in the first place, meaning by the time you get the G_IO_ERROR_PENDING, it would actually be a lie (since the operation is no longer pending). This sort of goes towards the idea that G_IO_ERROR_PENDING may have been a bad idea to begin with: it's a programmer error to issue two conflicting async operations, so maybe this should have always been a critical. We could save the state inside of GTask by just giving a pointer to some boolean within the object that the task is operating on... that would also allow easy use of criticals to ensure that the operation is not already pending on both async and sync calls. In any case, the magic piece that we really cannot do from outside of GTask is that whatever flag we use for pending, we must clear it from inside of GTask just before calling the user's callback.
I only scanned this bug but FWIW what I currently do is: https://git.gnome.org/browse/libgssh/tree/src/gssh-channel-input-stream.c#n121 Then: https://git.gnome.org/browse/libgssh/tree/src/gssh-channel-input-stream.c#n191 https://git.gnome.org/browse/libgssh/tree/src/gssh-channel-input-stream.c#n214 It's ugly but I think it works? Basically using a struct pointer where you use userdata.
> I do know when I request the user's callback to be scheduled (via > g_task_return_*) but there could be other things scheduled on the mainloop that > run before the actual dispatch, which is where the trouble lies. A proper > solution would see new operations rejected during this interim period. I don't think it's legitimate for an operation to schedule portions of itself to run in between the g_task_return* call and the invocation of the callback. But I agree that the operation should still be considered pending until the callback is invoked. (If you consider G_IO_ERROR_PENDING to be a programmer error in disguise, then the operation has to be considered pending until the callback runs, since the caller has no other way of knowing when the operation has completed. If you consider it to be a legitimate runtime error, then maybe there's less argument for this, but...) > There are a couple of approaches we could take here. > > One of the more obvious is to try and get GTask to... Note that if the object has synchronous operations as well as async ones, then you want to have a single "pending" flag that applies to both kinds of ops. (ie, you can't do an async read while doing a sync one in another thread). So that argues against implementing it entirely in GTask. > In any case, the magic piece that we really cannot do from outside of GTask is > that whatever flag we use for pending, we must clear it from inside of GTask > just before calling the user's callback. Something like g_task_set_pre_callback_callback() would allow for that in a generic way. Although, this wouldn't help in GInputStream's case, because there you have GInputStream keeping track of the "pending" flag, but the subclass is the one creating the GTask. (Though it's possible that there's no good solution that would work within the constraints of GInputStream's existing API, so we may just have to ignore them.)
(In reply to comment #0) > On the other hand, this could very well result in the "pending" error arriving > _after_ the successful result in the first place, meaning by the time you get > the G_IO_ERROR_PENDING, it would actually be a lie (since the operation is no > longer pending). This sort of goes towards the idea that G_IO_ERROR_PENDING > may have been a bad idea to begin with: it's a programmer error to issue two > conflicting async operations, so maybe this should have always been a critical. In a recent project, I took the approach of not implementing G_IO_ERROR_PENDING in custom async operations, and instead yielding on completion of any pending async operations (with a given uniqueness key) when starting another instance of such an operation. This required a few additions to GTask (in a custom version of GLib, ewww): How about a GTask::returned signal which is emitted in the task’s context, immediately after the user’s callback is invoked? Signal emission is synchronous, so it won’t run into problems with other things being scheduled in the main context between the user’s callback and the internal callback of the class being implemented. I used this (in a custom version of GLib, ewwww) in a recent project – not for eliminating G_IO_ERROR_PENDING, but for yielding on cancelling ongoing GTasks[1] – and it worked pretty well. API additions required: • GTask::returned — signal, no parameters • gboolean g_task_has_returned(GTask *task) — whether that signal has been emitted For my use case I could then implement: • void my_g_task_cancel_multiple_async (GTask **tasks, guint n_tasks, GCancellable *cancellable, GAsyncReadyCallback callback, gpointer user_data) which would cancel all the @tasks and yield on them completing. Although revisiting this, I’m not sure I got the locking around g_task_has_returned() correct. We might need something like g_task_connect_has_returned(), similar to g_cancellable_connect(). So for G_IO_ERROR_PENDING, when the user initiates an async operation on a custom class implementation which would currently return G_IO_ERROR_PENDING, the class could either: • Connect to GTask::returned for the existing GTask, and yield on that, then schedule the next GTask afterwards, eliminating G_IO_ERROR_PENDING; or • Check g_task_has_returned() and return G_IO_ERROR_PENDING if that’s false. There’s a race here though. I prefer the first option; the need for G_IO_ERROR_PENDING has never been clear to me, when contrasted with simply yielding the second async call on completion of the first. [1]: A class being implemented had multiple user-initiated GTasks which could be ongoing at once, but calling my_class_close_async() needed to cancel them all and yield on them all completing to guarantee all resources had been freed.
(In reply to comment #3) > In a recent project, I took the approach of not implementing G_IO_ERROR_PENDING > in custom async operations, and instead yielding on completion of any pending > async operations (with a given uniqueness key) when starting another instance > of such an operation. > > This required a few additions to GTask (in a custom version of GLib, ewww): Er, ignore that earlier draft of the comment. :-\
This was fixed with the GTask:completed property in bug #743636. *** This bug has been marked as a duplicate of bug 743636 ***