From 1a9bcde9d746a0bf53b928bd9c5a7c354fd87560 Mon Sep 17 00:00:00 2001 From: wwbmmm Date: Fri, 10 Jun 2022 14:58:26 +0800 Subject: [PATCH 1/2] Fix Acceptor::BeforeRecycle during Accept::StartAccept dead lock --- src/brpc/socket.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/brpc/socket.cpp b/src/brpc/socket.cpp index 59a570ac71..cdb51d1a33 100644 --- a/src/brpc/socket.cpp +++ b/src/brpc/socket.cpp @@ -651,6 +651,7 @@ int Socket::Create(const SocketOptions& options, SocketId* id) { // just after calling ResetFileDescriptor. if (m->ResetFileDescriptor(options.fd) != 0) { const int saved_errno = errno; + m->_user = NULL; // Avoid calling user->BeforeRecycle() which may cause dead lock PLOG(ERROR) << "Fail to ResetFileDescriptor"; m->SetFailed(saved_errno, "Fail to ResetFileDescriptor: %s", berror(saved_errno)); From a9b3aca2257cfb9a4939425d015b5c4694190c89 Mon Sep 17 00:00:00 2001 From: wwbmmm Date: Mon, 13 Jun 2022 14:47:55 +0800 Subject: [PATCH 2/2] Don't call _user->BeforeRecycle on Socket::Create --- src/brpc/socket.cpp | 18 +++++++++--------- src/brpc/socket.h | 3 +++ 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/src/brpc/socket.cpp b/src/brpc/socket.cpp index cdb51d1a33..8ce682889f 100644 --- a/src/brpc/socket.cpp +++ b/src/brpc/socket.cpp @@ -618,8 +618,7 @@ int Socket::Create(const SocketOptions& options, SocketId* id) { m->_auth_flag_error.store(0, butil::memory_order_relaxed); const int rc2 = bthread_id_create(&m->_auth_id, NULL, NULL); if (rc2) { - LOG(ERROR) << "Fail to create auth_id: " << berror(rc2); - m->SetFailed(rc2, "Fail to create auth_id: %s", berror(rc2)); + m->SetFailedOnCreate(rc2, "Fail to create auth_id"); return -1; } // Disable SSL check if there is no SSL context @@ -640,8 +639,7 @@ int Socket::Create(const SocketOptions& options, SocketId* id) { // NOTE: last two params are useless in bthread > r32787 const int rc = bthread_id_list_init(&m->_id_wait_list, 512, 512); if (rc) { - LOG(ERROR) << "Fail to init _id_wait_list: " << berror(rc); - m->SetFailed(rc, "Fail to init _id_wait_list: %s", berror(rc)); + m->SetFailedOnCreate(rc, "Fail to init _id_wait_list"); return -1; } m->_last_writetime_us.store(cpuwide_now, butil::memory_order_relaxed); @@ -650,11 +648,7 @@ int Socket::Create(const SocketOptions& options, SocketId* id) { // Must be last one! Internal fields of this Socket may be access // just after calling ResetFileDescriptor. if (m->ResetFileDescriptor(options.fd) != 0) { - const int saved_errno = errno; - m->_user = NULL; // Avoid calling user->BeforeRecycle() which may cause dead lock - PLOG(ERROR) << "Fail to ResetFileDescriptor"; - m->SetFailed(saved_errno, "Fail to ResetFileDescriptor: %s", - berror(saved_errno)); + m->SetFailedOnCreate(errno, "Fail to ResetFileDescriptor"); return -1; } *id = m->_this_id; @@ -876,6 +870,12 @@ int Socket::SetFailed() { return SetFailed(EFAILEDSOCKET, NULL); } +int Socket::SetFailedOnCreate(int error_code, const char* error_text) { + _user = NULL; // Avoid calling _user->BeforeRecycle() which may cause dead lock or double free + LOG(ERROR) << error_text << ": " << berror(error_code); + return SetFailed(error_code, "%s: %s", error_text, berror(error_code)); +} + void Socket::FeedbackCircuitBreaker(int error_code, int64_t latency_us) { if (!GetOrNewSharedPart()->circuit_breaker.OnCallEnd(error_code, latency_us)) { if (SetFailed(main_socket_id()) == 0) { diff --git a/src/brpc/socket.h b/src/brpc/socket.h index 4be6a73165..525c145abc 100644 --- a/src/brpc/socket.h +++ b/src/brpc/socket.h @@ -586,6 +586,9 @@ friend void DereferenceSocket(Socket*); int ResetFileDescriptor(int fd); + // Handle failure on Socket::Create + int SetFailedOnCreate(int error_code, const char* error_text); + // Wait until nref hits `expected_nref' and reset some internal resources. int WaitAndReset(int32_t expected_nref);