Skip to content

Commit

Permalink
reduce failure logging in TSocket::openConnection
Browse files Browse the repository at this point in the history
TSocket::local_open() calls openConnection for all addresses associated with a given
hostname and succeeds on first one that succeeds. The failed tries lead to unnecessary
socket failure logs, so added a flag to openConnection to skip logging which is
set to true from local_open() only for if the address being tried is the last one.

Removed inExit flag and instead check for existing openSSLInitialized before trying
to close SSL connections.
  • Loading branch information
sumwale committed May 30, 2021
1 parent 56e9641 commit 3df5174
Show file tree
Hide file tree
Showing 4 changed files with 32 additions and 30 deletions.
3 changes: 1 addition & 2 deletions lib/cpp/src/thrift/transport/TSSLSocket.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,7 @@ void TSSLSocket::open() {
*/
void TSSLSocket::close() {
if (ssl_ != nullptr) {
if (!TSSLSocketFactory::inExit()) try {
if (openSSLInitialized) try {
int rc;
int errno_copy = 0;
int error = 0;
Expand Down Expand Up @@ -854,7 +854,6 @@ unsigned int TSSLSocket::waitForEvent(bool wantRead) {
uint64_t TSSLSocketFactory::count_ = 0;
Mutex TSSLSocketFactory::mutex_;
bool TSSLSocketFactory::manualOpenSSLInitialization_ = false;
bool TSSLSocketFactory::inExit_ = false;

TSSLSocketFactory::TSSLSocketFactory(SSLProtocol protocol) : server_(false) {
Guard guard(mutex_);
Expand Down
15 changes: 0 additions & 15 deletions lib/cpp/src/thrift/transport/TSSLSocket.h
Original file line number Diff line number Diff line change
Expand Up @@ -309,20 +309,6 @@ class TSSLSocketFactory {
static void setManualOpenSSLInitialization(bool manualOpenSSLInitialization) {
manualOpenSSLInitialization_ = manualOpenSSLInitialization;
}
/**
* Set the "inExit_" flag that will avoid SSL_shutdown() for any subsequent
* TSSLSocket::close() calls. Typically useful during global destructor/exit
* that can otherwise cause unexpected trouble in SSL_shutdown().
*/
static void setInExit(bool inExit) {
inExit_ = inExit;
}
/**
* Return the global flag as set by setInExit().
*/
static bool inExit() {
return inExit_;
}

protected:
std::shared_ptr<SSLContext> ctx_;
Expand All @@ -342,7 +328,6 @@ class TSSLSocketFactory {
static concurrency::Mutex mutex_;
static uint64_t count_;
THRIFT_EXPORT static bool manualOpenSSLInitialization_;
THRIFT_EXPORT static bool inExit_;
void setup(std::shared_ptr<TSSLSocket> ssl);
static int passwordCallback(char* password, int size, int, void* data);
};
Expand Down
42 changes: 30 additions & 12 deletions lib/cpp/src/thrift/transport/TSocket.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@ bool TSocket::peek() {
return (r > 0);
}

void TSocket::openConnection(struct addrinfo* res) {
void TSocket::openConnection(struct addrinfo* res, bool logError) {

if (isOpen()) {
return;
Expand All @@ -270,7 +270,9 @@ void TSocket::openConnection(struct addrinfo* res) {

if (socket_ == THRIFT_INVALID_SOCKET) {
int errno_copy = THRIFT_GET_SOCKET_ERROR;
GlobalOutput.perror("TSocket::open() socket() " + getSocketInfo(), errno_copy);
if (logError) {
GlobalOutput.perror("TSocket::open() socket() " + getSocketInfo(), errno_copy);
}
throw TTransportException(TTransportException::NOT_OPEN, "socket()", errno_copy);
}

Expand Down Expand Up @@ -314,13 +316,17 @@ void TSocket::openConnection(struct addrinfo* res) {
if (connTimeout_ > 0) {
if (-1 == THRIFT_FCNTL(socket_, THRIFT_F_SETFL, flags | THRIFT_O_NONBLOCK)) {
int errno_copy = THRIFT_GET_SOCKET_ERROR;
GlobalOutput.perror("TSocket::open() THRIFT_FCNTL() " + getSocketInfo(), errno_copy);
if (logError) {
GlobalOutput.perror("TSocket::open() THRIFT_FCNTL() " + getSocketInfo(), errno_copy);
}
throw TTransportException(TTransportException::NOT_OPEN, "THRIFT_FCNTL() failed", errno_copy);
}
} else {
if (-1 == THRIFT_FCNTL(socket_, THRIFT_F_SETFL, flags & ~THRIFT_O_NONBLOCK)) {
int errno_copy = THRIFT_GET_SOCKET_ERROR;
GlobalOutput.perror("TSocket::open() THRIFT_FCNTL " + getSocketInfo(), errno_copy);
if (logError) {
GlobalOutput.perror("TSocket::open() THRIFT_FCNTL " + getSocketInfo(), errno_copy);
}
throw TTransportException(TTransportException::NOT_OPEN, "THRIFT_FCNTL() failed", errno_copy);
}
}
Expand Down Expand Up @@ -357,7 +363,9 @@ void TSocket::openConnection(struct addrinfo* res) {
if ((THRIFT_GET_SOCKET_ERROR != THRIFT_EINPROGRESS)
&& (THRIFT_GET_SOCKET_ERROR != THRIFT_EWOULDBLOCK)) {
int errno_copy = THRIFT_GET_SOCKET_ERROR;
GlobalOutput.perror("TSocket::open() connect() " + getSocketInfo(), errno_copy);
if (logError) {
GlobalOutput.perror("TSocket::open() connect() " + getSocketInfo(), errno_copy);
}
throw TTransportException(TTransportException::NOT_OPEN, "connect() failed", errno_copy);
}

Expand All @@ -375,33 +383,43 @@ void TSocket::openConnection(struct addrinfo* res) {
int ret2 = getsockopt(socket_, SOL_SOCKET, SO_ERROR, cast_sockopt(&val), &lon);
if (ret2 == -1) {
int errno_copy = THRIFT_GET_SOCKET_ERROR;
GlobalOutput.perror("TSocket::open() getsockopt() " + getSocketInfo(), errno_copy);
if (logError) {
GlobalOutput.perror("TSocket::open() getsockopt() " + getSocketInfo(), errno_copy);
}
throw TTransportException(TTransportException::NOT_OPEN, "getsockopt()", errno_copy);
}
// no errors on socket, go to town
if (val == 0) {
goto done;
}
GlobalOutput.perror("TSocket::open() error on socket (after THRIFT_POLL) " + getSocketInfo(),
val);
if (logError) {
GlobalOutput.perror("TSocket::open() error on socket (after THRIFT_POLL) " +
getSocketInfo(), val);
}
throw TTransportException(TTransportException::NOT_OPEN, "socket open() error", val);
} else if (ret == 0) {
// socket timed out
string errStr = "TSocket::open() timed out " + getSocketInfo();
GlobalOutput(errStr.c_str());
if (logError) {
GlobalOutput(errStr.c_str());
}
throw TTransportException(TTransportException::NOT_OPEN, "open() timed out");
} else {
// error on THRIFT_POLL()
int errno_copy = THRIFT_GET_SOCKET_ERROR;
GlobalOutput.perror("TSocket::open() THRIFT_POLL() " + getSocketInfo(), errno_copy);
if (logError) {
GlobalOutput.perror("TSocket::open() THRIFT_POLL() " + getSocketInfo(), errno_copy);
}
throw TTransportException(TTransportException::NOT_OPEN, "THRIFT_POLL() failed", errno_copy);
}

done:
// Set socket back to normal mode (blocking)
if (-1 == THRIFT_FCNTL(socket_, THRIFT_F_SETFL, flags)) {
int errno_copy = THRIFT_GET_SOCKET_ERROR;
GlobalOutput.perror("TSocket::open() THRIFT_FCNTL " + getSocketInfo(), errno_copy);
if (logError) {
GlobalOutput.perror("TSocket::open() THRIFT_FCNTL " + getSocketInfo(), errno_copy);
}
throw TTransportException(TTransportException::NOT_OPEN, "THRIFT_FCNTL() failed", errno_copy);
}

Expand Down Expand Up @@ -480,7 +498,7 @@ void TSocket::local_open() {
// connects or push the exception up.
for (res = res0; res; res = res->ai_next) {
try {
openConnection(res);
openConnection(res, res->ai_next == nullptr);
break;
} catch (TTransportException&) {
if (res->ai_next) {
Expand Down
2 changes: 1 addition & 1 deletion lib/cpp/src/thrift/transport/TSocket.h
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,7 @@ class TSocket : public TVirtualTransport<TSocket> {

protected:
/** connect, called by open */
void openConnection(struct addrinfo* res);
void openConnection(struct addrinfo* res, bool logError = true);

/** Host to connect to */
std::string host_;
Expand Down

0 comments on commit 3df5174

Please sign in to comment.