Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 17d6aef7 authored by Jack He's avatar Jack He
Browse files

Test: Stability improvements on bluetooth_stack_with_facade 

Call std::map::erase before std::map::emplace
* std::map::emplace does not replace existing value if the key
  already exists. The newly constructed element will be destroyed
  immediately if there already is an element with the key in the
  container. Hence, if we want to emplace a new value, we should
  always erase the old one or use std::map::insert_or_assign
* If the value is move-only, insert_or_assign may not work

Avoid shutting down gRPC server in signal handler
* See https://github.com/grpc/grpc/issues/24884
* Create a separate shutdown thread for this purpose

Make sure all facade server values are deleted when exit
* To avoid memory leak

Do not call server->Wait() on a deleted gRPC server
* Instead, use a std::uniptr to hold the server object until the
  wrapper class is freed

Bug: 228619929
Bug: 235872679
Bug: 232048916
Ignore-AOSP-First: cherry-pick

Merged-In: Idf64df4ca61e0820ec9d48fdc9b0d191ddf0d352
Change-Id: Idf64df4ca61e0820ec9d48fdc9b0d191ddf0d352
parent df236fc7

system/gd/facade/facade_main.cc

+19 −1
Original line number Diff line number Diff line
@@ -51,13 +51,15 @@ extern "C" const char* __asan_default_options() { 
namespace {

::bluetooth::facade::GrpcRootServer grpc_root_server;



std::promise<void> interrupt_promise;

std::future<void> interrupt_future;

bool interrupted = false;

struct sigaction old_act = {};

void interrupt_handler(int signal_number) {

  if (!interrupted) {

    interrupted = true;

    LOG_INFO("Stopping gRPC root server due to signal: %s[%d]", strsignal(signal_number), signal_number);

    grpc_root_server.StopServer();

    interrupt_promise.set_value();

  } else {

    LOG_WARN("Already interrupted by signal: %s[%d]", strsignal(signal_number), signal_number);

  }
@@ -94,6 +96,15 @@ bool crash_callback(const void* crash_context, size_t crash_context_size, void* 
  return true;

}



// Need to stop server on a thread that is not part of a signal handler due to an issue with gRPC

// See: https://github.com/grpc/grpc/issues/24884

void thread_check_shutdown() {

  LOG_INFO("shutdown thread waiting for interruption");

  interrupt_future.wait();

  LOG_INFO("interrupted, stopping server");

  grpc_root_server.StopServer();

}



}  // namespace



// The entry point for the binary with libbluetooth + facades
@@ -148,9 +159,16 @@ int main(int argc, const char** argv) { 
    LOG_ERROR("sigaction error: %s", strerror(errno));

  }



  LOG_INFO("Starting Server");

  grpc_root_server.StartServer("0.0.0.0", root_server_port, grpc_port);

  LOG_INFO("Server started");

  auto wait_thread = std::thread([] { grpc_root_server.RunGrpcLoop(); });

  interrupt_future = interrupt_promise.get_future();

  auto shutdown_thread = std::thread{thread_check_shutdown};

  wait_thread.join();

  LOG_INFO("Server terminated");

  shutdown_thread.join();

  LOG_INFO("Shutdown thread terminated");



  return 0;

}

system/gd/facade/grpc_root_server.cc

+29 −35
Original line number Diff line number Diff line
@@ -29,26 +29,15 @@ 
#include "hci/facade/le_advertising_manager_facade.h"

#include "hci/facade/le_initiator_address_facade.h"

#include "hci/facade/le_scanning_manager_facade.h"

#include "hci/hci_layer.h"

#include "hci/le_advertising_manager.h"

#include "hci/le_scanning_manager.h"

#include "iso/facade.h"

#include "l2cap/classic/facade.h"

#include "l2cap/le/facade.h"

#include "neighbor/connectability.h"

#include "neighbor/discoverability.h"

#include "neighbor/facade/facade.h"

#include "neighbor/inquiry.h"

#include "neighbor/name.h"

#include "neighbor/page.h"

#include "os/log.h"

#include "os/thread.h"

#include "security/facade.h"

#include "security/security_module.h"

#include "shim/dumpsys.h"

#include "shim/facade/facade.h"

#include "stack_manager.h"

#include "storage/storage_module.h"



namespace bluetooth {

namespace facade {
@@ -57,10 +46,9 @@ using ::blueberry::facade::BluetoothModule; 
using ::bluetooth::grpc::GrpcModule;

using ::bluetooth::os::Thread;



namespace {

class RootFacadeService : public ::blueberry::facade::RootFacade::Service {

 public:

  RootFacadeService(int grpc_port) : grpc_port_(grpc_port) {}

  explicit RootFacadeService(int grpc_port) : grpc_port_(grpc_port) {}



  ::grpc::Status StartStack(

      ::grpc::ServerContext* context,
@@ -123,13 +111,13 @@ class RootFacadeService : public ::blueberry::facade::RootFacade::Service { 
        return ::grpc::Status(::grpc::StatusCode::INVALID_ARGUMENT, "invalid module under test");

    }



    stack_thread_ = new Thread("stack_thread", Thread::Priority::NORMAL);

    stack_manager_.StartUp(&modules, stack_thread_);

    stack_thread_ = std::make_unique<Thread>("stack_thread", Thread::Priority::NORMAL);

    stack_manager_.StartUp(&modules, stack_thread_.get());



    GrpcModule* grpc_module = stack_manager_.GetInstance<GrpcModule>();

    grpc_module->StartServer("0.0.0.0", grpc_port_);



    grpc_loop_thread_ = new std::thread([grpc_module] { grpc_module->RunGrpcLoop(); });

    grpc_loop_thread_ = std::make_unique<std::thread>([grpc_module] { grpc_module->RunGrpcLoop(); });

    is_running_ = true;



    return ::grpc::Status::OK;
@@ -145,50 +133,56 @@ class RootFacadeService : public ::blueberry::facade::RootFacade::Service { 


    stack_manager_.GetInstance<GrpcModule>()->StopServer();

    grpc_loop_thread_->join();

    delete grpc_loop_thread_;

    grpc_loop_thread_.reset();



    stack_manager_.ShutDown();

    delete stack_thread_;

    stack_thread_.reset();

    is_running_ = false;

    return ::grpc::Status::OK;

  }



 private:

  Thread* stack_thread_ = nullptr;

  std::unique_ptr<Thread> stack_thread_ = nullptr;

  bool is_running_ = false;

  std::thread* grpc_loop_thread_ = nullptr;

  std::unique_ptr<std::thread> grpc_loop_thread_ = nullptr;

  StackManager stack_manager_;

  int grpc_port_ = 8898;

};



RootFacadeService* root_facade_service;

}  // namespace

struct GrpcRootServer::impl {

  bool started_ = false;

  std::unique_ptr<RootFacadeService> root_facade_service_ = nullptr;

  std::unique_ptr<::grpc::Server> server_ = nullptr;

};



GrpcRootServer::GrpcRootServer() : pimpl_(new impl()) {}



GrpcRootServer::~GrpcRootServer() = default;



void GrpcRootServer::StartServer(const std::string& address, int grpc_root_server_port, int grpc_port) {

  ASSERT(!started_);

  started_ = true;

  ASSERT(!pimpl_->started_);

  pimpl_->started_ = true;



  std::string listening_port = address + ":" + std::to_string(grpc_root_server_port);

  ::grpc::ServerBuilder builder;

  root_facade_service = new RootFacadeService(grpc_port);

  builder.RegisterService(root_facade_service);



  pimpl_->root_facade_service_ = std::make_unique<RootFacadeService>(grpc_port);

  builder.RegisterService(pimpl_->root_facade_service_.get());

  builder.AddListeningPort(listening_port, ::grpc::InsecureServerCredentials());

  server_ = builder.BuildAndStart();

  pimpl_->server_ = builder.BuildAndStart();



  ASSERT(server_ != nullptr);

  ASSERT(pimpl_->server_ != nullptr);

}



void GrpcRootServer::StopServer() {

  ASSERT(started_);

  server_->Shutdown();

  started_ = false;

  server_.reset();

  delete root_facade_service;

  ASSERT(pimpl_->started_);

  pimpl_->server_->Shutdown();

  pimpl_->started_ = false;

}



void GrpcRootServer::RunGrpcLoop() {

  ASSERT(started_);

  server_->Wait();

  ASSERT(pimpl_->started_);

  pimpl_->server_->Wait();

}



}  // namespace facade

system/gd/facade/grpc_root_server.h

+5 −2
Original line number Diff line number Diff line
@@ -26,6 +26,9 @@ namespace facade { 


class GrpcRootServer {

 public:

  GrpcRootServer();

  ~GrpcRootServer();



  void StartServer(const std::string& address, int grpc_root_server_port, int grpc_port);



  void StopServer();
@@ -33,8 +36,8 @@ class GrpcRootServer { 
  void RunGrpcLoop();



 private:

  bool started_ = false;

  std::unique_ptr<::grpc::Server> server_ = nullptr;

  struct impl;

  std::unique_ptr<impl> pimpl_;

};



}  // namespace facade

system/gd/hci/facade/acl_manager_facade.cc

+1 −0
Original line number Diff line number Diff line
@@ -355,6 +355,7 @@ class AclManagerFacadeService : public AclManagerFacade::Service, public Connect 
    std::unique_lock<std::mutex> lock(acl_connections_mutex_);

    std::shared_ptr<ClassicAclConnection> shared_connection = std::move(connection);

    uint16_t handle = to_handle(current_connection_request_);

    acl_connections_.erase(handle);

    acl_connections_.emplace(

        std::piecewise_construct,

        std::forward_as_tuple(handle),