From ffdb373a3e3fd523e78890fa8ab4ac1e8b49090f Mon Sep 17 00:00:00 2001 From: "jamesr@chromium.org" Date: Thu, 24 Jul 2014 03:26:37 +0000 Subject: Mojo: Use InterfaceFactory for service registration This adds an InterfaceFactory type and allows registering a service through its provider. Using an InterfaceFactory allows the app to be in control of the lifetime of the implementation of the interface and hides all of the implementation details of the interface from the application library code (i.e. no subclassing impl classes or anything like that). The default binding behavior is to bind the lifetime of the impl to the lifetime of the pipe, but the application can also weakly bind to a service in cases where it needs to manage the lifetime explicitly. Review URL: https://codereview.chromium.org/380413003 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@285139 0039d316-1c4b-4281-b951-d872f2087c98 --- .../cpp/application/application_connection.h | 51 +++------- mojo/public/cpp/application/interface_factory.h | 30 ++++++ .../application/interface_factory_with_context.h | 34 +++++++ .../cpp/application/lib/mojo_main_chromium.cc | 16 +-- .../cpp/application/lib/mojo_main_standalone.cc | 15 ++- .../public/cpp/application/lib/service_connector.h | 110 +++------------------ mojo/public/cpp/bindings/interface_impl.h | 51 ++++++++-- .../cpp/bindings/lib/interface_impl_internal.h | 32 +++++- .../cpp/bindings/tests/handle_passing_unittest.cc | 12 --- .../cpp/bindings/tests/interface_ptr_unittest.cc | 8 -- .../bindings/tests/request_response_unittest.cc | 4 - .../cpp/bindings/tests/validation_unittest.cc | 4 - 12 files changed, 182 insertions(+), 185 deletions(-) create mode 100644 mojo/public/cpp/application/interface_factory.h create mode 100644 mojo/public/cpp/application/interface_factory_with_context.h (limited to 'mojo/public') diff --git a/mojo/public/cpp/application/application_connection.h b/mojo/public/cpp/application/application_connection.h index 2044af1..c0e3480 100644 --- a/mojo/public/cpp/application/application_connection.h +++ b/mojo/public/cpp/application/application_connection.h @@ -21,51 +21,26 @@ namespace mojo { // to implement a service named Foo. // That class must subclass an InterfaceImpl specialization. // -// If there is context that is to be shared amongst all instances, define a -// constructor with that class as its only argument, otherwise define an empty -// constructor. +// Then implement an InterfaceFactory that binds instances of FooImpl to +// InterfaceRequests and register that on the connection. // -// class FooImpl : public InterfaceImpl { -// public: -// explicit FooImpl(ApplicationConnnection* connection) {} -// }; +// connection->AddService(&factory); // -// or +// Or if you have multiple factories implemented by the same type, explicitly +// specify the interface to register the factory for: // -// class BarImpl : public InterfaceImpl { -// public: -// // contexts will remain valid for the lifetime of BarImpl. -// BarImpl(ApplicationConnnection* connection, BarContext* service_context) -// : connection_(connection), servicecontext_(context) {} +// connection->AddService(&my_foo_and_bar_factory_); +// connection->AddService(&my_foo_and_bar_factory_); // -// Create an ApplicationDelegate instance and pass it to the constructor -// of an ApplicationImpl. The delegate will be called when new connections are -// made to other applications. -// -// connection->AddService(); -// -// BarContext context; -// connection->AddService(&context); +// The InterfaceFactory must outlive the ApplicationConnection. class ApplicationConnection { public: virtual ~ApplicationConnection(); - // Impl’s constructor will receive two arguments: - // Impl::Impl(Application::Context* app_context, - // ServiceContext* svc_context) - template - void AddService(ServiceContext* context) { - AddServiceConnector( - new internal::ServiceConnector(Impl::Name_, - context)); - } - - // Impl’s constructor will receive one argument: - // Impl::Impl(Application::Context* app_context) - template - void AddService() { + template + void AddService(InterfaceFactory* factory) { AddServiceConnector( - new internal::ServiceConnector(Impl::Name_, NULL)); + new internal::InterfaceFactoryConnector(factory)); } // Connect to the service implementing |Interface|. @@ -96,8 +71,8 @@ class ApplicationConnection { // Raw ServiceProvider interface to remote application. virtual ServiceProvider* GetServiceProvider() = 0; -private: - virtual void AddServiceConnector( + private: + virtual void AddServiceConnector( internal::ServiceConnectorBase* service_connector) = 0; }; diff --git a/mojo/public/cpp/application/interface_factory.h b/mojo/public/cpp/application/interface_factory.h new file mode 100644 index 0000000..90abd13 --- /dev/null +++ b/mojo/public/cpp/application/interface_factory.h @@ -0,0 +1,30 @@ +// Copyright 2014 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef MOJO_PUBLIC_CPP_APPLICATION_INTERFACE_FACTORY_H_ +#define MOJO_PUBLIC_CPP_APPLICATION_INTERFACE_FACTORY_H_ + +#include "mojo/public/cpp/bindings/interface_impl.h" +#include "mojo/public/cpp/bindings/interface_request.h" + +namespace mojo { + +class ApplicationConnection; +template class InterfaceRequest; + +// Implement this class to provide implementations of a given interface and +// bind them to incoming requests. The implementation of this class is +// responsible for managing the lifetime of the implementations of the +// interface. +template +class InterfaceFactory { + public: + virtual ~InterfaceFactory() {} + virtual void Create(ApplicationConnection* connection, + InterfaceRequest request) = 0; +}; + +} // namespace mojo + +#endif // MOJO_PUBLIC_CPP_APPLICATION_INTERFACE_FACTORY_H_ diff --git a/mojo/public/cpp/application/interface_factory_with_context.h b/mojo/public/cpp/application/interface_factory_with_context.h new file mode 100644 index 0000000..8ea7215 --- /dev/null +++ b/mojo/public/cpp/application/interface_factory_with_context.h @@ -0,0 +1,34 @@ +// Copyright 2014 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef MOJO_PUBLIC_CPP_APPLICATION_INTERFACE_FACTORY_WITH_CONTEXT_H_ +#define MOJO_PUBLIC_CPP_APPLICATION_INTERFACE_FACTORY_WITH_CONTEXT_H_ + +#include "mojo/public/cpp/application/interface_factory.h" + +namespace mojo { + +// Use this class to allocate and bind instances of Impl constructed with a +// context parameter to interface requests. The lifetime of the constructed +// Impls is bound to the pipe. +template +class InterfaceFactoryWithContext : public InterfaceFactory { + public: + explicit InterfaceFactoryWithContext(Context* context) : context_(context) {} + virtual ~InterfaceFactoryWithContext() {} + + virtual void Create(ApplicationConnection* connection, + InterfaceRequest request) MOJO_OVERRIDE { + BindToRequest(new Impl(context_), &request); + } + + private: + Context* context_; +}; + +} // namespace mojo + +#endif // MOJO_PUBLIC_CPP_APPLICATION_INTERFACE_FACTORY_WITH_CONTEXT_H_ diff --git a/mojo/public/cpp/application/lib/mojo_main_chromium.cc b/mojo/public/cpp/application/lib/mojo_main_chromium.cc index 269b7a2..d8c5e3f 100644 --- a/mojo/public/cpp/application/lib/mojo_main_chromium.cc +++ b/mojo/public/cpp/application/lib/mojo_main_chromium.cc @@ -14,12 +14,16 @@ extern "C" APPLICATION_EXPORT MojoResult CDECL MojoMain( #if !defined(COMPONENT_BUILD) base::AtExitManager at_exit; #endif - base::MessageLoop loop; - scoped_ptr delegate( - mojo::ApplicationDelegate::Create()); - mojo::ApplicationImpl app(delegate.get()); - app.BindShell(shell_handle); - loop.Run(); + scoped_ptr delegate; + { + // We have to shut down the MessageLoop before destroying the + // ApplicationDelegate. + base::MessageLoop loop; + delegate.reset(mojo::ApplicationDelegate::Create()); + mojo::ApplicationImpl app(delegate.get()); + app.BindShell(shell_handle); + loop.Run(); + } return MOJO_RESULT_OK; } diff --git a/mojo/public/cpp/application/lib/mojo_main_standalone.cc b/mojo/public/cpp/application/lib/mojo_main_standalone.cc index 0ba078b..6071b6b 100644 --- a/mojo/public/cpp/application/lib/mojo_main_standalone.cc +++ b/mojo/public/cpp/application/lib/mojo_main_standalone.cc @@ -10,12 +10,17 @@ extern "C" APPLICATION_EXPORT MojoResult CDECL MojoMain( MojoHandle shell_handle) { mojo::Environment env; - mojo::RunLoop loop; - mojo::ApplicationDelegate* delegate = mojo::ApplicationDelegate::Create(); + mojo::ApplicationDelegate* delegate = NULL; { - mojo::ApplicationImpl app(delegate); - app.BindShell(shell_handle); - loop.Run(); + // We have to shut down the RunLoop before destroying the + // ApplicationDelegate. + mojo::RunLoop loop; + delegate = mojo::ApplicationDelegate::Create(); + { + mojo::ApplicationImpl app(delegate); + app.BindShell(shell_handle); + loop.Run(); + } } delete delegate; diff --git a/mojo/public/cpp/application/lib/service_connector.h b/mojo/public/cpp/application/lib/service_connector.h index d2de48b..aeb7de1 100644 --- a/mojo/public/cpp/application/lib/service_connector.h +++ b/mojo/public/cpp/application/lib/service_connector.h @@ -5,10 +5,8 @@ #ifndef MOJO_PUBLIC_CPP_APPLICATION_LIB_SERVICE_CONNECTOR_H_ #define MOJO_PUBLIC_CPP_APPLICATION_LIB_SERVICE_CONNECTOR_H_ -#include - -#include - +#include "mojo/public/cpp/application/interface_factory.h" +#include "mojo/public/cpp/bindings/interface_request.h" #include "mojo/public/interfaces/service_provider/service_provider.mojom.h" namespace mojo { @@ -16,59 +14,6 @@ class ApplicationConnection; namespace internal { -template -class ServiceConnector; - -// Specialization of ServiceConnection. -// ServiceImpl: Subclass of InterfaceImpl<...>. -// Context: Type of shared context. -template -class ServiceConnection : public ServiceImpl { - public: - explicit ServiceConnection(ApplicationConnection* connection) - : ServiceImpl(connection) {} - ServiceConnection(ApplicationConnection* connection, - Context* context) : ServiceImpl(connection, context) {} - - virtual void OnConnectionError() MOJO_OVERRIDE { - service_connector_->RemoveConnection(static_cast(this)); - ServiceImpl::OnConnectionError(); - } - -private: - friend class ServiceConnector; - - // Called shortly after this class is instantiated. - void set_service_connector( - ServiceConnector* connector) { - service_connector_ = connector; - } - - ServiceConnector* service_connector_; - - MOJO_DISALLOW_COPY_AND_ASSIGN(ServiceConnection); -}; - -template -struct ServiceConstructor { - static ServiceConnection* New( - ApplicationConnection* connection, - Context* context) { - return new ServiceConnection( - connection, context); - } -}; - -template -struct ServiceConstructor { - public: - static ServiceConnection* New( - ApplicationConnection* connection, - void* context) { - return new ServiceConnection(connection); - } -}; - class ServiceConnectorBase { public: ServiceConnectorBase(const std::string& name); @@ -86,53 +31,22 @@ class ServiceConnectorBase { MOJO_DISALLOW_COPY_AND_ASSIGN(ServiceConnectorBase); }; -template -class ServiceConnector : public internal::ServiceConnectorBase { +template +class InterfaceFactoryConnector : public ServiceConnectorBase { public: - ServiceConnector(const std::string& name, Context* context = NULL) - : ServiceConnectorBase(name), context_(context) {} - - virtual ~ServiceConnector() { - ConnectionList doomed; - doomed.swap(connections_); - for (typename ConnectionList::iterator it = doomed.begin(); - it != doomed.end(); ++it) { - delete *it; - } - assert(connections_.empty()); // No one should have added more! - } + explicit InterfaceFactoryConnector(InterfaceFactory* factory) + : ServiceConnectorBase(Interface::Name_), factory_(factory) {} + virtual ~InterfaceFactoryConnector() {} virtual void ConnectToService(const std::string& name, - ScopedMessagePipeHandle handle) MOJO_OVERRIDE { - ServiceConnection* impl = - ServiceConstructor::New(application_connection_, - context_); - impl->set_service_connector(this); - BindToPipe(impl, handle.Pass()); - - connections_.push_back(impl); - } - - void RemoveConnection(ServiceImpl* impl) { - // Called from ~ServiceImpl, in response to a connection error. - for (typename ConnectionList::iterator it = connections_.begin(); - it != connections_.end(); ++it) { - if (*it == impl) { - delete impl; - connections_.erase(it); - return; - } - } + ScopedMessagePipeHandle client_handle) { + factory_->Create(application_connection_, + MakeRequest(client_handle.Pass())); } - Context* context() const { return context_; } - private: - typedef std::vector ConnectionList; - ConnectionList connections_; - Context* context_; - - MOJO_DISALLOW_COPY_AND_ASSIGN(ServiceConnector); + InterfaceFactory* factory_; + MOJO_DISALLOW_COPY_AND_ASSIGN(InterfaceFactoryConnector); }; } // namespace internal diff --git a/mojo/public/cpp/bindings/interface_impl.h b/mojo/public/cpp/bindings/interface_impl.h index 4a78cb4..da1055d 100644 --- a/mojo/public/cpp/bindings/interface_impl.h +++ b/mojo/public/cpp/bindings/interface_impl.h @@ -19,6 +19,7 @@ template class InterfaceImpl : public internal::InterfaceImplBase { public: typedef typename Interface::Client Client; + typedef Interface ImplementedInterface; InterfaceImpl() : internal_state_(this) {} virtual ~InterfaceImpl() {} @@ -57,12 +58,15 @@ class InterfaceImpl : public internal::InterfaceImplBase { // Takes an instance of an InterfaceImpl<..> subclass and binds it to the given // MessagePipe. The instance is returned for convenience in member initializer -// lists, etc. If the pipe is closed, the instance's OnConnectionError method -// will be called. +// lists, etc. +// +// If the pipe is closed, the instance's OnConnectionError method will be called +// and then the instance will be deleted. // // The instance is also bound to the current thread. Its methods will only be -// called on the current thread, and if the current thread exits, then it will -// also be deleted, and along with it, its end point of the pipe will be closed. +// called on the current thread, and if the current thread exits, then the end +// point of the pipe will be closed and the error handler's OnConnectionError +// method will be called. // // Before returning, the instance's OnConnectionEstablished method is called. template @@ -70,14 +74,25 @@ Impl* BindToPipe( Impl* instance, ScopedMessagePipeHandle handle, const MojoAsyncWaiter* waiter = Environment::GetDefaultAsyncWaiter()) { - instance->internal_state()->Bind(handle.Pass(), waiter); + instance->internal_state()->Bind(handle.Pass(), true, waiter); + return instance; +} + +// Like BindToPipe but does not delete the instance after a channel error. +template +Impl* WeakBindToPipe( + Impl* instance, + ScopedMessagePipeHandle handle, + const MojoAsyncWaiter* waiter = Environment::GetDefaultAsyncWaiter()) { + instance->internal_state()->Bind(handle.Pass(), false, waiter); return instance; } // Takes an instance of an InterfaceImpl<..> subclass and binds it to the given // InterfacePtr<..>. The instance is returned for convenience in member // initializer lists, etc. If the pipe is closed, the instance's -// OnConnectionError method will be called. +// OnConnectionError method will be called and then the instance will be +// deleted. // // The instance is also bound to the current thread. Its methods will only be // called on the current thread, and if the current thread exits, then it will @@ -89,14 +104,25 @@ Impl* BindToProxy( Impl* instance, InterfacePtr* ptr, const MojoAsyncWaiter* waiter = Environment::GetDefaultAsyncWaiter()) { - instance->internal_state()->BindProxy(ptr, waiter); + instance->internal_state()->BindProxy(ptr, true, waiter); + return instance; +} + +// Like BindToProxy but does not delete the instance after a channel error. +template +Impl* WeakBindToProxy( + Impl* instance, + InterfacePtr* ptr, + const MojoAsyncWaiter* waiter = Environment::GetDefaultAsyncWaiter()) { + instance->internal_state()->BindProxy(ptr, false, waiter); return instance; } // Takes an instance of an InterfaceImpl<..> subclass and binds it to the given // InterfaceRequest<..>. The instance is returned for convenience in member // initializer lists, etc. If the pipe is closed, the instance's -// OnConnectionError method will be called. +// OnConnectionError method will be called and then the instance will be +// deleted. // // The instance is also bound to the current thread. Its methods will only be // called on the current thread, and if the current thread exits, then it will @@ -112,6 +138,15 @@ Impl* BindToRequest( return BindToPipe(instance, request->PassMessagePipe(), waiter); } +// Like BindToRequest but does not delete the instance after a channel error. +template +Impl* WeakBindToRequest( + Impl* instance, + InterfaceRequest* request, + const MojoAsyncWaiter* waiter = Environment::GetDefaultAsyncWaiter()) { + return WeakBindToPipe(instance, request->PassMessagePipe(), waiter); +} + } // namespace mojo #endif // MOJO_PUBLIC_CPP_BINDINGS_INTERFACE_IMPL_H_ diff --git a/mojo/public/cpp/bindings/lib/interface_impl_internal.h b/mojo/public/cpp/bindings/lib/interface_impl_internal.h index d94d12d..79f59d8 100644 --- a/mojo/public/cpp/bindings/lib/interface_impl_internal.h +++ b/mojo/public/cpp/bindings/lib/interface_impl_internal.h @@ -31,12 +31,21 @@ class InterfaceImplState : public ErrorHandler { explicit InterfaceImplState(InterfaceImplBase* instance) : router_(NULL), - proxy_(NULL) { + proxy_(NULL), + instance_bound_to_pipe_(false) +#ifndef NDEBUG + , + deleting_instance_due_to_error_(false) +#endif + { MOJO_DCHECK(instance); stub_.set_sink(instance); } virtual ~InterfaceImplState() { +#ifndef NDEBUG + MOJO_DCHECK(!instance_bound_to_pipe_ || deleting_instance_due_to_error_); +#endif delete proxy_; if (router_) { router_->set_error_handler(NULL); @@ -46,13 +55,15 @@ class InterfaceImplState : public ErrorHandler { void BindProxy( InterfacePtr* ptr, + bool instance_bound_to_pipe, const MojoAsyncWaiter* waiter = Environment::GetDefaultAsyncWaiter()) { MessagePipe pipe; ptr->Bind(pipe.handle0.Pass(), waiter); - Bind(pipe.handle1.Pass(), waiter); + Bind(pipe.handle1.Pass(), instance_bound_to_pipe, waiter); } void Bind(ScopedMessagePipeHandle handle, + bool instance_bound_to_pipe, const MojoAsyncWaiter* waiter) { MOJO_DCHECK(!router_); @@ -67,6 +78,8 @@ class InterfaceImplState : public ErrorHandler { proxy_ = new typename Client::Proxy_(router_); + instance_bound_to_pipe_ = instance_bound_to_pipe; + instance()->OnConnectionEstablished(); } @@ -84,12 +97,27 @@ class InterfaceImplState : public ErrorHandler { } virtual void OnConnectionError() MOJO_OVERRIDE { + // If the the instance is not bound to the pipe, the instance might choose + // to delete itself in the OnConnectionError handler, which would in turn + // delete this. Save the error behavior before invoking the error handler + // so we can correctly decide what to do. + bool bound = instance_bound_to_pipe_; instance()->OnConnectionError(); + if (!bound) + return; +#ifndef NDEBUG + deleting_instance_due_to_error_ = true; +#endif + delete instance(); } Router* router_; typename Client::Proxy_* proxy_; typename Interface::Stub_ stub_; + bool instance_bound_to_pipe_; +#ifndef NDEBUG + bool deleting_instance_due_to_error_; +#endif MOJO_DISALLOW_COPY_AND_ASSIGN(InterfaceImplState); }; diff --git a/mojo/public/cpp/bindings/tests/handle_passing_unittest.cc b/mojo/public/cpp/bindings/tests/handle_passing_unittest.cc index d2e287f..696e613 100644 --- a/mojo/public/cpp/bindings/tests/handle_passing_unittest.cc +++ b/mojo/public/cpp/bindings/tests/handle_passing_unittest.cc @@ -29,10 +29,6 @@ class StringRecorder { class ImportedInterfaceImpl : public InterfaceImpl { public: - virtual void OnConnectionError() MOJO_OVERRIDE { - delete this; - } - virtual void DoSomething() MOJO_OVERRIDE { do_something_count_++; } @@ -46,10 +42,6 @@ int ImportedInterfaceImpl::do_something_count_ = 0; class SampleNamedObjectImpl : public InterfaceImpl { public: - virtual void OnConnectionError() MOJO_OVERRIDE { - delete this; - } - virtual void SetName(const mojo::String& name) MOJO_OVERRIDE { name_ = name; } @@ -65,10 +57,6 @@ class SampleNamedObjectImpl : public InterfaceImpl { class SampleFactoryImpl : public InterfaceImpl { public: - virtual void OnConnectionError() MOJO_OVERRIDE { - delete this; - } - virtual void DoStuff(sample::RequestPtr request, ScopedMessagePipeHandle pipe) MOJO_OVERRIDE { std::string text1; diff --git a/mojo/public/cpp/bindings/tests/interface_ptr_unittest.cc b/mojo/public/cpp/bindings/tests/interface_ptr_unittest.cc index 533d990..e3e9f1e 100644 --- a/mojo/public/cpp/bindings/tests/interface_ptr_unittest.cc +++ b/mojo/public/cpp/bindings/tests/interface_ptr_unittest.cc @@ -41,10 +41,6 @@ class MathCalculatorImpl : public InterfaceImpl { got_connection_ = true; } - virtual void OnConnectionError() MOJO_OVERRIDE { - delete this; - } - virtual void Clear() MOJO_OVERRIDE { client()->Output(total_); } @@ -164,10 +160,6 @@ class ReentrantServiceImpl : public InterfaceImpl { got_connection_ = true; } - virtual void OnConnectionError() MOJO_OVERRIDE { - delete this; - } - bool got_connection() const { return got_connection_; } diff --git a/mojo/public/cpp/bindings/tests/request_response_unittest.cc b/mojo/public/cpp/bindings/tests/request_response_unittest.cc index 338d968..a082a09 100644 --- a/mojo/public/cpp/bindings/tests/request_response_unittest.cc +++ b/mojo/public/cpp/bindings/tests/request_response_unittest.cc @@ -15,10 +15,6 @@ namespace { class ProviderImpl : public InterfaceImpl { public: - virtual void OnConnectionError() MOJO_OVERRIDE { - delete this; - } - virtual void EchoString( const String& a, const Callback& callback) MOJO_OVERRIDE { diff --git a/mojo/public/cpp/bindings/tests/validation_unittest.cc b/mojo/public/cpp/bindings/tests/validation_unittest.cc index 9269212..6463a9e 100644 --- a/mojo/public/cpp/bindings/tests/validation_unittest.cc +++ b/mojo/public/cpp/bindings/tests/validation_unittest.cc @@ -275,10 +275,6 @@ class IntegrationTestInterface1Impl virtual void Method0(BasicStructPtr param0) MOJO_OVERRIDE { } - - virtual void OnConnectionError() MOJO_OVERRIDE { - delete this; - } }; TEST_F(ValidationTest, InputParser) { -- cgit v1.1