Revert "RPC Binder: dropping all binders drops session"
This reverts commit 75047ca4df8f6d88e16f131b2a8d54bfa013c2e6.
Reason for revert: See b/273486801. Build broken: branch git_master, target aosp_arm64-userdebug_tidy.
Change-Id: I79c9b003299b159dfdad18d70a5677e03e7da130
diff --git a/libs/binder/RpcState.cpp b/libs/binder/RpcState.cpp
index ed3ce24..38bd081 100644
--- a/libs/binder/RpcState.cpp
+++ b/libs/binder/RpcState.cpp
@@ -262,10 +262,8 @@
}
void RpcState::clear() {
- return clear(RpcMutexUniqueLock(mNodeMutex));
-}
+ RpcMutexUniqueLock _l(mNodeMutex);
-void RpcState::clear(RpcMutexUniqueLock nodeLock) {
if (mTerminated) {
LOG_ALWAYS_FATAL_IF(!mNodeForAddress.empty(),
"New state should be impossible after terminating!");
@@ -294,7 +292,7 @@
auto temp = std::move(mNodeForAddress);
mNodeForAddress.clear(); // RpcState isn't reusable, but for future/explicit
- nodeLock.unlock();
+ _l.unlock();
temp.clear(); // explicit
}
@@ -706,7 +704,7 @@
};
{
- RpcMutexUniqueLock _l(mNodeMutex);
+ RpcMutexLockGuard _l(mNodeMutex);
if (mTerminated) return DEAD_OBJECT; // avoid fatal only, otherwise races
auto it = mNodeForAddress.find(addr);
LOG_ALWAYS_FATAL_IF(it == mNodeForAddress.end(),
@@ -722,9 +720,8 @@
body.amount = it->second.timesRecd - target;
it->second.timesRecd = target;
- LOG_ALWAYS_FATAL_IF(nullptr != tryEraseNode(session, std::move(_l), it),
+ LOG_ALWAYS_FATAL_IF(nullptr != tryEraseNode(it),
"Bad state. RpcState shouldn't own received binder");
- // LOCK ALREADY RELEASED
}
RpcWireHeader cmd = {
@@ -1167,8 +1164,8 @@
it->second.timesSent);
it->second.timesSent -= body.amount;
- sp<IBinder> tempHold = tryEraseNode(session, std::move(_l), it);
- // LOCK ALREADY RELEASED
+ sp<IBinder> tempHold = tryEraseNode(it);
+ _l.unlock();
tempHold = nullptr; // destructor may make binder calls on this session
return OK;
@@ -1232,10 +1229,7 @@
return OK;
}
-sp<IBinder> RpcState::tryEraseNode(const sp<RpcSession>& session, RpcMutexUniqueLock nodeLock,
- std::map<uint64_t, BinderNode>::iterator& it) {
- bool shouldShutdown = false;
-
+sp<IBinder> RpcState::tryEraseNode(std::map<uint64_t, BinderNode>::iterator& it) {
sp<IBinder> ref;
if (it->second.timesSent == 0) {
@@ -1245,27 +1239,9 @@
LOG_ALWAYS_FATAL_IF(!it->second.asyncTodo.empty(),
"Can't delete binder w/ pending async transactions");
mNodeForAddress.erase(it);
-
- if (mNodeForAddress.size() == 0) {
- shouldShutdown = true;
- }
}
}
- // If we shutdown, prevent RpcState from being re-used. This prevents another
- // thread from getting the root object again.
- if (shouldShutdown) {
- clear(std::move(nodeLock));
- } else {
- nodeLock.unlock(); // explicit
- }
- // LOCK IS RELEASED
-
- if (shouldShutdown) {
- ALOGI("RpcState has no binders left, so triggering shutdown...");
- (void)session->shutdownAndWait(false);
- }
-
return ref;
}
diff --git a/libs/binder/RpcState.h b/libs/binder/RpcState.h
index 0e23ea7..ac86585 100644
--- a/libs/binder/RpcState.h
+++ b/libs/binder/RpcState.h
@@ -168,7 +168,6 @@
void clear();
private:
- void clear(RpcMutexUniqueLock nodeLock);
void dumpLocked();
// Alternative to std::vector<uint8_t> that doesn't abort on allocation failure and caps
@@ -269,20 +268,11 @@
std::string toString() const;
};
- // Checks if there is any reference left to a node and erases it. If this
- // is the last node, shuts down the session.
- //
- // Node lock is passed here for convenience, so that we can release it
- // and terminate the session, but we could leave it up to the caller
- // by returning a continuation if we needed to erase multiple specific
- // nodes. It may be tempting to allow the client to keep on holding the
- // lock and instead just return whether or not we should shutdown, but
- // this introduces the posssibility that another thread calls
- // getRootBinder and thinks it is valid, rather than immediately getting
- // an error.
- sp<IBinder> tryEraseNode(const sp<RpcSession>& session, RpcMutexUniqueLock nodeLock,
- std::map<uint64_t, BinderNode>::iterator& it);
-
+ // checks if there is any reference left to a node and erases it. If erase
+ // happens, and there is a strong reference to the binder kept by
+ // binderNode, this returns that strong reference, so that it can be
+ // dropped after any locks are removed.
+ sp<IBinder> tryEraseNode(std::map<uint64_t, BinderNode>::iterator& it);
// true - success
// false - session shutdown, halt
[[nodiscard]] bool nodeProgressAsyncNumber(BinderNode* node);
diff --git a/libs/binder/include/binder/RpcSession.h b/libs/binder/include/binder/RpcSession.h
index a323feb..0750ccf 100644
--- a/libs/binder/include/binder/RpcSession.h
+++ b/libs/binder/include/binder/RpcSession.h
@@ -51,9 +51,6 @@
* This represents a session (group of connections) between a client
* and a server. Multiple connections are needed for multiple parallel "binder"
* calls which may also have nested calls.
- *
- * Once a binder exists in the session, if all references to all binders are dropped,
- * the session shuts down.
*/
class RpcSession final : public virtual RefBase {
public:
diff --git a/libs/binder/tests/binderRpcBenchmark.cpp b/libs/binder/tests/binderRpcBenchmark.cpp
index 5939273..52ba9b0 100644
--- a/libs/binder/tests/binderRpcBenchmark.cpp
+++ b/libs/binder/tests/binderRpcBenchmark.cpp
@@ -102,11 +102,9 @@
}
static sp<RpcSession> gSession = RpcSession::make();
-static sp<IBinder> gRpcBinder;
// Certificate validation happens during handshake and does not affect the result of benchmarks.
// Skip certificate validation to simplify the setup process.
static sp<RpcSession> gSessionTls = RpcSession::make(makeFactoryTls());
-static sp<IBinder> gRpcTlsBinder;
#ifdef __BIONIC__
static const String16 kKernelBinderInstance = String16(u"binderRpcBenchmark-control");
static sp<IBinder> gKernelBinder;
@@ -120,9 +118,9 @@
return gKernelBinder;
#endif
case RPC:
- return gRpcBinder;
+ return gSession->getRootObject();
case RPC_TLS:
- return gRpcTlsBinder;
+ return gSessionTls->getRootObject();
default:
LOG(FATAL) << "Unknown transport value: " << transport;
return nullptr;
@@ -256,13 +254,11 @@
(void)unlink(addr.c_str());
forkRpcServer(addr.c_str(), RpcServer::make(RpcTransportCtxFactoryRaw::make()));
setupClient(gSession, addr.c_str());
- gRpcBinder = gSession->getRootObject();
std::string tlsAddr = tmp + "/binderRpcTlsBenchmark";
(void)unlink(tlsAddr.c_str());
forkRpcServer(tlsAddr.c_str(), RpcServer::make(makeFactoryTls()));
setupClient(gSessionTls, tlsAddr.c_str());
- gRpcTlsBinder = gSessionTls->getRootObject();
::benchmark::RunSpecifiedBenchmarks();
return 0;
diff --git a/libs/binder/tests/binderRpcTest.cpp b/libs/binder/tests/binderRpcTest.cpp
index 87c84ba..5952c41 100644
--- a/libs/binder/tests/binderRpcTest.cpp
+++ b/libs/binder/tests/binderRpcTest.cpp
@@ -163,8 +163,7 @@
session.root = nullptr;
}
- for (size_t sessionNum = 0; sessionNum < sessions.size(); sessionNum++) {
- auto& info = sessions.at(sessionNum);
+ for (auto& info : sessions) {
sp<RpcSession>& session = info.session;
EXPECT_NE(nullptr, session);
@@ -180,7 +179,6 @@
for (size_t i = 0; i < 3; i++) {
sp<RpcSession> strongSession = weakSession.promote();
EXPECT_EQ(nullptr, strongSession)
- << "For session " << sessionNum << ". "
<< (debugBacktrace(host.getPid()), debugBacktrace(getpid()),
"Leaked sess: ")
<< strongSession->getStrongCount() << " checked time " << i;
@@ -256,10 +254,6 @@
const BinderRpcOptions& options) {
CHECK_GE(options.numSessions, 1) << "Must have at least one session to a server";
- if (options.numIncomingConnectionsBySession.size() != 0) {
- CHECK_EQ(options.numIncomingConnectionsBySession.size(), options.numSessions);
- }
-
SocketType socketType = std::get<0>(GetParam());
RpcSecurity rpcSecurity = std::get<1>(GetParam());
uint32_t clientVersion = std::get<2>(GetParam());
@@ -357,15 +351,9 @@
status_t status;
- for (size_t i = 0; i < sessions.size(); i++) {
- const auto& session = sessions.at(i);
-
- size_t numIncoming = options.numIncomingConnectionsBySession.size() > 0
- ? options.numIncomingConnectionsBySession.at(i)
- : 0;
-
+ for (const auto& session : sessions) {
CHECK(session->setProtocolVersion(clientVersion));
- session->setMaxIncomingThreads(numIncoming);
+ session->setMaxIncomingThreads(options.numIncomingConnections);
session->setMaxOutgoingConnections(options.numOutgoingConnections);
session->setFileDescriptorTransportMode(options.clientFileDescriptorTransportMode);
@@ -671,32 +659,6 @@
proc.proc->sessions.erase(proc.proc->sessions.begin() + 1);
}
-TEST_P(BinderRpc, SessionWithIncomingThreadpoolDoesntLeak) {
- if (clientOrServerSingleThreaded()) {
- GTEST_SKIP() << "This test requires multiple threads";
- }
-
- // session 0 - will check for leaks in destrutor of proc
- // session 1 - we want to make sure it gets deleted when we drop all references to it
- auto proc = createRpcTestSocketServerProcess(
- {.numThreads = 1, .numIncomingConnectionsBySession = {0, 1}, .numSessions = 2});
-
- wp<RpcSession> session = proc.proc->sessions.at(1).session;
-
- // remove all references to the second session
- proc.proc->sessions.at(1).root = nullptr;
- proc.proc->sessions.erase(proc.proc->sessions.begin() + 1);
-
- // TODO(b/271830568) more efficient way to wait for other incoming threadpool
- // to drain commands.
- for (size_t i = 0; i < 100; i++) {
- usleep(10 * 1000);
- if (session.promote() == nullptr) break;
- }
-
- EXPECT_EQ(nullptr, session.promote());
-}
-
TEST_P(BinderRpc, SingleDeathRecipient) {
if (clientOrServerSingleThreaded()) {
GTEST_SKIP() << "This test requires multiple threads";
@@ -714,7 +676,7 @@
// Death recipient needs to have an incoming connection to be called
auto proc = createRpcTestSocketServerProcess(
- {.numThreads = 1, .numSessions = 1, .numIncomingConnectionsBySession = {1}});
+ {.numThreads = 1, .numSessions = 1, .numIncomingConnections = 1});
auto dr = sp<MyDeathRec>::make();
ASSERT_EQ(OK, proc.rootBinder->linkToDeath(dr, (void*)1, 0));
@@ -727,10 +689,6 @@
ASSERT_TRUE(dr->mCv.wait_for(lock, 100ms, [&]() { return dr->dead; }));
// need to wait for the session to shutdown so we don't "Leak session"
- // can't do this before checking the death recipient by calling
- // forceShutdown earlier, because shutdownAndWait will also trigger
- // a death recipient, but if we had a way to wait for the service
- // to gracefully shutdown, we could use that here.
EXPECT_TRUE(proc.proc->sessions.at(0).session->shutdownAndWait(true));
proc.expectAlreadyShutdown = true;
}
@@ -752,7 +710,7 @@
// Death recipient needs to have an incoming connection to be called
auto proc = createRpcTestSocketServerProcess(
- {.numThreads = 1, .numSessions = 1, .numIncomingConnectionsBySession = {1}});
+ {.numThreads = 1, .numSessions = 1, .numIncomingConnections = 1});
auto dr = sp<MyDeathRec>::make();
EXPECT_EQ(OK, proc.rootBinder->linkToDeath(dr, (void*)1, 0));
@@ -785,7 +743,8 @@
void binderDied(const wp<IBinder>& /* who */) override {}
};
- auto proc = createRpcTestSocketServerProcess({.numThreads = 1, .numSessions = 1});
+ auto proc = createRpcTestSocketServerProcess(
+ {.numThreads = 1, .numSessions = 1, .numIncomingConnections = 0});
auto dr = sp<MyDeathRec>::make();
EXPECT_EQ(INVALID_OPERATION, proc.rootBinder->linkToDeath(dr, (void*)1, 0));
@@ -804,13 +763,19 @@
// Death recipient needs to have an incoming connection to be called
auto proc = createRpcTestSocketServerProcess(
- {.numThreads = 1, .numSessions = 1, .numIncomingConnectionsBySession = {1}});
+ {.numThreads = 1, .numSessions = 1, .numIncomingConnections = 1});
auto dr = sp<MyDeathRec>::make();
ASSERT_EQ(OK, proc.rootBinder->linkToDeath(dr, (void*)1, 0));
ASSERT_EQ(OK, proc.rootBinder->unlinkToDeath(dr, (void*)1, 0, nullptr));
- proc.forceShutdown();
+ if (auto status = proc.rootIface->scheduleShutdown(); !status.isOk()) {
+ EXPECT_EQ(DEAD_OBJECT, status.transactionError()) << status;
+ }
+
+ // need to wait for the session to shutdown so we don't "Leak session"
+ EXPECT_TRUE(proc.proc->sessions.at(0).session->shutdownAndWait(true));
+ proc.expectAlreadyShutdown = true;
}
TEST_P(BinderRpc, Die) {
diff --git a/libs/binder/tests/binderRpcTestCommon.h b/libs/binder/tests/binderRpcTestCommon.h
index 37c43f5..a467ee3 100644
--- a/libs/binder/tests/binderRpcTestCommon.h
+++ b/libs/binder/tests/binderRpcTestCommon.h
@@ -126,11 +126,7 @@
struct BinderRpcOptions {
size_t numThreads = 1;
size_t numSessions = 1;
- // right now, this can be empty, or length numSessions, where each value
- // represents the info for the corresponding session, but we should
- // probably switch this to be a list of sessions options so that other
- // options can all be specified per session
- std::vector<size_t> numIncomingConnectionsBySession = {};
+ size_t numIncomingConnections = 0;
size_t numOutgoingConnections = SIZE_MAX;
RpcSession::FileDescriptorTransportMode clientFileDescriptorTransportMode =
RpcSession::FileDescriptorTransportMode::NONE;
diff --git a/libs/binder/tests/binderRpcTestFixture.h b/libs/binder/tests/binderRpcTestFixture.h
index 20fb6bf..c99d68a 100644
--- a/libs/binder/tests/binderRpcTestFixture.h
+++ b/libs/binder/tests/binderRpcTestFixture.h
@@ -64,21 +64,6 @@
// whether session should be invalidated by end of run
bool expectAlreadyShutdown = false;
- // TODO(b/271830568): fix this in binderRpcTest, we always use the first session to cause the
- // remote process to shutdown. Normally, when we shutdown, the default in the destructor is to
- // check that there are no leaks and shutdown. However, when there are incoming threadpools,
- // there will be a few extra binder threads there, so we can't shutdown the server. We should
- // consider an alternative way of doing the test so that we don't need this, some ideas, such as
- // program in understanding of incoming threadpool into the destructor so that (e.g.
- // intelligently wait for sessions to shutdown now that they will do this)
- void forceShutdown() {
- if (auto status = rootIface->scheduleShutdown(); !status.isOk()) {
- EXPECT_EQ(DEAD_OBJECT, status.transactionError()) << status;
- }
- EXPECT_TRUE(proc->sessions.at(0).session->shutdownAndWait(true));
- expectAlreadyShutdown = true;
- }
-
BinderRpcTestProcessSession(BinderRpcTestProcessSession&&) = default;
~BinderRpcTestProcessSession() {
if (!expectAlreadyShutdown) {
diff --git a/libs/binder/tests/binderRpcTestTrusty.cpp b/libs/binder/tests/binderRpcTestTrusty.cpp
index 85794bd..63b56a3 100644
--- a/libs/binder/tests/binderRpcTestTrusty.cpp
+++ b/libs/binder/tests/binderRpcTestTrusty.cpp
@@ -60,9 +60,9 @@
// threads.
std::unique_ptr<ProcessSession> BinderRpc::createRpcTestSocketServerProcessEtc(
const BinderRpcOptions& options) {
- LOG_ALWAYS_FATAL_IF(options.numIncomingConnectionsBySession.size() != 0,
+ LOG_ALWAYS_FATAL_IF(options.numIncomingConnections != 0,
"Non-zero incoming connections %zu on Trusty",
- options.numIncomingConnectionsBySession.size());
+ options.numIncomingConnections);
uint32_t clientVersion = std::get<2>(GetParam());
uint32_t serverVersion = std::get<3>(GetParam());
diff --git a/libs/binder/tests/binderRpcUniversalTests.cpp b/libs/binder/tests/binderRpcUniversalTests.cpp
index 1f46010..11a22b0 100644
--- a/libs/binder/tests/binderRpcUniversalTests.cpp
+++ b/libs/binder/tests/binderRpcUniversalTests.cpp
@@ -463,7 +463,7 @@
auto proc = createRpcTestSocketServerProcess(
{.numThreads = 1,
.numSessions = 1,
- .numIncomingConnectionsBySession = {numIncomingConnections}});
+ .numIncomingConnections = numIncomingConnections});
auto cb = sp<MyBinderRpcCallback>::make();
if (callIsOneway) {
@@ -491,7 +491,16 @@
<< "callIsOneway: " << callIsOneway
<< " callbackIsOneway: " << callbackIsOneway << " delayed: " << delayed;
- proc.forceShutdown();
+ // since we are severing the connection, we need to go ahead and
+ // tell the server to shutdown and exit so that waitpid won't hang
+ if (auto status = proc.rootIface->scheduleShutdown(); !status.isOk()) {
+ EXPECT_EQ(DEAD_OBJECT, status.transactionError()) << status;
+ }
+
+ // since this session has an incoming connection w/ a threadpool, we
+ // need to manually shut it down
+ EXPECT_TRUE(proc.proc->sessions.at(0).session->shutdownAndWait(true));
+ proc.expectAlreadyShutdown = true;
}
}
}