C++ bindings proposal: replace the ErrorHandler interface with a mojo::Callback?

[Yuzhu Shen]

Hi,

Currently we use ErrorHandler to report connection error for InterfacePtr<> and Binding<>. For example,

class Foo : ErrorHandler {

  void SomeMethod() {

    ...

    bar_binding_.set_error_handler(this);

    ...

  }

  void OnConnectionError() override { ... }

  Binding<Bar> bar_binding_;

  QuxPtr qux_ptr_;

};

I've encountered a few problems with this design:

(1) When I want to handle errors differently for multiple interface pointers or bindings, I have to define separate ErrorHandler subclasses. For example,there is no way for Foo to provide different error handlers for |bar_binding_| and |qux_ptr_|.

(2) The name ErrorHandler and OnConnectionError seem a little vague. Without looking at the method body, I usually cannot tell which pointer/binding the handler is used for.

I feel that it is more flexible if we replace the ErrorHandler interface with a mojo::Callback.

That way, Foo can provide error handling for both |bar_binding_| and |qux_ptr_| directly.

We can use lambda and base::Bind (in chromium):

qux_ptr_.set_connection_error_handler(

    [this]() { qux_ptr_ = nullptr; });

We can choose more meaningful names as well.

bar_binding_.set_connection_error_handler(

    [this] () { OnBarBindingConnectionError(); });

What do you think? Thanks!

[Benjamin Lerman]

+1. Python binding already takes the equivalent here.

[Colin Blundell]

+1. I've struggled with both issues that Yuzhu describes.

[Ken Rockot]

This would impact a large amount of code. It might be worth doing but I think you'd have to support both ErrorHandler and callback error handlers for a transition period.

Why not just create a callback-wrapping ErrorHandler for when you need one?

class CallbackErrorHandler : public ErrorHandler {

 public:

  CallbackErrorHandler(const mojo::Closure& callback) : callback_(callback) {}

  ~CallbackErrorHandler() override {}

  void OnConnectionError() override { callback_.Run(); }

 private:

  mojo::Closure callback_;

};

You of course still have to deal with lifetime of the ErrorHandler, but it's less of a burden than defining a new class for every different behavior.

[Yuzhu Shen]

Thanks for all the feedbacks!

On Wed, Jun 3, 2015 at 10:09 AM, Ken Rockot <roc...@chromium.org> wrote:

This would impact a large amount of code. It might be worth doing but I think you'd have to support both ErrorHandler and callback error handlers for a transition period.

Yeah. If we decide to do it, we will have to support both ways while we deprecate the old way.

It would be trivial to implement the old way using the new way. Something like: (Haven't tried compiling it. :) )

class InterfacePtr {

...

  // The new way.

  void set_connection_error_handler(

      const Callback<void()>& callback) { ... }

  // The old way:

  void set_error_handler(ErrorHandler* error_handler) {

    set_connection_error_handler(

        [error_handler]() { error_handler->OnConnectionError(); });

  }

};

Why not just create a callback-wrapping ErrorHandler for when you need one?

class CallbackErrorHandler : public ErrorHandler {

 public:

  CallbackErrorHandler(const mojo::Closure& callback) : callback_(callback) {}

  ~CallbackErrorHandler() override {}

  void OnConnectionError() override { callback_.Run(); }

 private:

  mojo::Closure callback_;

};

You of course still have to deal with lifetime of the ErrorHandler, but it's less of a burden than defining a new class for every different behavior.

Right. The fact that we still need to deal with its lifetime usually means that we have to make this CallbackErrorHandler a class member.

--

Best regards,
Yuzhu Shen.

[Yuzhu Shen]

Hi,

If no objections, I am going to make this happen: https://github.com/domokit/mojo/issues/203

I will add a method to InterfacePtr/Binding to accept a callback as handler for connection error, and gradually eliminate the current usage of ErrorHandler.

Please speak up if you have any concerns or suggestions. Thanks!

[Yuzhu Shen]

FYI: I have landed a CL to support using mojo::Callback<void()> (i.e., mojo::Closure) as connection error handler:

https://codereview.chromium.org/1174073002/

A new method has been added to InterfacePtr, Binding and StrongBinding:

void set_connection_error_handler(const Closure& error_handler);

I will gradually convert the callsites of the old set_error_handler(ErrorHandler*) method, and eventually remove that method.

It would be appreciated if you help to do some of the conversions. :)

[Ken Rockot]

Yay! Thanks for doing this.

[Colin Blundell]

This is really nice. Thanks, Yuzhu!

There's another related benefit here, which is in the pattern of a "FooFactory" or "FooContext" object that takes in InterfaceRequests for Foo and creates FooImpls, passing itself to them to provide some kind of global information. The factory object should naturally own the Impls since they need to go away when it goes away. However, it also needs to know if they experience a connection error so it can delete them. This relationship is much more natural to express with callbacks than it was in the ErrorHandler world; for example, see https://codereview.chromium.org/1185563003/.

Best,

Colin

[Yuzhu Shen]

On Fri, Jun 12, 2015 at 2:29 AM, Colin Blundell <blun...@chromium.org> wrote:

This is really nice. Thanks, Yuzhu!

There's another related benefit here, which is in the pattern of a "FooFactory" or "FooContext" object that takes in InterfaceRequests for Foo and creates FooImpls, passing itself to them to provide some kind of global information. The factory object should naturally own the Impls since they need to go away when it goes away. However, it also needs to know if they experience a connection error so it can delete them. This relationship is much more natural to express with callbacks than it was in the ErrorHandler world; for example, see https://codereview.chromium.org/1185563003/.

Agreed. It is a pretty common pattern. Glad that the change helps. :)

[Viet-Trung Luu]

To follow up on this, the old ErrorHandler/set_error_handler() are now gone from the mojo repo.

Others pulling the mojo repo will have to make sure they've made the conversions before rolling. Luckily, it's easy -- typically:

1. Stop including "mojo/public/cpp/bindings/error_handler.h" and stop inheriting from ErrorHandler. (If ErrorHandler was the only superclass, you may need to remove "override" from your destructor.)
2. Remove "override" from OnConnectionError() (maybe also make it private, maybe rename it, maybe even remove it -- see below).
3. Replace set_error_handler(this); with set_connection_error_handler([this]() { OnConnectionError(); });. (If OnConnectionError()'s implementation is short, you may want to just inline it.)
4. In the unlikely case that you have set_error_handler(nullptr);, you can just replace that with set_connection_error_handler(mojo::Closure());.

Note: If all your OnConnectionError() does is delete this; and you're using a mojo::Binding, you may want to just use a mojo::StrongBinding instead.

To unsubscribe from this group and stop receiving emails from it, send an email to mojo-dev+u...@chromium.org.