Repository: thrift Updated Branches: refs/heads/master 39310dad7 -> 9f9e30b51
THRIFT-4331: C++ TSSLSocket fixes for huge message handling Client: C++ fixed issue with large messages, where waitForEvent was called mutliple times waiting for SSL_read() to get bytes and running in the retry timeout. fixed issue where poll was not using the right flags. This fixes #1363 Project: http://git-wip-us.apache.org/repos/asf/thrift/repo Commit: http://git-wip-us.apache.org/repos/asf/thrift/commit/9f9e30b5 Tree: http://git-wip-us.apache.org/repos/asf/thrift/tree/9f9e30b5 Diff: http://git-wip-us.apache.org/repos/asf/thrift/diff/9f9e30b5 Branch: refs/heads/master Commit: 9f9e30b51e3912c0b63258badf5501d3cb2550be Parents: 39310da Author: Martin Haimberger <martin.haimber...@thincast.com> Authored: Fri Oct 6 09:57:27 2017 +0200 Committer: James E. King, III <jk...@apache.org> Committed: Fri Oct 6 05:22:13 2017 -0700 ---------------------------------------------------------------------- lib/cpp/src/thrift/transport/TSSLSocket.cpp | 28 +++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/thrift/blob/9f9e30b5/lib/cpp/src/thrift/transport/TSSLSocket.cpp ---------------------------------------------------------------------- diff --git a/lib/cpp/src/thrift/transport/TSSLSocket.cpp b/lib/cpp/src/thrift/transport/TSSLSocket.cpp index acf23aa..ddefb34 100644 --- a/lib/cpp/src/thrift/transport/TSSLSocket.cpp +++ b/lib/cpp/src/thrift/transport/TSSLSocket.cpp @@ -295,8 +295,9 @@ bool TSSLSocket::peek() { } case SSL_ERROR_WANT_READ: case SSL_ERROR_WANT_WRITE: - waitForEvent(error == SSL_ERROR_WANT_READ); - continue; + // in the case of SSL_ERROR_SYSCALL we want to wait for an read event again + waitForEvent(error != SSL_ERROR_WANT_WRITE); + continue; default:;// do nothing } string errors; @@ -338,6 +339,7 @@ void TSSLSocket::close() { } case SSL_ERROR_WANT_READ: case SSL_ERROR_WANT_WRITE: + // in the case of SSL_ERROR_SYSCALL we want to wait for an write/read event again waitForEvent(error == SSL_ERROR_WANT_READ); rc = 2; default:;// do nothing @@ -387,6 +389,7 @@ uint32_t TSSLSocket::read(uint8_t* buf, uint32_t len) { break; } int32_t errno_copy = THRIFT_GET_SOCKET_ERROR; + unsigned int waitEventReturn; switch (error) { case SSL_ERROR_SYSCALL: if ((errno_copy != THRIFT_EINTR) @@ -408,7 +411,8 @@ uint32_t TSSLSocket::read(uint8_t* buf, uint32_t len) { } throw TTransportException(TTransportException::INTERNAL_ERROR, "too much recv retries"); } - else if (waitForEvent(error == SSL_ERROR_WANT_READ) == TSSL_EINTR ) { + // in the case of SSL_ERROR_SYSCALL we want to wait for an read event again + else if ((waitEventReturn = waitForEvent(error != SSL_ERROR_WANT_WRITE)) == TSSL_EINTR ) { // repeat operation if (readRetryCount_ < maxRecvRetries_) { // THRIFT_EINTR needs to be handled manually and we can tolerate @@ -417,7 +421,15 @@ uint32_t TSSLSocket::read(uint8_t* buf, uint32_t len) { } throw TTransportException(TTransportException::INTERNAL_ERROR, "too much recv retries"); } - continue; + else if (waitEventReturn == TSSL_DATA) { + // in case of SSL and huge thrift packets, there may be a number of + // socket operations, before any data becomes available by SSL_read(). + // Therefore the number of retries should not be increased and + // the operation should be repeated. + readRetryCount_--; + continue; + } + throw TTransportException(TTransportException::INTERNAL_ERROR, "unkown waitForEvent return value"); default:;// do nothing } string errors; @@ -451,6 +463,7 @@ void TSSLSocket::write(const uint8_t* buf, uint32_t len) { return; } else { + // in the case of SSL_ERROR_SYSCALL we want to wait for an write event again waitForEvent(error == SSL_ERROR_WANT_READ); continue; } @@ -494,6 +507,7 @@ uint32_t TSSLSocket::write_partial(const uint8_t* buf, uint32_t len) { return 0; } else { + // in the case of SSL_ERROR_SYSCALL we want to wait for an write event again waitForEvent(error == SSL_ERROR_WANT_READ); continue; } @@ -579,6 +593,7 @@ void TSSLSocket::initializeHandshake() { } else { // repeat operation + // in the case of SSL_ERROR_SYSCALL we want to wait for an write/read event again waitForEvent(error == SSL_ERROR_WANT_READ); rc = 2; } @@ -610,6 +625,7 @@ void TSSLSocket::initializeHandshake() { } else { // repeat operation + // in the case of SSL_ERROR_SYSCALL we want to wait for an write/read event again waitForEvent(error == SSL_ERROR_WANT_READ); rc = 2; } @@ -759,7 +775,9 @@ unsigned int TSSLSocket::waitForEvent(bool wantRead) { struct THRIFT_POLLFD fds[2]; memset(fds, 0, sizeof(fds)); fds[0].fd = fdSocket; - fds[0].events = wantRead ? THRIFT_POLLIN : THRIFT_POLLOUT; + // use POLLIN also on write operations too, this is needed for operations + // which requires read and write on the socket. + fds[0].events = wantRead ? THRIFT_POLLIN : THRIFT_POLLIN | THRIFT_POLLOUT; if (interruptListener_) { fds[1].fd = *(interruptListener_.get());
