diff --git a/include/envoy/buffer/buffer.h b/include/envoy/buffer/buffer.h index 719d63a060d9d..a874168b1d930 100644 --- a/include/envoy/buffer/buffer.h +++ b/include/envoy/buffer/buffer.h @@ -59,6 +59,13 @@ class Instance { * @return the actual number of slices needed, which may be greater than out_size. Passing * nullptr for out and 0 for out_size will just return the size of the array needed * to capture all of the slice data. + * TODO(mattklein123): WARNING: The underlying implementation of this function currently uses + * libevent's evbuffer. It has the infuriating property where calling getRawSlices(nullptr, 0) + * will return the slices that include all of the buffer data, but not any empty slices at the + * end. However, calling getRawSlices(iovec, SOME_CONST), WILL return potentially empty slices + * beyond the end of the buffer. Code that is trying to avoid stack overflow by limiting the + * number of returned slices needs to deal with this. When we get rid of evbuffer we can rework + * all of this. */ virtual uint64_t getRawSlices(RawSlice* out, uint64_t out_size) const PURE; diff --git a/source/common/ssl/connection_impl.cc b/source/common/ssl/connection_impl.cc index 81798fa9ca052..bdf71dc0e9d25 100644 --- a/source/common/ssl/connection_impl.cc +++ b/source/common/ssl/connection_impl.cc @@ -134,22 +134,21 @@ Network::ConnectionImpl::IoResult ConnectionImpl::doWriteToSocket() { } } + uint64_t original_buffer_length = write_buffer_.length(); uint64_t total_bytes_written = 0; bool keep_writing = true; - while ((write_buffer_.length() > 0) && keep_writing) { + while ((original_buffer_length != total_bytes_written) && keep_writing) { // Protect against stack overflow if the buffer has a very large buffer chain. - // TODO(mattklein123): The current evbuffer Buffer::Instance implementation will iterate through - // the entire chain each time this is called to determine how many slices would be needed. In - // this case, we don't care, and only want to fill up to MAX_SLICES. When we swap out evbuffer - // we can change this behavior. + // TODO(mattklein123): See the comment on getRawSlices() for why we have to also check + // original_buffer_length != total_bytes_written during loop iteration. // TODO(mattklein123): As it relates to our fairness efforts, we might want to limit the number // of iterations of this loop, either by pure iterations, bytes written, etc. const uint64_t MAX_SLICES = 32; Buffer::RawSlice slices[MAX_SLICES]; - uint64_t num_slices = std::min(MAX_SLICES, write_buffer_.getRawSlices(slices, MAX_SLICES)); + uint64_t num_slices = write_buffer_.getRawSlices(slices, MAX_SLICES); uint64_t inner_bytes_written = 0; - for (uint64_t i = 0; i < num_slices; i++) { + for (uint64_t i = 0; (i < num_slices) && (original_buffer_length != total_bytes_written); i++) { // SSL_write() requires that if a previous call returns SSL_ERROR_WANT_WRITE, we need to call // it again with the same parameters. Most implementations keep track of the last write size. // In our case we don't need to do that because: a) SSL_write() will not write partial diff --git a/test/common/ssl/connection_impl_test.cc b/test/common/ssl/connection_impl_test.cc index 13fa2a85d9aad..289519f46132f 100644 --- a/test/common/ssl/connection_impl_test.cc +++ b/test/common/ssl/connection_impl_test.cc @@ -248,7 +248,7 @@ TEST(SslConnectionImplTest, SslError) { class SslReadBufferLimitTest : public testing::Test { public: void readBufferLimitTest(uint32_t read_buffer_limit, uint32_t expected_chunk_size, - uint32_t write_size, uint32_t num_writes) { + uint32_t write_size, uint32_t num_writes, bool reserve_write_space) { Stats::IsolatedStoreImpl stats_store; Event::DispatcherImpl dispatcher; Network::TcpListenSocket socket(uint32_t(10000), true); @@ -325,6 +325,16 @@ class SslReadBufferLimitTest : public testing::Test { for (uint32_t i = 0; i < num_writes; i++) { Buffer::OwnedImpl data(std::string(write_size, 'a')); + + // Incredibly contrived way of making sure that the write buffer has an empty chain in it. + if (reserve_write_space) { + Buffer::RawSlice iovecs[2]; + EXPECT_EQ(2UL, data.reserve(16384, iovecs, 2)); + iovecs[0].len_ = 0; + iovecs[1].len_ = 0; + data.commit(iovecs, 2); + } + client_connection->write(data); } @@ -332,14 +342,18 @@ class SslReadBufferLimitTest : public testing::Test { } }; -TEST_F(SslReadBufferLimitTest, NoLimit) { readBufferLimitTest(0, 256 * 1024, 256 * 1024, 1); } +TEST_F(SslReadBufferLimitTest, NoLimit) { + readBufferLimitTest(0, 256 * 1024, 256 * 1024, 1, false); +} + +TEST_F(SslReadBufferLimitTest, NoLimitReserveSpace) { readBufferLimitTest(0, 512, 512, 1, true); } TEST_F(SslReadBufferLimitTest, NoLimitSmallWrites) { - readBufferLimitTest(0, 256 * 1024, 1, 256 * 1024); + readBufferLimitTest(0, 256 * 1024, 1, 256 * 1024, false); } TEST_F(SslReadBufferLimitTest, SomeLimit) { - readBufferLimitTest(32 * 1024, 32 * 1024, 256 * 1024, 1); + readBufferLimitTest(32 * 1024, 32 * 1024, 256 * 1024, 1, false); } } // Ssl