[adb data server] wait for installation results before terminates
Currently the server often quits before installation finishes. As a
result, there is no difference in the commandline output between a
successful installation and a failed one.
Let adb client wait till installation fails or succeeds by parsing the
output from the inc-server process.
Test: $ adb install --incremental ~/Downloads/base.apk
Test: Performing Incremental Install
Test: Serving...
Test: All files should be loaded. Notifying the device.
Test: Failure [INSTALL_PARSE_FAILED_NOT_APK: Failed to parse /data/app/vmdl749343150.tmp/base.apk: Failed to load asset path /data/app/vmdl749343150.tmp/base.apk]
Test: Install command complete (ms: 91 total, 0 apk prep, 91 install)
BUG: b/150865433
Change-Id: Ie33505f9cc08fc6d60ad4a5d709526e7aa9a0ad1
Merged-In: Ie33505f9cc08fc6d60ad4a5d709526e7aa9a0ad1
diff --git a/client/commandline.cpp b/client/commandline.cpp
index 7c7da08..f154d70 100644
--- a/client/commandline.cpp
+++ b/client/commandline.cpp
@@ -1443,6 +1443,26 @@
#endif
}
+static bool _is_valid_fd(int fd) {
+ // Disallow invalid FDs and stdin/out/err as well.
+ if (fd < 3) {
+ return false;
+ }
+#ifdef _WIN32
+ HANDLE handle = adb_get_os_handle(fd);
+ DWORD info = 0;
+ if (GetHandleInformation(handle, &info) == 0) {
+ return false;
+ }
+#else
+ int flags = fcntl(fd, F_GETFD);
+ if (flags == -1) {
+ return false;
+ }
+#endif
+ return true;
+}
+
int adb_commandline(int argc, const char** argv) {
bool no_daemon = false;
bool is_daemon = false;
@@ -2009,17 +2029,28 @@
}
}
} else if (!strcmp(argv[0], "inc-server")) {
- if (argc < 3) {
- error_exit("usage: adb inc-server FD FILE1 FILE2 ...");
+ if (argc < 4) {
+#ifdef _WIN32
+ error_exit("usage: adb inc-server CONNECTION_HANDLE OUTPUT_HANDLE FILE1 FILE2 ...");
+#else
+ error_exit("usage: adb inc-server CONNECTION_FD OUTPUT_FD FILE1 FILE2 ...");
+#endif
}
- int fd = atoi(argv[1]);
- if (fd < 3) {
- // Disallow invalid FDs and stdin/out/err as well.
- error_exit("Invalid fd number given: %d", fd);
+ int connection_fd = atoi(argv[1]);
+ if (!_is_valid_fd(connection_fd)) {
+ error_exit("Invalid connection_fd number given: %d", connection_fd);
}
- fd = adb_register_socket(fd);
- close_on_exec(fd);
- return incremental::serve(fd, argc - 2, argv + 2);
+
+ connection_fd = adb_register_socket(connection_fd);
+ close_on_exec(connection_fd);
+
+ int output_fd = atoi(argv[2]);
+ if (!_is_valid_fd(output_fd)) {
+ error_exit("Invalid output_fd number given: %d", output_fd);
+ }
+ output_fd = adb_register_socket(output_fd);
+ close_on_exec(output_fd);
+ return incremental::serve(connection_fd, output_fd, argc - 3, argv + 3);
}
error_exit("unknown command %s", argv[0]);
diff --git a/client/incremental.cpp b/client/incremental.cpp
index 3ceb374..fd608cc 100644
--- a/client/incremental.cpp
+++ b/client/incremental.cpp
@@ -193,20 +193,72 @@
auto fd_param = std::to_string(osh);
#endif
+ // pipe for child process to write output
+ int print_fds[2];
+ if (adb_socketpair(print_fds) != 0) {
+ fprintf(stderr, "Failed to create socket pair for child to print to parent\n");
+ return {};
+ }
+ auto [pipe_read_fd, pipe_write_fd] = print_fds;
+ auto pipe_write_fd_param = std::to_string(pipe_write_fd);
+ close_on_exec(pipe_read_fd);
+
std::vector<std::string> args(std::move(files));
- args.insert(args.begin(), {"inc-server", fd_param});
- auto child = adb_launch_process(adb_path, std::move(args), {connection_fd.get()});
+ args.insert(args.begin(), {"inc-server", fd_param, pipe_write_fd_param});
+ auto child =
+ adb_launch_process(adb_path, std::move(args), {connection_fd.get(), pipe_write_fd});
if (!child) {
fprintf(stderr, "adb: failed to fork: %s\n", strerror(errno));
return {};
}
+ adb_close(pipe_write_fd);
+
auto killOnExit = [](Process* p) { p->kill(); };
std::unique_ptr<Process, decltype(killOnExit)> serverKiller(&child, killOnExit);
- // TODO: Terminate server process if installation fails.
- serverKiller.release();
+ Result result = wait_for_installation(pipe_read_fd);
+ adb_close(pipe_read_fd);
+
+ if (result == Result::Success) {
+ // adb client exits now but inc-server can continue
+ serverKiller.release();
+ }
return child;
}
+Result wait_for_installation(int read_fd) {
+ static constexpr int maxMessageSize = 256;
+ std::vector<char> child_stdout(CHUNK_SIZE);
+ int bytes_read;
+ int buf_size = 0;
+ // TODO(b/150865433): optimize child's output parsing
+ while ((bytes_read = adb_read(read_fd, child_stdout.data() + buf_size,
+ child_stdout.size() - buf_size)) > 0) {
+ // print to parent's stdout
+ fprintf(stdout, "%.*s", bytes_read, child_stdout.data() + buf_size);
+
+ buf_size += bytes_read;
+ const std::string_view stdout_str(child_stdout.data(), buf_size);
+ // wait till installation either succeeds or fails
+ if (stdout_str.find("Success") != std::string::npos) {
+ return Result::Success;
+ }
+ // on failure, wait for full message
+ static constexpr auto failure_msg_head = "Failure ["sv;
+ if (const auto begin_itr = stdout_str.find(failure_msg_head);
+ begin_itr != std::string::npos) {
+ if (buf_size >= maxMessageSize) {
+ return Result::Failure;
+ }
+ const auto end_itr = stdout_str.rfind("]");
+ if (end_itr != std::string::npos && end_itr >= begin_itr + failure_msg_head.size()) {
+ return Result::Failure;
+ }
+ }
+ child_stdout.resize(buf_size + CHUNK_SIZE);
+ }
+ return Result::None;
+}
+
} // namespace incremental
diff --git a/client/incremental.h b/client/incremental.h
index 4b9f6bd..731e6fb 100644
--- a/client/incremental.h
+++ b/client/incremental.h
@@ -27,4 +27,7 @@
std::optional<Process> install(std::vector<std::string> files);
+enum class Result { Success, Failure, None };
+Result wait_for_installation(int read_fd);
+
} // namespace incremental
diff --git a/client/incremental_server.cpp b/client/incremental_server.cpp
index 2512d05..6c36a0f 100644
--- a/client/incremental_server.cpp
+++ b/client/incremental_server.cpp
@@ -18,13 +18,6 @@
#include "incremental_server.h"
-#include "adb.h"
-#include "adb_io.h"
-#include "adb_trace.h"
-#include "adb_unique_fd.h"
-#include "adb_utils.h"
-#include "sysdeps.h"
-
#include <android-base/endian.h>
#include <android-base/strings.h>
#include <inttypes.h>
@@ -41,6 +34,13 @@
#include <type_traits>
#include <unordered_set>
+#include "adb.h"
+#include "adb_io.h"
+#include "adb_trace.h"
+#include "adb_unique_fd.h"
+#include "adb_utils.h"
+#include "sysdeps.h"
+
namespace incremental {
static constexpr int kBlockSize = 4096;
@@ -49,6 +49,7 @@
static constexpr short kCompressionLZ4 = 1;
static constexpr int kCompressBound = std::max(kBlockSize, LZ4_COMPRESSBOUND(kBlockSize));
static constexpr auto kReadBufferSize = 128 * 1024;
+static constexpr int kPollTimeoutMillis = 300000; // 5 minutes
using BlockSize = int16_t;
using FileId = int16_t;
@@ -61,9 +62,10 @@
static constexpr MagicType INCR = 0x494e4352; // LE INCR
-static constexpr RequestType EXIT = 0;
+static constexpr RequestType SERVING_COMPLETE = 0;
static constexpr RequestType BLOCK_MISSING = 1;
static constexpr RequestType PREFETCH = 2;
+static constexpr RequestType DESTROY = 3;
static constexpr inline int64_t roundDownToBlockOffset(int64_t val) {
return val & ~(kBlockSize - 1);
@@ -162,8 +164,8 @@
class IncrementalServer {
public:
- IncrementalServer(unique_fd fd, std::vector<File> files)
- : adb_fd_(std::move(fd)), files_(std::move(files)) {
+ IncrementalServer(unique_fd adb_fd, unique_fd output_fd, std::vector<File> files)
+ : adb_fd_(std::move(adb_fd)), output_fd_(std::move(output_fd)), files_(std::move(files)) {
buffer_.reserve(kReadBufferSize);
}
@@ -197,9 +199,10 @@
void Send(const void* data, size_t size, bool flush);
void Flush();
using TimePoint = decltype(std::chrono::high_resolution_clock::now());
- bool Exit(std::optional<TimePoint> startTime, int missesCount, int missesSent);
+ bool ServingComplete(std::optional<TimePoint> startTime, int missesCount, int missesSent);
unique_fd const adb_fd_;
+ unique_fd const output_fd_;
std::vector<File> files_;
// Incoming data buffer.
@@ -210,6 +213,9 @@
long long sentSize_ = 0;
std::vector<char> pendingBlocks_;
+
+ // True when client notifies that all the data has been received
+ bool servingComplete_;
};
bool IncrementalServer::SkipToRequest(void* buffer, size_t* size, bool blocking) {
@@ -217,7 +223,8 @@
// Looking for INCR magic.
bool magic_found = false;
int bcur = 0;
- for (int bsize = buffer_.size(); bcur + 4 < bsize; ++bcur) {
+ int bsize = buffer_.size();
+ for (bcur = 0; bcur + 4 < bsize; ++bcur) {
uint32_t magic = be32toh(*(uint32_t*)(buffer_.data() + bcur));
if (magic == INCR) {
magic_found = true;
@@ -226,8 +233,8 @@
}
if (bcur > 0) {
- // Stream the rest to stderr.
- fprintf(stderr, "%.*s", bcur, buffer_.data());
+ // output the rest.
+ WriteFdExactly(output_fd_, buffer_.data(), bcur);
erase_buffer_head(bcur);
}
@@ -239,17 +246,26 @@
}
adb_pollfd pfd = {adb_fd_.get(), POLLIN, 0};
- auto res = adb_poll(&pfd, 1, blocking ? -1 : 0);
+ auto res = adb_poll(&pfd, 1, blocking ? kPollTimeoutMillis : 0);
+
if (res != 1) {
+ WriteFdExactly(output_fd_, buffer_.data(), buffer_.size());
if (res < 0) {
- fprintf(stderr, "Failed to poll: %s\n", strerror(errno));
+ D("Failed to poll: %s\n", strerror(errno));
+ return false;
+ }
+ if (blocking) {
+ fprintf(stderr, "Timed out waiting for data from device.\n");
+ }
+ if (blocking && servingComplete_) {
+ // timeout waiting from client. Serving is complete, so quit.
return false;
}
*size = 0;
return true;
}
- auto bsize = buffer_.size();
+ bsize = buffer_.size();
buffer_.resize(kReadBufferSize);
int r = adb_read(adb_fd_, buffer_.data() + bsize, kReadBufferSize - bsize);
if (r > 0) {
@@ -257,21 +273,19 @@
continue;
}
- if (r == -1) {
- fprintf(stderr, "Failed to read from fd %d: %d. Exit\n", adb_fd_.get(), errno);
- return false;
- }
-
- // socket is closed
- return false;
+ D("Failed to read from fd %d: %d. Exit\n", adb_fd_.get(), errno);
+ break;
}
+ // socket is closed. print remaining messages
+ WriteFdExactly(output_fd_, buffer_.data(), buffer_.size());
+ return false;
}
std::optional<RequestCommand> IncrementalServer::ReadRequest(bool blocking) {
uint8_t commandBuf[sizeof(RequestCommand)];
auto size = sizeof(commandBuf);
if (!SkipToRequest(&commandBuf, &size, blocking)) {
- return {{EXIT}};
+ return {{DESTROY}};
}
if (size < sizeof(RequestCommand)) {
return {};
@@ -391,17 +405,17 @@
pendingBlocks_.clear();
}
-bool IncrementalServer::Exit(std::optional<TimePoint> startTime, int missesCount, int missesSent) {
+bool IncrementalServer::ServingComplete(std::optional<TimePoint> startTime, int missesCount,
+ int missesSent) {
+ servingComplete_ = true;
using namespace std::chrono;
auto endTime = high_resolution_clock::now();
- fprintf(stderr,
- "Connection failed or received exit command. Exit.\n"
- "Misses: %d, of those unique: %d; sent compressed: %d, uncompressed: "
- "%d, mb: %.3f\n"
- "Total time taken: %.3fms\n",
- missesCount, missesSent, compressed_, uncompressed_, sentSize_ / 1024.0 / 1024.0,
- duration_cast<microseconds>(endTime - (startTime ? *startTime : endTime)).count() /
- 1000.0);
+ D("Streaming completed.\n"
+ "Misses: %d, of those unique: %d; sent compressed: %d, uncompressed: "
+ "%d, mb: %.3f\n"
+ "Total time taken: %.3fms\n",
+ missesCount, missesSent, compressed_, uncompressed_, sentSize_ / 1024.0 / 1024.0,
+ duration_cast<microseconds>(endTime - (startTime ? *startTime : endTime)).count() / 1000.0);
return true;
}
@@ -425,7 +439,7 @@
std::all_of(files_.begin(), files_.end(), [](const File& f) {
return f.sentBlocksCount == NumBlocks(f.sentBlocks.size());
})) {
- fprintf(stdout, "All files should be loaded. Notifying the device.\n");
+ fprintf(stderr, "All files should be loaded. Notifying the device.\n");
SendDone();
doneSent = true;
}
@@ -446,9 +460,14 @@
BlockIdx blockIdx = request->block_idx;
switch (request->request_type) {
- case EXIT: {
+ case DESTROY: {
// Stop everything.
- return Exit(startTime, missesCount, missesSent);
+ return true;
+ }
+ case SERVING_COMPLETE: {
+ // Not stopping the server here.
+ ServingComplete(startTime, missesCount, missesSent);
+ break;
}
case BLOCK_MISSING: {
++missesCount;
@@ -502,8 +521,9 @@
}
}
-bool serve(int adb_fd, int argc, const char** argv) {
- auto connection_fd = unique_fd(adb_fd);
+bool serve(int connection_fd, int output_fd, int argc, const char** argv) {
+ auto connection_ufd = unique_fd(connection_fd);
+ auto output_ufd = unique_fd(output_fd);
if (argc <= 0) {
error_exit("inc-server: must specify at least one file.");
}
@@ -526,7 +546,7 @@
files.emplace_back(filepath, i, st.st_size, std::move(fd));
}
- IncrementalServer server(std::move(connection_fd), std::move(files));
+ IncrementalServer server(std::move(connection_ufd), std::move(output_ufd), std::move(files));
printf("Serving...\n");
fclose(stdin);
fclose(stdout);
diff --git a/client/incremental_server.h b/client/incremental_server.h
index 53f011e..55b8215 100644
--- a/client/incremental_server.h
+++ b/client/incremental_server.h
@@ -21,6 +21,6 @@
// Expecting arguments like:
// {FILE1 FILE2 ...}
// Where FILE* are files to serve.
-bool serve(int adbFd, int argc, const char** argv);
+bool serve(int connection_fd, int output_fd, int argc, const char** argv);
} // namespace incremental
