From 42436ae09d97720d88e01d759b64cd83afcaee08 Mon Sep 17 00:00:00 2001 From: Rahman Abber Tahir Date: Wed, 22 Nov 2023 18:56:05 +0100 Subject: [PATCH 1/8] add hard-code deadline for refelection stream. Signed-off-by: Rahman Abber Tahir --- src/libCli/Call.cpp | 9 +++------ src/libCli/ConnectionManager.cpp | 5 ++--- src/libCli/libCli/ConnectionManager.hpp | 6 ++---- src/libLocalDescriptorCache/DescDbProxy.cpp | 5 ++--- src/libLocalDescriptorCache/DescDbProxy.hpp | 5 ++--- .../proto_reflection_descriptor_database.h | 5 ++--- .../proto_reflection_descriptor_database.cc | 14 +++++++++----- 7 files changed, 22 insertions(+), 27 deletions(-) diff --git a/src/libCli/Call.cpp b/src/libCli/Call.cpp index 287b68f..2f898ea 100644 --- a/src/libCli/Call.cpp +++ b/src/libCli/Call.cpp @@ -191,14 +191,11 @@ namespace cli } //before calling the RPC, close the DescDb connection with a timeout. - grpc::Status dbDescStatus = ConnectionManager::getInstance().closeDescDbWithDeadline(serverAddress, deadline); + grpc::Status dbDescStatus = ConnectionManager::getInstance().closeDescDbStream(serverAddress); if (not dbDescStatus.ok()) { - std::cerr << "Failed to close reflection stream ;( Status code: " << std::to_string(dbDescStatus.error_code()) << " " << cli::getGrpcStatusCodeAsString(dbDescStatus.error_code()) << ", error message: " << dbDescStatus.error_message() << std::endl; - if(dbDescStatus.error_code() == grpc::StatusCode::DEADLINE_EXCEEDED) - { - std::cerr << "Note: You can increase the deadline by setting the --rpcTimeoutMilliseconds option to a number or 'None'." << std::endl; - } + std::cerr << "Failed to close reflection stream ;( Pls try again." << std::endl; + std::cerr << "Status code: " << std::to_string(dbDescStatus.error_code()) << " " << cli::getGrpcStatusCodeAsString(dbDescStatus.error_code()) << ", error message: " << dbDescStatus.error_message() << std::endl; return -1; } diff --git a/src/libCli/ConnectionManager.cpp b/src/libCli/ConnectionManager.cpp index 5e1c084..350ef25 100644 --- a/src/libCli/ConnectionManager.cpp +++ b/src/libCli/ConnectionManager.cpp @@ -65,8 +65,7 @@ namespace cli return m_connections[f_serverAddress].descPool; } - grpc::Status ConnectionManager::closeDescDbWithDeadline(std::string f_serverAddress, - std::optional> deadline) + grpc::Status ConnectionManager::closeDescDbStream(std::string f_serverAddress) { if (m_connections[f_serverAddress].descDbProxy == nullptr) { @@ -75,7 +74,7 @@ namespace cli } //if proxy exists close the stream with a deadline. - grpc::Status status = m_connections[f_serverAddress].descDbProxy->closeDescDbStream(deadline); + grpc::Status status = m_connections[f_serverAddress].descDbProxy->closeDescDbStream(); //delete the proxy, findChannelByAddress() protects from accessing uninitialzed DbProxy. m_connections[f_serverAddress].descDbProxy.reset(); diff --git a/src/libCli/libCli/ConnectionManager.hpp b/src/libCli/libCli/ConnectionManager.hpp index 2515af3..19e8539 100644 --- a/src/libCli/libCli/ConnectionManager.hpp +++ b/src/libCli/libCli/ConnectionManager.hpp @@ -52,13 +52,11 @@ namespace cli /// @returns the gRpc DescriptorPool of the corresponding server address. std::shared_ptr getDescPool(std::string f_serverAddress, ArgParse::ParsedElement &f_parseTree); - /// @brief closes the DescDb stream with a given deadline. + /// @brief closes the DescDb stream with a default deadline. /// @param f_serverAddress server addresss to lookup the assigned DescDbProxy. - /// @param deadline optional dealine for closing the stream. /// @return returns grpc::StatusCode::ABORTED status if no DescDb proxy is attached to the server address, /// otherwise grpc status as a result of stream closure. - grpc::Status closeDescDbWithDeadline(std::string f_serverAddress, - std::optional> deadline); + grpc::Status closeDescDbStream(std::string f_serverAddress); private: ConnectionManager() {} diff --git a/src/libLocalDescriptorCache/DescDbProxy.cpp b/src/libLocalDescriptorCache/DescDbProxy.cpp index 4afbfbb..66baed1 100644 --- a/src/libLocalDescriptorCache/DescDbProxy.cpp +++ b/src/libLocalDescriptorCache/DescDbProxy.cpp @@ -372,13 +372,13 @@ void DescDbProxy::getDescriptors(const std::string &f_hostAddress) } } -grpc::Status DescDbProxy::closeDescDbStream(std::optional> deadline) +grpc::Status DescDbProxy::closeDescDbStream() { if ( m_reflectionDescDb == nullptr ) { return grpc::Status::OK; } - return m_reflectionDescDb->closeStreamWithDeadline(deadline); + return m_reflectionDescDb->closeDescDbStream(); } DescDbProxy::DescDbProxy(bool disableCache, const std::string &hostAddress, std::shared_ptr channel, @@ -386,7 +386,6 @@ DescDbProxy::DescDbProxy(bool disableCache, const std::string &hostAddress, std: { m_channel = channel; m_parseTree = parseTree; - m_disableCache = disableCache; if(disableCache) { // Get Desc directly via reflection and without touching localDB diff --git a/src/libLocalDescriptorCache/DescDbProxy.hpp b/src/libLocalDescriptorCache/DescDbProxy.hpp index f8652a9..674bd04 100644 --- a/src/libLocalDescriptorCache/DescDbProxy.hpp +++ b/src/libLocalDescriptorCache/DescDbProxy.hpp @@ -58,10 +58,9 @@ class DescDbProxy : public grpc::protobuf::DescriptorDatabase{ /// @param hostAdress Address to the current host void getDescriptors(const std::string &hostAddress); - /// @brief close the DescDb stream with a given deadline. If the dealine is not set it waits for the stream to close indefinitely. - /// @param deadline optional deadline to close the DescDb stream. + /// @brief close the DescDb stream with a default deadline. /// @return return grpc status as a result of call the finish() on the DescDb stream. - grpc::Status closeDescDbStream(std::optional> deadline); + grpc::Status closeDescDbStream(); DescDbProxy(bool disableCache, const std::string &hostAddress, std::shared_ptr channel, ArgParse::ParsedElement &parseTree); diff --git a/third_party/gRPC_utils/gRPC_utils/proto_reflection_descriptor_database.h b/third_party/gRPC_utils/gRPC_utils/proto_reflection_descriptor_database.h index 41cdf22..1e758cb 100644 --- a/third_party/gRPC_utils/gRPC_utils/proto_reflection_descriptor_database.h +++ b/third_party/gRPC_utils/gRPC_utils/proto_reflection_descriptor_database.h @@ -82,10 +82,9 @@ class ProtoReflectionDescriptorDatabase : public protobuf::DescriptorDatabase { // Provide a list of full names of registered services bool GetServices(std::vector* output); - /// @brief close the reflection stream with a given deadline. If the dealine is not set it waits for the stream to close indefinitely. - /// @param deadline optional deadline to close the reflection stream. + /// @brief close the reflection stream with a default deadline. /// @return return grpc status as a result of call the finish() on the reflection stream. - grpc::Status closeStreamWithDeadline(std::optional> deadline); + grpc::Status closeDescDbStream(); private: typedef ClientReaderWriter< diff --git a/third_party/gRPC_utils/proto_reflection_descriptor_database.cc b/third_party/gRPC_utils/proto_reflection_descriptor_database.cc index ed374c0..83f8943 100644 --- a/third_party/gRPC_utils/proto_reflection_descriptor_database.cc +++ b/third_party/gRPC_utils/proto_reflection_descriptor_database.cc @@ -32,6 +32,7 @@ using grpc::reflection::v1alpha::ServerReflection; using grpc::reflection::v1alpha::ServerReflectionRequest; using grpc::reflection::v1alpha::ServerReflectionResponse; +const uint8_t g_timeoutGrpcMainStreamSeconds = 10; //using default gwhisper timeout of 10 seconds. namespace grpc { ProtoReflectionDescriptorDatabase::ProtoReflectionDescriptorDatabase( @@ -300,6 +301,9 @@ void ProtoReflectionDescriptorDatabase::AddFileFromResponse( const std::shared_ptr ProtoReflectionDescriptorDatabase::GetStream() { if (!stream_) { + std::chrono::system_clock::time_point deadline = + std::chrono::system_clock::now() + std::chrono::seconds(g_timeoutGrpcMainStreamSeconds); + ctx_.set_deadline(deadline); stream_ = stub_->ServerReflectionInfo(&ctx_); } return stream_; @@ -317,16 +321,13 @@ bool ProtoReflectionDescriptorDatabase::DoOneRequest( return success; } -grpc::Status ProtoReflectionDescriptorDatabase::closeStreamWithDeadline(std::optional> deadline) +grpc::Status ProtoReflectionDescriptorDatabase::closeDescDbStream() { stream_mutex_.lock(); - if( deadline != std::nullopt ) - { - ctx_.set_deadline(deadline.value()); - } auto status = closeStream(); stream_.reset(); + stream_mutex_.unlock(); return status; } @@ -342,6 +343,9 @@ grpc::Status ProtoReflectionDescriptorDatabase::closeStream() fprintf(stderr, "Reflection request not implemented; " "is the ServerReflection service enabled?\n"); + } else if (status.error_code() == StatusCode::DEADLINE_EXCEEDED) { + fprintf(stderr, + "ServerReflectionInfo rpc failed. Grpc Server failed to close the stream within %d seconds.\n", g_timeoutGrpcMainStreamSeconds); } else { fprintf(stderr, "ServerReflectionInfo rpc failed. Error code: %d, message: %s, " From b9d3df156ad7e06f0909a6e18c848f8ebc62029f Mon Sep 17 00:00:00 2001 From: Rahman Abber Tahir Date: Wed, 22 Nov 2023 21:24:24 +0100 Subject: [PATCH 2/8] remove cache file when failed to close descdb stream gracefully. Signed-off-by: Rahman Abber Tahir --- src/libLocalDescriptorCache/DescDbProxy.cpp | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/src/libLocalDescriptorCache/DescDbProxy.cpp b/src/libLocalDescriptorCache/DescDbProxy.cpp index 66baed1..6d11032 100644 --- a/src/libLocalDescriptorCache/DescDbProxy.cpp +++ b/src/libLocalDescriptorCache/DescDbProxy.cpp @@ -374,11 +374,20 @@ void DescDbProxy::getDescriptors(const std::string &f_hostAddress) grpc::Status DescDbProxy::closeDescDbStream() { + grpc::Status status; if ( m_reflectionDescDb == nullptr ) { - return grpc::Status::OK; + return status; } - return m_reflectionDescDb->closeDescDbStream(); + status = m_reflectionDescDb->closeDescDbStream(); + if(not status.ok()) + { + //failure to close stream leads to invalid cache, + //removing it here so it will be written again next time. + std::string cacheFilePath = prepareCacheFile(); + std::filesystem::remove(cacheFilePath); + } + return status; } DescDbProxy::DescDbProxy(bool disableCache, const std::string &hostAddress, std::shared_ptr channel, @@ -409,4 +418,8 @@ DescDbProxy::DescDbProxy(bool disableCache, const std::string &hostAddress, std: } } -DescDbProxy::~DescDbProxy(){} \ No newline at end of file +DescDbProxy::~DescDbProxy() +{ + closeDescDbStream(); //close it here to ensure invalid cache file is removed if an error occurs. + //enforcing descDb repopulation next time. +} From 0e77e011808f1bfcc2548928f57338f950ca9e8e Mon Sep 17 00:00:00 2001 From: Rahman Abber Tahir Date: Wed, 22 Nov 2023 21:31:24 +0100 Subject: [PATCH 3/8] log error and continue with rpc if the desc db stream was not closed gracefully. Signed-off-by: Rahman Abber Tahir --- src/libCli/Call.cpp | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/libCli/Call.cpp b/src/libCli/Call.cpp index 2f898ea..9956d20 100644 --- a/src/libCli/Call.cpp +++ b/src/libCli/Call.cpp @@ -190,14 +190,9 @@ namespace cli } } - //before calling the RPC, close the DescDb connection with a timeout. + //before calling the RPC, close the DescDb connection with a timeout. We still continue with rpc call + //but remove the cache file so fetch the reflection data again. grpc::Status dbDescStatus = ConnectionManager::getInstance().closeDescDbStream(serverAddress); - if (not dbDescStatus.ok()) - { - std::cerr << "Failed to close reflection stream ;( Pls try again." << std::endl; - std::cerr << "Status code: " << std::to_string(dbDescStatus.error_code()) << " " << cli::getGrpcStatusCodeAsString(dbDescStatus.error_code()) << ", error message: " << dbDescStatus.error_message() << std::endl; - return -1; - } grpc::testing::CliCall call(channel, methodStr, clientMetadata, deadline); From 08fd4447089033ead69e70817ebb7916484fc7e3 Mon Sep 17 00:00:00 2001 From: Rahman Abber Tahir Date: Wed, 22 Nov 2023 21:35:57 +0100 Subject: [PATCH 4/8] fix comments. Signed-off-by: Rahman Abber Tahir --- src/libCli/Call.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libCli/Call.cpp b/src/libCli/Call.cpp index 9956d20..c2fc33e 100644 --- a/src/libCli/Call.cpp +++ b/src/libCli/Call.cpp @@ -190,8 +190,8 @@ namespace cli } } - //before calling the RPC, close the DescDb connection with a timeout. We still continue with rpc call - //but remove the cache file so fetch the reflection data again. + //before calling the RPC, close the DescDb connection with a default timeout. We still continue with rpc call + //but remove the cache file if the stream was not closed gracefully so it fetches the reflection data again next time. grpc::Status dbDescStatus = ConnectionManager::getInstance().closeDescDbStream(serverAddress); grpc::testing::CliCall call(channel, methodStr, clientMetadata, deadline); From 6929a236004444aed3c509ed94abe765d1898ecb Mon Sep 17 00:00:00 2001 From: Rahman Abber Tahir Date: Thu, 23 Nov 2023 09:57:34 +0100 Subject: [PATCH 5/8] increase timeout from 10 to 20 seconds. Signed-off-by: Rahman Abber Tahir --- third_party/gRPC_utils/proto_reflection_descriptor_database.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/third_party/gRPC_utils/proto_reflection_descriptor_database.cc b/third_party/gRPC_utils/proto_reflection_descriptor_database.cc index 83f8943..7f3bc3a 100644 --- a/third_party/gRPC_utils/proto_reflection_descriptor_database.cc +++ b/third_party/gRPC_utils/proto_reflection_descriptor_database.cc @@ -32,7 +32,7 @@ using grpc::reflection::v1alpha::ServerReflection; using grpc::reflection::v1alpha::ServerReflectionRequest; using grpc::reflection::v1alpha::ServerReflectionResponse; -const uint8_t g_timeoutGrpcMainStreamSeconds = 10; //using default gwhisper timeout of 10 seconds. +const uint8_t g_timeoutGrpcMainStreamSeconds = 20; //using default gwhisper timeout of 20 seconds. namespace grpc { ProtoReflectionDescriptorDatabase::ProtoReflectionDescriptorDatabase( From 117c2155f54e713b073a541f8f4d1a8659a6fbfd Mon Sep 17 00:00:00 2001 From: Rahman Abber Tahir Date: Mon, 4 Dec 2023 10:32:14 +0100 Subject: [PATCH 6/8] added some comments to mention default timeout to close descDb stream. Signed-off-by: Rahman Abber Tahir --- src/libCli/Call.cpp | 2 +- src/libCli/ConnectionManager.cpp | 2 +- src/libCli/libCli/ConnectionManager.hpp | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libCli/Call.cpp b/src/libCli/Call.cpp index c2fc33e..510f059 100644 --- a/src/libCli/Call.cpp +++ b/src/libCli/Call.cpp @@ -192,7 +192,7 @@ namespace cli //before calling the RPC, close the DescDb connection with a default timeout. We still continue with rpc call //but remove the cache file if the stream was not closed gracefully so it fetches the reflection data again next time. - grpc::Status dbDescStatus = ConnectionManager::getInstance().closeDescDbStream(serverAddress); + ConnectionManager::getInstance().closeDescDbStream(serverAddress); grpc::testing::CliCall call(channel, methodStr, clientMetadata, deadline); diff --git a/src/libCli/ConnectionManager.cpp b/src/libCli/ConnectionManager.cpp index 350ef25..3422e9d 100644 --- a/src/libCli/ConnectionManager.cpp +++ b/src/libCli/ConnectionManager.cpp @@ -73,7 +73,7 @@ namespace cli return grpc::Status( grpc::StatusCode::ABORTED, "descDbProxy has not been initialized."); } - //if proxy exists close the stream with a deadline. + //if proxy exists close the stream. grpc::Status status = m_connections[f_serverAddress].descDbProxy->closeDescDbStream(); //delete the proxy, findChannelByAddress() protects from accessing uninitialzed DbProxy. diff --git a/src/libCli/libCli/ConnectionManager.hpp b/src/libCli/libCli/ConnectionManager.hpp index 19e8539..2a48f78 100644 --- a/src/libCli/libCli/ConnectionManager.hpp +++ b/src/libCli/libCli/ConnectionManager.hpp @@ -52,7 +52,7 @@ namespace cli /// @returns the gRpc DescriptorPool of the corresponding server address. std::shared_ptr getDescPool(std::string f_serverAddress, ArgParse::ParsedElement &f_parseTree); - /// @brief closes the DescDb stream with a default deadline. + /// @brief closes the DescDb stream with a default deadline of 2 seconds. /// @param f_serverAddress server addresss to lookup the assigned DescDbProxy. /// @return returns grpc::StatusCode::ABORTED status if no DescDb proxy is attached to the server address, /// otherwise grpc status as a result of stream closure. From 958a2e23f003cc65ea7db5a3cbc075be9efdc84b Mon Sep 17 00:00:00 2001 From: Rahman Abber Tahir Date: Mon, 4 Dec 2023 10:35:22 +0100 Subject: [PATCH 7/8] comment-correction: timeout is 20secs. Signed-off-by: Rahman Abber Tahir --- src/libCli/libCli/ConnectionManager.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libCli/libCli/ConnectionManager.hpp b/src/libCli/libCli/ConnectionManager.hpp index 2a48f78..d0c2547 100644 --- a/src/libCli/libCli/ConnectionManager.hpp +++ b/src/libCli/libCli/ConnectionManager.hpp @@ -52,7 +52,7 @@ namespace cli /// @returns the gRpc DescriptorPool of the corresponding server address. std::shared_ptr getDescPool(std::string f_serverAddress, ArgParse::ParsedElement &f_parseTree); - /// @brief closes the DescDb stream with a default deadline of 2 seconds. + /// @brief closes the DescDb stream with a default deadline of 20 seconds. /// @param f_serverAddress server addresss to lookup the assigned DescDbProxy. /// @return returns grpc::StatusCode::ABORTED status if no DescDb proxy is attached to the server address, /// otherwise grpc status as a result of stream closure. From 6d49288dfd94c3a157aa8ebfcfac72d9d7d44c42 Mon Sep 17 00:00:00 2001 From: Rahman Abber Tahir Date: Mon, 4 Dec 2023 10:44:56 +0100 Subject: [PATCH 8/8] add comment for streamClose in DescDbProxy dtor. Signed-off-by: Rahman Abber Tahir --- src/libLocalDescriptorCache/DescDbProxy.cpp | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/libLocalDescriptorCache/DescDbProxy.cpp b/src/libLocalDescriptorCache/DescDbProxy.cpp index 6d11032..6ac5373 100644 --- a/src/libLocalDescriptorCache/DescDbProxy.cpp +++ b/src/libLocalDescriptorCache/DescDbProxy.cpp @@ -374,7 +374,7 @@ void DescDbProxy::getDescriptors(const std::string &f_hostAddress) grpc::Status DescDbProxy::closeDescDbStream() { - grpc::Status status; + ::grpc::Status status; if ( m_reflectionDescDb == nullptr ) { return status; @@ -420,6 +420,11 @@ DescDbProxy::DescDbProxy(bool disableCache, const std::string &hostAddress, std: DescDbProxy::~DescDbProxy() { - closeDescDbStream(); //close it here to ensure invalid cache file is removed if an error occurs. - //enforcing descDb repopulation next time. + //This call is a noop if desc db stream is already closed. + //There are two scenarios when we close the stream here. + //i) Stream failed to close within a timeout. + // Our rpc may still succeed but this invalidates the cache so we remove the cache file, + // to repopulate desc db on next rpc. + //íi) Stream successfully closed, leave the cache as it is. + closeDescDbStream(); }