GNOME Bugzilla – Bug 794153
GstPromise: improve documentation to describe design choices
Last modified: 2018-03-12 11:07:49 UTC
I'm familiar with Promises (JavaScript async), Futures (Python concurrent and asyncio) and GTask. An unordered list of gripes with GstPromise: - It has no concept of failure. - Nullable GstStructure seems to be a bad fit for a reply. - Forced into the schema of a name + string-GValue-map. - Overkill for returning a simple value. - No obvious way of signaling an error. - No means of signaling the promiser from the promisee. - No cancellation of operations other than the promiser polling the promise for result changes. - The purpose of INTERRUPTED and EXPIRED is not clear. - INTERRUPTED results in a reply succeeding but the structure is freed. - EXPIRED results in criticals on any manipulation, but nothing using GstPromise sets or checks for expiration. - Was INTERRUPTED or EXPIRED meant for cancellation? - The promisee creating the Promise and passing it to the promiser is weird. - I am used to seeing the promiser returning a promise. Some suggestions: - Split the promise into two objects created together, one for the promiser (GstPromise?) and one for the promisee (GstFuture?). - Both sides have a change notification callback. - Setting a callback on a non-PENDING side immediately fires the callback. - The promisee side can be cancelled, then both sides become INTERRUPTED/CANCELLED. - If any side is freed while PENDING, the other side is EXPIRED. - EXPIRED cannot be set any other way. - EXPIRED on the promisee side results in a noisy warning. Promisers shouldn't forget about their promises! - Trying to reply to an EXPIRED or INTERRUPTED promise succeeds but does nothing. - Interruption is a signal to the promiser to abort, expiration is not necessarily one. - Split REPLIED into RESOLVED/FULFILLED/SUCCESS/OK and REJECTED/FAILURE/ERROR. - Success and failure come with a GValue. - Have the promiser create the objects and return the promise. - Have some additional constructors to shortcut creating a promise that's already resolved or already rejected. - Have some helper functions to shortcut fulfilling with common types or rejecting with a GError.
(In reply to Jan Alexander Steffens (heftig) from comment #0) > I'm familiar with Promises (JavaScript async), Futures (Python concurrent > and asyncio) and GTask. An unordered list of gripes with GstPromise: > > - It has no concept of failure. Failure us a reply with a different valu. > - Nullable GstStructure seems to be a bad fit for a reply. Means the value of the reply doesn't matter. Only the act of replying has any significance. > - Forced into the schema of a name + string-GValue-map. Nothing forces you to have strings for values. > - Overkill for returning a simple value. That it may be. > - No obvious way of signaling an error. Return a different value. > - No means of signaling the promiser from the promisee. What do you mean with this? The consumer can signal INTERRUPTED for saying it doesn't want the producer (promiser) to produce the result anymore. > - No cancellation of operations other than the promiser polling the > promise for result changes. INTERRUPTED? > - The purpose of INTERRUPTED and EXPIRED is not clear. INTERRUPTED - see above. EXPIRED - no one could answer the promise in cases where there's a message loop in place like GstBus. > - INTERRUPTED results in a reply succeeding but the structure is freed. Correct. No one cares about an interrupte promise. > - EXPIRED results in criticals on any manipulation, but nothing using > GstPromise sets or checks for expiration. > - Was INTERRUPTED or EXPIRED meant for cancellation? > - The promisee creating the Promise and passing it to the promiser is weird. > - I am used to seeing the promiser returning a promise. The producer creating promise API has some problems with thread safety when the producer attempts to create the promise and a change function is installed by the consumer. > Some suggestions: > > - Split the promise into two objects created together, one for the promiser > (GstPromise?) and one for the promisee (GstFuture?). > - Both sides have a change notification callback. > - Setting a callback on a non-PENDING side immediately fires the callback. This also has thread safety ramifications with holding locks and such and introduces two places for the callback to be called which we all know from pad probe API and idle pads is a source of confusion :) > - The promisee side can be cancelled, then both sides become > INTERRUPTED/CANCELLED. > - If any side is freed while PENDING, the other side is EXPIRED. Relying on refs held is fraught with danger especially when the consumer is already holding one or more refs. > - EXPIRED cannot be set any other way. > - EXPIRED on the promisee side results in a noisy warning. Promisers > shouldn't forget about their promises! The producer doesn't forget. It is notified no one will answer. > - Trying to reply to an EXPIRED or INTERRUPTED promise succeeds but does > nothing. This is correct for interrupted. Expired means no one is going to answer so replying will never happen. > - Interruption is a signal to the promiser to abort, expiration is not > necessarily one. Already the case for interrupt. Expiration is the other way around and signals that a value will never be produced to avoid pesky deadlocks waiting for a promise. > - Split REPLIED into RESOLVED/FULFILLED/SUCCESS/OK and > REJECTED/FAILURE/ERROR. > - Success and failure come with a GValue. This API can be built on top if you want something like this. > - Have the promiser create the objects and return the promise. As above, this has a problem with thread safety and deadlocks. > - Have some additional constructors to shortcut creating a promise that's > already resolved or already rejected. > - Have some helper functions to shortcut fulfilling with common types or > rejecting with a GError.
> Nothing forces you to have strings for values. I mean the structure has a name itself as well as a map of string keys to GValues. > Already the case for interrupt. Expiration is the other way around and signals that a value will never be produced to avoid pesky deadlocks waiting for a promise. So EXPIRED was meant for the producer to notify a consumer that there won't be a reply? Why not use a reply for this as well? > The consumer can signal INTERRUPTED for saying it doesn't want the producer (promiser) to produce the result anymore. The producer needs to poll for this signal by checking the promise result. There's only one callback on the promise for use by the consumer.
(In reply to Jan Alexander Steffens (heftig) from comment #2) > > Nothing forces you to have strings for values. > > I mean the structure has a name itself as well as a map of string keys to > GValues. Right. These names can mean whatever you want them to mean if anything. > > Already the case for interrupt. Expiration is the other way around and signals that a value will never be produced to avoid pesky deadlocks waiting for a promise. > > So EXPIRED was meant for the producer to notify a consumer that there won't > be a reply? Why not use a reply for this as well? Not the producer but something else that does not know anything about expected values exchanged. A reply could be constructed but then that specific reply could never be used for a real value. > > The consumer can signal INTERRUPTED for saying it doesn't want the producer (promiser) to produce the result anymore. > > The producer needs to poll for this signal by checking the promise result. > There's only one callback on the promise for use by the consumer. Yes. In some way that is specific to the value to be produced.
Mathew, from heftig's comments, I think it's clear that the documentation needs improving. Can we have some explanation of those various types of results in the general blurb about the promise? Basically put in there was you gave in your first reply?
Created attachment 369420 [details] [review] promise: more explicit documentation about what/when occurs with GstPromise using API. Like this? @Olivier Do you have any specific gripes with the docs?
Review of attachment 369420 [details] [review]: Maybe instead of having one long sentence if you split it a bit more for readability. Also mention what happens with the value/structure in each state. Maybe also for the doc of each function gst_promise_expire & _interrupt, mention who is meant to call them and what happens to the reply structure (that they may return with no value).
Pushed an updated change. commit 7a008ea481a95e80d9f09ffeb8ed4b7a0f0c8667 Author: Matthew Waters <matthew@centricular.com> Date: Thu Mar 8 13:30:30 2018 +1100 promise: be more explicit in docs about who/when to use reply/interrupt/expire https://bugzilla.gnome.org/show_bug.cgi?id=794153