Created attachment 362870 [details] [review]
gst: add a promise object

Review of attachment 362870 [details] [review]:

The API seems relatively sane. The only thing I'm not so happy about is the change function being called synchronously with the mutex held, is that really required? I'd feel better having it called from another thread, doing something like gst_element_call_async(), it could even be using the same thread pool.

It may also be nice to have a vaarg version of gst_promise_reply_...() which calls gst_structure_new_valist() internally to save a line.

::: gst/gstpromise.c
@@ +40,3 @@
+ *
+ * A #GstPromise can be created with gst_promise_new(), replied to
+ * with gst_promise_reply(), interrupted with gst_promise_reply() and

I guess you mean "interrupted with gst_promise_interrupt()" ?

@@ +157,3 @@
+    GST_LOG ("%p replied", promise);
+
+    promise->promise = s;

Maybe you want to also do gst_structure_set_parent_refcount() ? Or does it make no sense at all?

::: gst/gstpromise.h
@@ +69,3 @@
+  GstStructure         *promise;
+
+  /*< private >*/

Shouldn't this be in a hidden struct part of the struct? Maybe using the GstXXXImpl trick used by GstBuffer. I would hide all of the implementation details, including the structure and the result and only allow access to the accessor function.

Is the padding in the GstPromise struct needed? It's a mini object, so can't be subclassed anyway, can it?

(In reply to Olivier Crête from comment #2)
> Review of attachment 362870 [details] [review] [review]:
> 
> The API seems relatively sane. The only thing I'm not so happy about is the
> change function being called synchronously with the mutex held, is that
> really required? I'd feel better having it called from another thread, doing
> something like gst_element_call_async(), it could even be using the same
> thread pool.

Yea, see I'm very hesitant to add another thread into the mix.

Removing the change_func callback outside the mutex also introduces a race whereby gst_promise_set_change_callback() can race with calling the change_func and access freed data.

i.e. before the change_func is called and after the state has changed, it's possible to 1. change the function but not the data, 2. notify (and therefore free) the data that will be used in the state change.  Both of these are pretty severe memory issues :).  The only way I can see to mitigate these is some extra checking to e.g. seal the state change function once set and passed for reply and not allow that scenario to occur.

> It may also be nice to have a vaarg version of gst_promise_reply_...() which
> calls gst_structure_new_valist() internally to save a line.

Can be done.

> ::: gst/gstpromise.c
> @@ +40,3 @@
> + *
> + * A #GstPromise can be created with gst_promise_new(), replied to
> + * with gst_promise_reply(), interrupted with gst_promise_reply() and
> 
> I guess you mean "interrupted with gst_promise_interrupt()" ?

Yea

> @@ +157,3 @@
> +    GST_LOG ("%p replied", promise);
> +
> +    promise->promise = s;
> 
> Maybe you want to also do gst_structure_set_parent_refcount() ? Or does it
> make no sense at all?

It sort of makes sense to do that however the idea is that once the structure is set on the promise, it is immutable which is not quite what gst_structure_set_parent_refcount() ensures.

> ::: gst/gstpromise.h
> @@ +69,3 @@
> +  GstStructure         *promise;
> +
> +  /*< private >*/
> 
> Shouldn't this be in a hidden struct part of the struct? Maybe using the
> GstXXXImpl trick used by GstBuffer. I would hide all of the implementation
> details, including the structure and the result and only allow access to the
> accessor function.

Yea, I'll look at that.

(In reply to Tim-Philipp Müller from comment #3)
> Is the padding in the GstPromise struct needed? It's a mini object, so can't
> be subclassed anyway, can it?

GstMiniObject's can be subclassed and embedded in other structs.  See GstMemory and all the memory subclasses :).  In this case I don't think I envision anybody attempting to subclass GstPromise.  Adding fields to the GstStructure is enough for user-side extensibility and with the GstXXXImpl we get implementation-side extensibility as well.

(In reply to Matthew Waters (ystreet00) from comment #4)
> (In reply to Olivier Crête from comment #2)
> > Review of attachment 362870 [details] [review] [review] [review]:
> > 
> > The API seems relatively sane. The only thing I'm not so happy about is the
> > change function being called synchronously with the mutex held, is that
> > really required? I'd feel better having it called from another thread, doing
> > something like gst_element_call_async(), it could even be using the same
> > thread pool.
> 
> Yea, see I'm very hesitant to add another thread into the mix.
> 
> Removing the change_func callback outside the mutex also introduces a race
> whereby gst_promise_set_change_callback() can race with calling the
> change_func and access freed data.
> 
> i.e. before the change_func is called and after the state has changed, it's
> possible to 1. change the function but not the data, 2. notify (and
> therefore free) the data that will be used in the state change.  Both of
> these are pretty severe memory issues :).  The only way I can see to
> mitigate these is some extra checking to e.g. seal the state change function
> once set and passed for reply and not allow that scenario to occur.

I'd be favorable to making it possible to set the callback function+data multiple times and then stealing it when it's triggered.. And immediately sending it to a thread to be triggered if it's set after the promise has been acted upon, to prevent the race where it's set after the promise has been replied/interrrupted/etc.


> > @@ +157,3 @@
> > +    GST_LOG ("%p replied", promise);
> > +
> > +    promise->promise = s;
> > 
> > Maybe you want to also do gst_structure_set_parent_refcount() ? Or does it
> > make no sense at all?
> 
> It sort of makes sense to do that however the idea is that once the
> structure is set on the promise, it is immutable which is not quite what
> gst_structure_set_parent_refcount() ensures.

I wonder if we can create a "const int refcount = 2;" and then set that as the refcoun to make it immutable.

(In reply to Olivier Crête from comment #5)
> (In reply to Matthew Waters (ystreet00) from comment #4)
> > (In reply to Olivier Crête from comment #2)
> > > Review of attachment 362870 [details] [review] [review] [review] [review]:
> > > 
> > > The API seems relatively sane. The only thing I'm not so happy about is the
> > > change function being called synchronously with the mutex held, is that
> > > really required? I'd feel better having it called from another thread, doing
> > > something like gst_element_call_async(), it could even be using the same
> > > thread pool.
> > 
> > Yea, see I'm very hesitant to add another thread into the mix.
> > 
> > Removing the change_func callback outside the mutex also introduces a race
> > whereby gst_promise_set_change_callback() can race with calling the
> > change_func and access freed data.
> > 
> > i.e. before the change_func is called and after the state has changed, it's
> > possible to 1. change the function but not the data, 2. notify (and
> > therefore free) the data that will be used in the state change.  Both of
> > these are pretty severe memory issues :).  The only way I can see to
> > mitigate these is some extra checking to e.g. seal the state change function
> > once set and passed for reply and not allow that scenario to occur.
> 
> I'd be favorable to making it possible to set the callback function+data
> multiple times and then stealing it when it's triggered.. And immediately
> sending it to a thread to be triggered if it's set after the promise has
> been acted upon, to prevent the race where it's set after the promise has
> been replied/interrrupted/etc.

Originally I had the webrtcbin implementation so that the callee had to create the promise and thus the setting of the change callback could race with the reply.  That's since been changed so that the caller creates the promise and sets the change callback before passing for a reply.

I don't really want to support the first case as setting the change callback after a reply/interrupt/expire doesn't seem like a valid use case?

Stealing the func/data is a good idea though.

> > > @@ +157,3 @@
> > > +    GST_LOG ("%p replied", promise);
> > > +
> > > +    promise->promise = s;
> > > 
> > > Maybe you want to also do gst_structure_set_parent_refcount() ? Or does it
> > > make no sense at all?
> > 
> > It sort of makes sense to do that however the idea is that once the
> > structure is set on the promise, it is immutable which is not quite what
> > gst_structure_set_parent_refcount() ensures.
> 
> I wonder if we can create a "const int refcount = 2;" and then set that as
> the refcoun to make it immutable.

That sounds crazy but actually looks like it might work :)

(In reply to Matthew Waters (ystreet00) from comment #6)
> Originally I had the webrtcbin implementation so that the callee had to
> create the promise and thus the setting of the change callback could race
> with the reply.  That's since been changed so that the caller creates the
> promise and sets the change callback before passing for a reply.
> 
> I don't really want to support the first case as setting the change callback
> after a reply/interrupt/expire doesn't seem like a valid use case?
> 
> Stealing the func/data is a good idea though.

In that case you may just want to let the user set the callback on the gst_promise_new() and avoid the whole issue?

Created attachment 364028 [details] [review]
gst: add a promise object

Review of attachment 364028 [details] [review]:

Only a short review, generally looks fine to me though

::: gst/gstpromise.c
@@ +334,3 @@
+ * @user_data: (closure): argument to call @func with
+ * @notify: notification function that @user_data is no longer needed
+ *

Maybe write somewhere what the use-case of this is? I assume it's for users of promises to implement asynchronous handling of promises (instead of blocking wait()), and e.g. chain promises to each other?

::: gst/gstpromise.h
@@ +36,3 @@
+ * @GST_PROMISE_RESULT_INTERRUPTED: Interrupted by the consumer
+ * @GST_PROMISE_RESULT_REPLIED: A producer marked a reply
+ * @GST_PROMISE_RESULT_EXPIRED: The promise expired (the carrying object lost all refs)

What's the carrying object? How do interrupted and expired differ, interrupted is caused by the consumer of the promise and expired by the producer?

@@ +45,3 @@
+  GST_PROMISE_RESULT_INTERRUPTED,
+  GST_PROMISE_RESULT_REPLIED,
+  GST_PROMISE_RESULT_EXPIRED,

It might be good to also add an ERROR case here, and allow to get/pass a GError in that case. Everything can fail

(In reply to Sebastian Dröge (slomo) from comment #9)
> ::: gst/gstpromise.c
> @@ +334,3 @@
> + * @user_data: (closure): argument to call @func with
> + * @notify: notification function that @user_data is no longer needed
> + *
> 
> Maybe write somewhere what the use-case of this is? I assume it's for users
> of promises to implement asynchronous handling of promises (instead of
> blocking wait()), and e.g. chain promises to each other?

Ok.

> ::: gst/gstpromise.h
> @@ +36,3 @@
> + * @GST_PROMISE_RESULT_INTERRUPTED: Interrupted by the consumer
> + * @GST_PROMISE_RESULT_REPLIED: A producer marked a reply
> + * @GST_PROMISE_RESULT_EXPIRED: The promise expired (the carrying object
> lost all refs)
> 
> What's the carrying object? How do interrupted and expired differ,
> interrupted is caused by the consumer of the promise and expired by the
> producer?

e.g. sending a GstMessage, or on some other parent (or carrying) object where task-like semantics are enforced e.g. GstBus.  Expired means that that object has been destroyed and so the promise will never be fulfilled.  Interrupted means that the waiter doesn't want/need the value anymore.

> @@ +45,3 @@
> +  GST_PROMISE_RESULT_INTERRUPTED,
> +  GST_PROMISE_RESULT_REPLIED,
> +  GST_PROMISE_RESULT_EXPIRED,
> 
> It might be good to also add an ERROR case here, and allow to get/pass a
> GError in that case. Everything can fail

I don't believe any of the promise functionality has the potential to fail without a warn/critical.  Did you have a specific case in mind where this is not the case?

(In reply to Matthew Waters (ystreet00) from comment #10)
> > ::: gst/gstpromise.h
> > @@ +36,3 @@
> > + * @GST_PROMISE_RESULT_INTERRUPTED: Interrupted by the consumer
> > + * @GST_PROMISE_RESULT_REPLIED: A producer marked a reply
> > + * @GST_PROMISE_RESULT_EXPIRED: The promise expired (the carrying object
> > lost all refs)
> > 
> > What's the carrying object? How do interrupted and expired differ,
> > interrupted is caused by the consumer of the promise and expired by the
> > producer?
> 
> e.g. sending a GstMessage, or on some other parent (or carrying) object
> where task-like semantics are enforced e.g. GstBus.  Expired means that that
> object has been destroyed and so the promise will never be fulfilled. 
> Interrupted means that the waiter doesn't want/need the value anymore.

Maybe a comment about that to the docs :)


> > @@ +45,3 @@
> > +  GST_PROMISE_RESULT_INTERRUPTED,
> > +  GST_PROMISE_RESULT_REPLIED,
> > +  GST_PROMISE_RESULT_EXPIRED,
> > 
> > It might be good to also add an ERROR case here, and allow to get/pass a
> > GError in that case. Everything can fail
> 
> I don't believe any of the promise functionality has the potential to fail
> without a warn/critical.  Did you have a specific case in mind where this is
> not the case?

Not the promise infrastructure, but an actual promise itself. It's similar to expired, but with a real error. Constructed example: Say you promise to produce some information that requires you to talk to the network, but then DNS resolution fails and you can't provide that information anymore. What would happen then? Ideally you'd like to tell the consumer that the promise is never going to be fulfilled anymore and that there was this specific error.

Note also that the promises in GIO are all fallible (GAsyncResult / GTask).

(In reply to Sebastian Dröge (slomo) from comment #11)
> (In reply to Matthew Waters (ystreet00) from comment #10)
> > > @@ +45,3 @@
> > > +  GST_PROMISE_RESULT_INTERRUPTED,
> > > +  GST_PROMISE_RESULT_REPLIED,
> > > +  GST_PROMISE_RESULT_EXPIRED,
> > > 
> > > It might be good to also add an ERROR case here, and allow to get/pass a
> > > GError in that case. Everything can fail
> > 
> > I don't believe any of the promise functionality has the potential to fail
> > without a warn/critical.  Did you have a specific case in mind where this is
> > not the case?
> 
> Not the promise infrastructure, but an actual promise itself. It's similar
> to expired, but with a real error. Constructed example: Say you promise to
> produce some information that requires you to talk to the network, but then
> DNS resolution fails and you can't provide that information anymore. What
> would happen then? Ideally you'd like to tell the consumer that the promise
> is never going to be fulfilled anymore and that there was this specific
> error.
> 
> Note also that the promises in GIO are all fallible (GAsyncResult / GTask).

An error is still a reply so this boils down to nothing changing.

Ok :)

Created attachment 364171 [details] [review]
gst: add a promise object

Incorporated latest review comments.

Comment on attachment 364171 [details] [review]
gst: add a promise object

Good to go now IMHO

commit 86abf49c23c88bda209fec6fd964e482b36377d1
Author: Matthew Waters <matthew@centricular.com>
Date:   Mon Apr 3 22:20:51 2017 +1000

    gst: add a promise object
    
    An object that can be waited on and asked for asynchronous values.
    In much the same way as promise/futures in js/java/etc
    
    A callback can be installed for when the promise changes state.
    
    Original idea by
    Jan Schmidt <jan@centricular.com>
    
    With contributions from
    Nirbheek Chauhan <nirbheek@centricular.com>
    Mathieu Duponchelle <mathieu@centricular.com>
    
    https://bugzilla.gnome.org/show_bug.cgi?id=789843