Skip to content

Commit

Permalink
MB-36418: Revert "MB-36028: Flush the SSL socket more aggressively"
Browse files Browse the repository at this point in the history 
This caused a throughput decrease in YCSB Workload E

This reverts commit 79a6721.

Change-Id: I067345b56518e92cc76dd0bb318bb2c31d334912
Reviewed-on: http://review.couchbase.org/116206
Tested-by: Build Bot <build@couchbase.com>
Reviewed-by: Dave Rigby <daver@couchbase.com>
  • Loading branch information
@trondn @daverigby
trondn authored and daverigby committed Oct 10, 2019
1 parent da75443 commit 960c5c7
Show file tree
Hide file tree
Showing 3 changed files with 116 additions and 217 deletions.
150 changes: 101 additions & 49 deletions daemon/connection.cc
View file
Expand Up @@ -789,11 +789,7 @@ void Connection::logSslErrorInfo(const std::string& method, int rval) {
}

int Connection::sslPreConnection() {
int r = ssl.accept(socketDescriptor);
if (ssl.hasError()) {
cb::net::set_econnreset();
return -1;
}
int r = ssl.accept();
if (r == 1) {
ssl.drainBioSendPipe(socketDescriptor);
ssl.setConnected();
Expand Down Expand Up @@ -845,17 +841,19 @@ int Connection::sslPreConnection() {
LOG_INFO("{}: Using SSL cipher:{}",
getId(),
ssl.getCurrentCipherName());
return 0;
}

auto err = ssl.getError(r);
if (err == SSL_ERROR_WANT_READ || err == SSL_ERROR_WANT_WRITE) {
cb::net::set_ewouldblock();
} else {
logSslErrorInfo("SSL_accept", r);
cb::net::set_econnreset();
if (ssl.getError(r) == SSL_ERROR_WANT_READ) {
ssl.drainBioSendPipe(socketDescriptor);
cb::net::set_ewouldblock();
return -1;
} else {
logSslErrorInfo("SSL_accept", r);
cb::net::set_econnreset();
return -1;
}
}
return -1;

return 0;
}

int Connection::recv(char* dest, size_t nbytes) {
Expand Down Expand Up @@ -1132,56 +1130,110 @@ Connection::TryReadResult Connection::tryReadNetwork() {
}

int Connection::sslRead(char* dest, size_t nbytes) {
const auto ret = ssl.read(socketDescriptor, dest, gsl::narrow<int>(nbytes));
if (ssl.hasError()) {
cb::net::set_econnreset();
return -1;
}

if (ret > 0) {
return ret;
}
const int error = ssl.getError(ret);
int ret = 0;

switch (error) {
case SSL_ERROR_WANT_READ:
case SSL_ERROR_WANT_WRITE:
cb::net::set_ewouldblock();
return -1;
while (ret < int(nbytes)) {
int n;
ssl.drainBioRecvPipe(socketDescriptor);
if (ssl.hasError()) {
cb::net::set_econnreset();
return -1;
}
n = ssl.read(dest + ret, (int)(nbytes - ret));
if (n > 0) {
ret += n;
} else {
/* n < 0 and n == 0 require a check of SSL error*/
const int error = ssl.getError(n);

switch (error) {
case SSL_ERROR_WANT_READ:
/*
* Drain the buffers and retry if we've got data in
* our input buffers
*/
if (ssl.moreInputAvailable()) {
/* our recv buf has data feed the BIO */
ssl.drainBioRecvPipe(socketDescriptor);
} else if (ret > 0) {
/* nothing in our recv buf, return what we have */
return ret;
} else {
cb::net::set_ewouldblock();
return -1;
}
break;

case SSL_ERROR_ZERO_RETURN:
/* The TLS/SSL connection has been closed (cleanly). */
return 0;
case SSL_ERROR_ZERO_RETURN:
/* The TLS/SSL connection has been closed (cleanly). */
return 0;

default:
logSslErrorInfo("SSL_read", ret);
cb::net::set_econnreset();
return -1;
default:
logSslErrorInfo("SSL_read", n);
cb::net::set_econnreset();
return -1;
}
}
}
// not reached

return ret;
}

int Connection::sslWrite(const char* src, size_t nbytes) {
const auto ret = ssl.write(socketDescriptor, src, gsl::narrow<int>(nbytes));
if (ssl.hasError() || ret == 0) {
// Start by trying to send everything we've already got
// buffered in our bio
ssl.drainBioSendPipe(socketDescriptor);
if (ssl.hasError()) {
cb::net::set_econnreset();
return -1;
}
if (ret > 0) {
return ret;
}

const int error = ssl.getError(ret);
if (error == SSL_ERROR_WANT_WRITE || error == SSL_ERROR_WANT_READ) {
// If the network socket is full there isn't much point
// of trying to add more to SSL
if (ssl.morePendingOutput()) {
cb::net::set_ewouldblock();
return -1;
}

// We encountered an error we don't have code to handle.
// Reset the connection
logSslErrorInfo("SSL_write", ret);
cb::net::set_econnreset();
return -1;
// We've got an empty buffer for SSL to operate on,
// so lets get to it.
do {
const auto n = ssl.write(src, int(nbytes));
if (n > 0) {
// we've successfully sent some bytes to
// SSL. Lets try to flush it to the network
// socket buffers.
ssl.drainBioSendPipe(socketDescriptor);
if (ssl.hasError()) {
cb::net::set_econnreset();
return -1;
}
// Return the number of bytes submitted to SSL
return n;
} else if (n == 0) {
// Closed connection.
cb::net::set_econnreset();
return -1;
} else {
const int error = ssl.getError(n);
if (error == SSL_ERROR_WANT_WRITE) {
ssl.drainBioSendPipe(socketDescriptor);
if (ssl.morePendingOutput()) {
cb::net::set_ewouldblock();
return -1;
}
// We've got space in the network buffer.
// retry the operation (and openssl will
// try to fill the network buffer again)
} else {
// We enountered an error we don't have code
// to handle. Reset the connection
logSslErrorInfo("SSL_write", n);
cb::net::set_econnreset();
return -1;
}
}
} while (true);
}

bool Connection::useCookieSendResponse(std::size_t size) const {
Expand Down
59 changes: 5 additions & 54 deletions daemon/ssl_context.h
View file
Expand Up @@ -107,18 +107,16 @@ class SslContext {
* as possible.
*
* @param sfd the socket to read data from
* @return true if any progress was made
*/
bool drainBioRecvPipe(SOCKET sfd);
void drainBioRecvPipe(SOCKET sfd);

/**
* Try to drain the SSL stream with as much data as possible and
* send it over the network.
*
* @param sfd the socket to write data to
* @return true if any progress was made
*/
bool drainBioSendPipe(SOCKET sfd);
void drainBioSendPipe(SOCKET sfd);

bool moreInputAvailable() const {
return !inputPipe.empty();
Expand All @@ -135,50 +133,13 @@ class SslContext {
*/
void dumpCipherList(uint32_t id) const;

/**
* Accept the next connection.
*
* Check hasError() before looking at the return code (hasError is for
* the underlying socket error)
*
* @param sfd The socket used for recv/send in the case we
* need more data
* @return 1 success
* 0 clean shutdown
* <0 an error
*/
int accept(SOCKET sfd);
int accept();

int getError(int errormask) const;

/**
* Read from the SSL pipe
*
* Check hasError() before looking at the return code (hasError is for
* the underlying socket error)
*
* @param sfd the underlying socket
* @param buf where to store the data
* @param num the amount of bytes to read
* @return > 0 - the number of bytes read
* <= 0 - an error occurred. Check with getError
*/
int read(SOCKET sfd, void* buf, int num);
int read(void* buf, int num);

/**
* Write data to the SSL pipe
*
* Check hasError() before looking at the return code (hasError is for
* the underlying socket error)
*
* @param sfd The underlying socket
* @param buf The data to send
* @param num The number of bytes ot send
* @return > 0 - The number of bytes sent
* 0 - Connection closed
* < 0 - An error occurred. Check with getError
*/
int write(SOCKET sfd, const void* buf, int num);
int write(const void* buf, int num);

bool havePendingInputData();

Expand All @@ -192,16 +153,6 @@ class SslContext {
const char* getCurrentCipherName() const;

protected:
enum class DrainProgress { Error, Progress, NoProgress };

/**
* Try to drain the SSL output buffer and fill its input buffer
*
* @param sock the socket to use for transfer
* @return true if no error occurred
*/
DrainProgress drainBios(SOCKET sock);

bool drainInputSocketBuf();

bool enabled = false;
Expand Down

0 comments on commit 960c5c7

Please sign in to comment.