From 707a6e469f9c9621f09f42c6c5419587580398ab Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Mon, 8 May 2017 17:51:07 -0700 Subject: [PATCH 1/3] adb: libusb: replace sleep with timed CV wait. Instead of sleeping for 500ms at the end of every device poll loop, use a timed condition variable wait so that we can tell the device poll thread to immediately commit suicide. Bug: http://b/37869663 Test: adb kill-server; adb start-server Change-Id: I597071866f7d9ef91900411727345d32c1a97556 --- adb/client/usb_libusb.cpp | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/adb/client/usb_libusb.cpp b/adb/client/usb_libusb.cpp index 15d4b7a8b..5a6586596 100644 --- a/adb/client/usb_libusb.cpp +++ b/adb/client/usb_libusb.cpp @@ -152,7 +152,9 @@ static auto& usb_handles = *new std::unordered_map terminate_device_poll_thread(false); +static bool terminate_device_poll_thread = false; +static auto& device_poll_mutex = *new std::mutex(); +static auto& device_poll_cv = *new std::condition_variable(); static std::string get_device_address(libusb_device* device) { return StringPrintf("usb:%d:%d", libusb_get_bus_number(device), @@ -376,7 +378,7 @@ static void process_device(libusb_device* device) { static void poll_for_devices() { libusb_device** list; adb_thread_setname("device poll"); - while (!terminate_device_poll_thread) { + while (true) { const ssize_t device_count = libusb_get_device_list(nullptr, &list); LOG(VERBOSE) << "found " << device_count << " attached devices"; @@ -389,7 +391,10 @@ static void poll_for_devices() { adb_notify_device_scan_complete(); - std::this_thread::sleep_for(500ms); + std::unique_lock lock(device_poll_mutex); + if (device_poll_cv.wait_for(lock, 500ms, []() { return terminate_device_poll_thread; })) { + return; + } } } @@ -412,7 +417,11 @@ void usb_init() { // TODO: Use libusb_hotplug_* instead? device_poll_thread = new std::thread(poll_for_devices); android::base::at_quick_exit([]() { - terminate_device_poll_thread = true; + { + std::unique_lock lock(device_poll_mutex); + terminate_device_poll_thread = true; + } + device_poll_cv.notify_all(); device_poll_thread->join(); }); } From b3c14ec693d4c061fc9ec7fdaa7897169d179479 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Mon, 8 May 2017 18:37:17 -0700 Subject: [PATCH 2/3] adb: make `adb kill-server` wait for the server to die. Make the host:kill service shutdown its socket on process exit, instead of immediately. Also, unify the two 'kill-server' implementations and hide _adb_connect. Bug: http://b/37104408 Test: adb kill-server; adb start-server Change-Id: I9475f5d084d5fb91d33e393f2fd4e34056613384 --- adb/adb.cpp | 11 ++++------- adb/adb_client.cpp | 34 +++++++++++++++++++++------------- adb/adb_client.h | 4 +++- adb/commandline.cpp | 20 +------------------- 4 files changed, 29 insertions(+), 40 deletions(-) diff --git a/adb/adb.cpp b/adb/adb.cpp index 0181daa60..55c64ec4e 100644 --- a/adb/adb.cpp +++ b/adb/adb.cpp @@ -1054,15 +1054,12 @@ int handle_host_request(const char* service, TransportType type, if (strcmp(service, "kill") == 0) { fprintf(stderr, "adb server killed by remote request\n"); fflush(stdout); + + // Send a reply even though we don't read it anymore, so that old versions + // of adb that do read it don't spew error messages. SendOkay(reply_fd); - // On Windows, if the process exits with open sockets that - // shutdown(SD_SEND) has not been called on, TCP RST segments will be - // sent to the peers which will cause their next recv() to error-out - // with WSAECONNRESET. In the case of this code, that means the client - // may not read the OKAY sent above. - adb_shutdown(reply_fd); - + // Rely on process exit to close the socket for us. android::base::quick_exit(0); } diff --git a/adb/adb_client.cpp b/adb/adb_client.cpp index 4f3ff25df..f5d0f02df 100644 --- a/adb/adb_client.cpp +++ b/adb/adb_client.cpp @@ -123,7 +123,7 @@ bool adb_status(int fd, std::string* error) { return false; } -int _adb_connect(const std::string& service, std::string* error) { +static int _adb_connect(const std::string& service, std::string* error) { D("_adb_connect: %s", service.c_str()); if (service.empty() || service.size() > MAX_PAYLOAD_V1) { *error = android::base::StringPrintf("bad service name length (%zd)", @@ -158,6 +158,25 @@ int _adb_connect(const std::string& service, std::string* error) { return fd; } +bool adb_kill_server() { + D("adb_kill_server"); + std::string reason; + int fd = socket_spec_connect(__adb_server_socket_spec, &reason); + if (fd < 0) { + fprintf(stderr, "cannot connect to daemon at %s: %s\n", __adb_server_socket_spec, + reason.c_str()); + return true; + } + + if (!SendProtocolString(fd, "host:kill")) { + fprintf(stderr, "error: write failure during connection: %s\n", strerror(errno)); + return false; + } + + ReadOrderlyShutdown(fd); + return true; +} + int adb_connect(const std::string& service, std::string* error) { // first query the adb server's version int fd = _adb_connect("host:version", error); @@ -214,18 +233,7 @@ int adb_connect(const std::string& service, std::string* error) { if (version != ADB_SERVER_VERSION) { fprintf(stderr, "adb server version (%d) doesn't match this client (%d); killing...\n", version, ADB_SERVER_VERSION); - fd = _adb_connect("host:kill", error); - if (fd >= 0) { - ReadOrderlyShutdown(fd); - adb_close(fd); - } else { - // If we couldn't connect to the server or had some other error, - // report it, but still try to start the server. - fprintf(stderr, "error: %s\n", error->c_str()); - } - - /* XXX can we better detect its death? */ - std::this_thread::sleep_for(2s); + adb_kill_server(); goto start_server; } } diff --git a/adb/adb_client.h b/adb/adb_client.h index d07c1e96b..fabec0050 100644 --- a/adb/adb_client.h +++ b/adb/adb_client.h @@ -26,7 +26,9 @@ // Connect to adb, connect to the named service, and return a valid fd for // interacting with that service upon success or a negative number on failure. int adb_connect(const std::string& service, std::string* _Nonnull error); -int _adb_connect(const std::string& service, std::string* _Nonnull error); + +// Kill the currently running adb server, if it exists. +bool adb_kill_server(); // Connect to adb, connect to the named service, returns true if the connection // succeeded AND the service returned OKAY. Outputs any returned error otherwise. diff --git a/adb/commandline.cpp b/adb/commandline.cpp index a9b1540d5..f49c69daf 100644 --- a/adb/commandline.cpp +++ b/adb/commandline.cpp @@ -1546,25 +1546,7 @@ int adb_commandline(int argc, const char** argv) { return 0; } else if (!strcmp(argv[0], "kill-server")) { - std::string error; - int fd = _adb_connect("host:kill", &error); - if (fd == -2) { - // Failed to make network connection to server. Don't output the - // network error since that is expected. - fprintf(stderr,"* server not running *\n"); - // Successful exit code because the server is already "killed". - return 0; - } else if (fd == -1) { - // Some other error. - fprintf(stderr, "error: %s\n", error.c_str()); - return 1; - } else { - // Successfully connected, kill command sent, okay status came back. - // Server should exit() in a moment, if not already. - ReadOrderlyShutdown(fd); - adb_close(fd); - return 0; - } + return adb_kill_server() ? 0 : 1; } else if (!strcmp(argv[0], "sideload")) { if (argc != 2) return syntax_error("sideload requires an argument"); From 01b7bc43e90b951675cabe88313b96f57b2da712 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Tue, 9 May 2017 13:43:35 -0700 Subject: [PATCH 3/3] adb: move all cleanup to a function with defined ordering. We want to explicitly define the order in which we teardown adb, so move all of the at_quick_exits sprinkled throughout into one function containing all of the cleanup functions. Bug: http://b/37104408 Test: adb kill-server; adb start-server Change-Id: I394f5782eb147e394d4b87df1ba364c061de4b90 --- adb/adb_listeners.cpp | 33 ++++++++++++++++++++++++++------- adb/adb_listeners.h | 2 ++ adb/client/main.cpp | 15 +++++++++++++-- adb/client/usb_dispatch.cpp | 6 ++++++ adb/client/usb_libusb.cpp | 20 +++++++++++++------- adb/client/usb_windows.cpp | 2 ++ adb/transport.cpp | 17 ++++++++--------- adb/transport.h | 1 + adb/usb.h | 1 + 9 files changed, 72 insertions(+), 25 deletions(-) diff --git a/adb/adb_listeners.cpp b/adb/adb_listeners.cpp index 18b14923c..30cb29b88 100644 --- a/adb/adb_listeners.cpp +++ b/adb/adb_listeners.cpp @@ -19,8 +19,12 @@ #include #include +#include +#include + #include #include +#include #include #include "socket_spec.h" @@ -64,8 +68,9 @@ alistener::~alistener() { // listener_list retains ownership of all created alistener objects. Removing an alistener from // this list will cause it to be deleted. +static auto& listener_list_mutex = *new std::mutex(); typedef std::list> ListenerList; -static ListenerList& listener_list = *new ListenerList(); +static ListenerList& listener_list GUARDED_BY(listener_list_mutex) = *new ListenerList(); static void ss_listener_event_func(int _fd, unsigned ev, void *_l) { if (ev & FDE_READ) { @@ -108,7 +113,8 @@ static void listener_event_func(int _fd, unsigned ev, void* _l) } // Called as a transport disconnect function. |arg| is the raw alistener*. -static void listener_disconnect(void* arg, atransport*) { +static void listener_disconnect(void* arg, atransport*) EXCLUDES(listener_list_mutex) { + std::lock_guard lock(listener_list_mutex); for (auto iter = listener_list.begin(); iter != listener_list.end(); ++iter) { if (iter->get() == arg) { (*iter)->transport = nullptr; @@ -119,7 +125,8 @@ static void listener_disconnect(void* arg, atransport*) { } // Write the list of current listeners (network redirections) into a string. -std::string format_listeners() { +std::string format_listeners() EXCLUDES(listener_list_mutex) { + std::lock_guard lock(listener_list_mutex); std::string result; for (auto& l : listener_list) { // Ignore special listeners like those for *smartsocket* @@ -135,7 +142,9 @@ std::string format_listeners() { return result; } -InstallStatus remove_listener(const char* local_name, atransport* transport) { +InstallStatus remove_listener(const char* local_name, atransport* transport) + EXCLUDES(listener_list_mutex) { + std::lock_guard lock(listener_list_mutex); for (auto iter = listener_list.begin(); iter != listener_list.end(); ++iter) { if (local_name == (*iter)->local_name) { listener_list.erase(iter); @@ -145,7 +154,8 @@ InstallStatus remove_listener(const char* local_name, atransport* transport) { return INSTALL_STATUS_LISTENER_NOT_FOUND; } -void remove_all_listeners() { +void remove_all_listeners() EXCLUDES(listener_list_mutex) { + std::lock_guard lock(listener_list_mutex); auto iter = listener_list.begin(); while (iter != listener_list.end()) { // Never remove smart sockets. @@ -157,9 +167,18 @@ void remove_all_listeners() { } } +void close_smartsockets() EXCLUDES(listener_list_mutex) { + std::lock_guard lock(listener_list_mutex); + auto pred = [](const std::unique_ptr& listener) { + return listener->local_name == "*smartsocket*"; + }; + listener_list.erase(std::remove_if(listener_list.begin(), listener_list.end(), pred)); +} + InstallStatus install_listener(const std::string& local_name, const char* connect_to, atransport* transport, int no_rebind, int* resolved_tcp_port, - std::string* error) { + std::string* error) EXCLUDES(listener_list_mutex) { + std::lock_guard lock(listener_list_mutex); for (auto& l : listener_list) { if (local_name == l->local_name) { // Can't repurpose a smartsocket. @@ -212,7 +231,7 @@ InstallStatus install_listener(const std::string& local_name, const char* connec if (transport) { listener->disconnect.opaque = listener.get(); - listener->disconnect.func = listener_disconnect; + listener->disconnect.func = listener_disconnect; transport->AddDisconnect(&listener->disconnect); } diff --git a/adb/adb_listeners.h b/adb/adb_listeners.h index 8eba00a5e..70a2ee121 100644 --- a/adb/adb_listeners.h +++ b/adb/adb_listeners.h @@ -41,4 +41,6 @@ std::string format_listeners(); InstallStatus remove_listener(const char* local_name, atransport* transport); void remove_all_listeners(void); +void close_smartsockets(); + #endif /* __ADB_LISTENERS_H */ diff --git a/adb/client/main.cpp b/adb/client/main.cpp index fe5099c35..bd9ad01f3 100644 --- a/adb/client/main.cpp +++ b/adb/client/main.cpp @@ -92,6 +92,16 @@ static BOOL WINAPI ctrlc_handler(DWORD type) { } #endif +void adb_server_cleanup() { + // Upon exit, we want to clean up in the following order: + // 1. close_smartsockets, so that we don't get any new clients + // 2. kick_all_transports, to avoid writing only part of a packet to a transport. + // 3. usb_cleanup, to tear down the USB stack. + close_smartsockets(); + kick_all_transports(); + usb_cleanup(); +} + int adb_server_main(int is_daemon, const std::string& socket_spec, int ack_reply_fd) { #if defined(_WIN32) // adb start-server starts us up with stdout and stderr hooked up to @@ -111,12 +121,13 @@ int adb_server_main(int is_daemon, const std::string& socket_spec, int ack_reply SetConsoleCtrlHandler(ctrlc_handler, TRUE); #else signal(SIGINT, [](int) { - android::base::quick_exit(0); + fdevent_run_on_main_thread([]() { android::base::quick_exit(0); }); }); #endif - init_transport_registration(); + android::base::at_quick_exit(adb_server_cleanup); + init_transport_registration(); init_mdns_transport_discovery(); usb_init(); diff --git a/adb/client/usb_dispatch.cpp b/adb/client/usb_dispatch.cpp index 710a3ce85..c4eed7876 100644 --- a/adb/client/usb_dispatch.cpp +++ b/adb/client/usb_dispatch.cpp @@ -27,6 +27,12 @@ void usb_init() { } } +void usb_cleanup() { + if (should_use_libusb()) { + libusb::usb_cleanup(); + } +} + int usb_write(usb_handle* h, const void* data, int len) { return should_use_libusb() ? libusb::usb_write(reinterpret_cast(h), data, len) diff --git a/adb/client/usb_libusb.cpp b/adb/client/usb_libusb.cpp index 5a6586596..18a8ff23a 100644 --- a/adb/client/usb_libusb.cpp +++ b/adb/client/usb_libusb.cpp @@ -415,15 +415,21 @@ void usb_init() { // Spawn a thread to do device enumeration. // TODO: Use libusb_hotplug_* instead? + std::unique_lock lock(device_poll_mutex); device_poll_thread = new std::thread(poll_for_devices); - android::base::at_quick_exit([]() { - { - std::unique_lock lock(device_poll_mutex); - terminate_device_poll_thread = true; +} + +void usb_cleanup() { + { + std::unique_lock lock(device_poll_mutex); + terminate_device_poll_thread = true; + + if (!device_poll_thread) { + return; } - device_poll_cv.notify_all(); - device_poll_thread->join(); - }); + } + device_poll_cv.notify_all(); + device_poll_thread->join(); } // Dispatch a libusb transfer, unlock |device_lock|, and then wait for the result. diff --git a/adb/client/usb_windows.cpp b/adb/client/usb_windows.cpp index ec55b0e2a..1620e6ec4 100644 --- a/adb/client/usb_windows.cpp +++ b/adb/client/usb_windows.cpp @@ -265,6 +265,8 @@ void usb_init() { std::thread(_power_notification_thread).detach(); } +void usb_cleanup() {} + usb_handle* do_usb_open(const wchar_t* interface_name) { unsigned long name_len = 0; diff --git a/adb/transport.cpp b/adb/transport.cpp index 20de6421e..13cfaa1d4 100644 --- a/adb/transport.cpp +++ b/adb/transport.cpp @@ -568,15 +568,14 @@ void init_transport_registration(void) { transport_registration_func, 0); fdevent_set(&transport_registration_fde, FDE_READ); -#if ADB_HOST - android::base::at_quick_exit([]() { - // To avoid only writing part of a packet to a transport after exit, kick all transports. - std::lock_guard lock(transport_lock); - for (auto t : transport_list) { - t->Kick(); - } - }); -#endif +} + +void kick_all_transports() { + // To avoid only writing part of a packet to a transport after exit, kick all transports. + std::lock_guard lock(transport_lock); + for (auto t : transport_list) { + t->Kick(); + } } /* the fdevent select pump is single threaded */ diff --git a/adb/transport.h b/adb/transport.h index e129355ba..006aaf411 100644 --- a/adb/transport.h +++ b/adb/transport.h @@ -207,6 +207,7 @@ void init_mdns_transport_discovery(void); std::string list_transports(bool long_listing); atransport* find_transport(const char* serial); void kick_all_tcp_devices(); +void kick_all_transports(); void register_usb_transport(usb_handle* h, const char* serial, const char* devpath, unsigned writeable); diff --git a/adb/usb.h b/adb/usb.h index e867ec8a3..f428ede48 100644 --- a/adb/usb.h +++ b/adb/usb.h @@ -22,6 +22,7 @@ #define ADB_USB_INTERFACE(handle_ref_type) \ void usb_init(); \ + void usb_cleanup(); \ int usb_write(handle_ref_type h, const void* data, int len); \ int usb_read(handle_ref_type h, void* data, int len); \ int usb_close(handle_ref_type h); \