From 1a00e07085c3981ba4ae38528a46b9a531f1dc6e Mon Sep 17 00:00:00 2001 From: Venil Noronha Date: Tue, 17 Jul 2018 10:27:29 -0700 Subject: [PATCH 1/3] Latch errno deeper in the buffer implementation The errno set by a syscall can be overwritten by code (e.g. logging) as it propagates up through the call stack. This commit refactors the buffer API to allow for returning the errno from deeper down the call stack i.e. as soon as a syscall is performed. Signed-off-by: Venil Noronha --- include/envoy/buffer/buffer.h | 9 ++--- source/common/buffer/buffer_impl.cc | 16 +++++---- source/common/buffer/buffer_impl.h | 4 +-- source/common/buffer/watermark_buffer.cc | 12 +++---- source/common/buffer/watermark_buffer.h | 4 +-- source/common/network/raw_buffer_socket.cc | 10 +++--- .../network/thrift_proxy/buffer_helper.h | 4 +-- test/common/buffer/owned_impl_test.cc | 36 +++++++++---------- test/common/buffer/watermark_buffer_test.cc | 6 ++-- test/common/network/connection_impl_test.cc | 4 +-- test/mocks/buffer/mocks.h | 13 +++---- 11 files changed, 63 insertions(+), 55 deletions(-) diff --git a/include/envoy/buffer/buffer.h b/include/envoy/buffer/buffer.h index 8882cc72dc82..39157d425ab4 100644 --- a/include/envoy/buffer/buffer.h +++ b/include/envoy/buffer/buffer.h @@ -4,6 +4,7 @@ #include #include #include +#include #include "envoy/common/pure.h" @@ -142,9 +143,9 @@ class Instance { * Read from a file descriptor directly into the buffer. * @param fd supplies the descriptor to read from. * @param max_length supplies the maximum length to read. - * @return the number of bytes read or -1 if there was an error. + * @return a tuple with the number of bytes read or -1 if there was an error, and the errno. */ - virtual int read(int fd, uint64_t max_length) PURE; + virtual std::tuple read(int fd, uint64_t max_length) PURE; /** * Reserve space in the buffer. @@ -173,9 +174,9 @@ class Instance { /** * Write the buffer out to a file descriptor. * @param fd supplies the descriptor to write to. - * @return the number of bytes written or -1 if there was an error. + * @return a tuple with the number of bytes written or -1 if there was an error, and the errno. */ - virtual int write(int fd) PURE; + virtual std::tuple write(int fd) PURE; }; typedef std::unique_ptr InstancePtr; diff --git a/source/common/buffer/buffer_impl.cc b/source/common/buffer/buffer_impl.cc index f7bdfcd12aa4..3e70b1d70b88 100644 --- a/source/common/buffer/buffer_impl.cc +++ b/source/common/buffer/buffer_impl.cc @@ -94,9 +94,9 @@ void OwnedImpl::move(Instance& rhs, uint64_t length) { static_cast(rhs).postProcess(); } -int OwnedImpl::read(int fd, uint64_t max_length) { +std::tuple OwnedImpl::read(int fd, uint64_t max_length) { if (max_length == 0) { - return 0; + return std::make_tuple(0, 0); } constexpr uint64_t MaxSlices = 2; RawSlice slices[MaxSlices]; @@ -115,8 +115,9 @@ int OwnedImpl::read(int fd, uint64_t max_length) { ASSERT(num_bytes_to_read <= max_length); auto& os_syscalls = Api::OsSysCallsSingleton::get(); const ssize_t rc = os_syscalls.readv(fd, iov, static_cast(num_slices_to_read)); + const int error = errno; if (rc < 0) { - return rc; + return std::make_tuple(rc, error); } uint64_t num_slices_to_commit = 0; uint64_t bytes_to_commit = rc; @@ -130,7 +131,7 @@ int OwnedImpl::read(int fd, uint64_t max_length) { } ASSERT(num_slices_to_commit <= num_slices); commit(slices, num_slices_to_commit); - return rc; + return std::make_tuple(rc, error); } uint64_t OwnedImpl::reserve(uint64_t length, RawSlice* iovecs, uint64_t num_iovecs) { @@ -151,7 +152,7 @@ ssize_t OwnedImpl::search(const void* data, uint64_t size, size_t start) const { return result_ptr.pos; } -int OwnedImpl::write(int fd) { +std::tuple OwnedImpl::write(int fd) { constexpr uint64_t MaxSlices = 16; RawSlice slices[MaxSlices]; const uint64_t num_slices = std::min(getRawSlices(slices, MaxSlices), MaxSlices); @@ -165,14 +166,15 @@ int OwnedImpl::write(int fd) { } } if (num_slices_to_write == 0) { - return 0; + return std::make_tuple(0, 0); } auto& os_syscalls = Api::OsSysCallsSingleton::get(); const ssize_t rc = os_syscalls.writev(fd, iov, num_slices_to_write); + const int error = errno; if (rc > 0) { drain(static_cast(rc)); } - return static_cast(rc); + return std::make_tuple(static_cast(rc), error); } OwnedImpl::OwnedImpl() : buffer_(evbuffer_new()) {} diff --git a/source/common/buffer/buffer_impl.h b/source/common/buffer/buffer_impl.h index 81245e430461..e4adc74d0f66 100644 --- a/source/common/buffer/buffer_impl.h +++ b/source/common/buffer/buffer_impl.h @@ -80,10 +80,10 @@ class OwnedImpl : public LibEventInstance { void* linearize(uint32_t size) override; void move(Instance& rhs) override; void move(Instance& rhs, uint64_t length) override; - int read(int fd, uint64_t max_length) override; + std::tuple read(int fd, uint64_t max_length) override; uint64_t reserve(uint64_t length, RawSlice* iovecs, uint64_t num_iovecs) override; ssize_t search(const void* data, uint64_t size, size_t start) const override; - int write(int fd) override; + std::tuple write(int fd) override; void postProcess() override {} std::string toString() const override; diff --git a/source/common/buffer/watermark_buffer.cc b/source/common/buffer/watermark_buffer.cc index 9eb32b1815ee..f09f9fb3cc94 100644 --- a/source/common/buffer/watermark_buffer.cc +++ b/source/common/buffer/watermark_buffer.cc @@ -40,10 +40,10 @@ void WatermarkBuffer::move(Instance& rhs, uint64_t length) { checkHighWatermark(); } -int WatermarkBuffer::read(int fd, uint64_t max_length) { - int bytes_read = OwnedImpl::read(fd, max_length); +std::tuple WatermarkBuffer::read(int fd, uint64_t max_length) { + std::tuple result = OwnedImpl::read(fd, max_length); checkHighWatermark(); - return bytes_read; + return result; } uint64_t WatermarkBuffer::reserve(uint64_t length, RawSlice* iovecs, uint64_t num_iovecs) { @@ -52,10 +52,10 @@ uint64_t WatermarkBuffer::reserve(uint64_t length, RawSlice* iovecs, uint64_t nu return bytes_reserved; } -int WatermarkBuffer::write(int fd) { - int bytes_written = OwnedImpl::write(fd); +std::tuple WatermarkBuffer::write(int fd) { + std::tuple result = OwnedImpl::write(fd); checkLowWatermark(); - return bytes_written; + return result; } void WatermarkBuffer::setWatermarks(uint32_t low_watermark, uint32_t high_watermark) { diff --git a/source/common/buffer/watermark_buffer.h b/source/common/buffer/watermark_buffer.h index 5be55409ef1e..9524530c23b9 100644 --- a/source/common/buffer/watermark_buffer.h +++ b/source/common/buffer/watermark_buffer.h @@ -28,9 +28,9 @@ class WatermarkBuffer : public OwnedImpl { void drain(uint64_t size) override; void move(Instance& rhs) override; void move(Instance& rhs, uint64_t length) override; - int read(int fd, uint64_t max_length) override; + std::tuple read(int fd, uint64_t max_length) override; uint64_t reserve(uint64_t length, RawSlice* iovecs, uint64_t num_iovecs) override; - int write(int fd) override; + std::tuple write(int fd) override; void postProcess() override { checkLowWatermark(); } void setWatermarks(uint32_t watermark) { setWatermarks(watermark / 2, watermark); } diff --git a/source/common/network/raw_buffer_socket.cc b/source/common/network/raw_buffer_socket.cc index 56f0584b1d60..0ab70d60ea6c 100644 --- a/source/common/network/raw_buffer_socket.cc +++ b/source/common/network/raw_buffer_socket.cc @@ -17,8 +17,9 @@ IoResult RawBufferSocket::doRead(Buffer::Instance& buffer) { bool end_stream = false; do { // 16K read is arbitrary. TODO(mattklein123) PERF: Tune the read size. - int rc = buffer.read(callbacks_->fd(), 16384); - const int error = errno; // Latch errno before any logging calls can overwrite it. + std::tuple result = buffer.read(callbacks_->fd(), 16384); + const int rc = std::get<0>(result); + const int error = std::get<1>(result); ENVOY_CONN_LOG(trace, "read returns: {}", callbacks_->connection(), rc); if (rc == 0) { @@ -60,8 +61,9 @@ IoResult RawBufferSocket::doWrite(Buffer::Instance& buffer, bool end_stream) { action = PostIoAction::KeepOpen; break; } - int rc = buffer.write(callbacks_->fd()); - const int error = errno; // Latch errno before any logging calls can overwrite it. + std::tuple result = buffer.write(callbacks_->fd()); + const int rc = std::get<0>(result); + const int error = std::get<1>(result); ENVOY_CONN_LOG(trace, "write returns: {}", callbacks_->connection(), rc); if (rc == -1) { ENVOY_CONN_LOG(trace, "write error: {} ({})", callbacks_->connection(), error, diff --git a/source/extensions/filters/network/thrift_proxy/buffer_helper.h b/source/extensions/filters/network/thrift_proxy/buffer_helper.h index 8efbebf1e5cf..8eb633a0140a 100644 --- a/source/extensions/filters/network/thrift_proxy/buffer_helper.h +++ b/source/extensions/filters/network/thrift_proxy/buffer_helper.h @@ -48,12 +48,12 @@ class BufferWrapper : public Buffer::Instance { } void move(Buffer::Instance&) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } void move(Buffer::Instance&, uint64_t) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } - int read(int, uint64_t) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } + std::tuple read(int, uint64_t) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } uint64_t reserve(uint64_t, Buffer::RawSlice*, uint64_t) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } ssize_t search(const void*, uint64_t, size_t) const override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } - int write(int) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } + std::tuple write(int) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } std::string toString() const override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } private: diff --git a/test/common/buffer/owned_impl_test.cc b/test/common/buffer/owned_impl_test.cc index 9977ea5112c9..b48dc4bb01a1 100644 --- a/test/common/buffer/owned_impl_test.cc +++ b/test/common/buffer/owned_impl_test.cc @@ -83,34 +83,34 @@ TEST_F(OwnedImplTest, Write) { Buffer::OwnedImpl buffer; buffer.add("example"); EXPECT_CALL(os_sys_calls, writev(_, _, _)).WillOnce(Return(7)); - int rc = buffer.write(-1); - EXPECT_EQ(7, rc); + std::tuple result = buffer.write(-1); + EXPECT_EQ(7, std::get<0>(result)); EXPECT_EQ(0, buffer.length()); buffer.add("example"); EXPECT_CALL(os_sys_calls, writev(_, _, _)).WillOnce(Return(6)); - rc = buffer.write(-1); - EXPECT_EQ(6, rc); + result = buffer.write(-1); + EXPECT_EQ(6, std::get<0>(result)); EXPECT_EQ(1, buffer.length()); EXPECT_CALL(os_sys_calls, writev(_, _, _)).WillOnce(Return(0)); - rc = buffer.write(-1); - EXPECT_EQ(0, rc); + result = buffer.write(-1); + EXPECT_EQ(0, std::get<0>(result)); EXPECT_EQ(1, buffer.length()); EXPECT_CALL(os_sys_calls, writev(_, _, _)).WillOnce(Return(-1)); - rc = buffer.write(-1); - EXPECT_EQ(-1, rc); + result = buffer.write(-1); + EXPECT_EQ(-1, std::get<0>(result)); EXPECT_EQ(1, buffer.length()); EXPECT_CALL(os_sys_calls, writev(_, _, _)).WillOnce(Return(1)); - rc = buffer.write(-1); - EXPECT_EQ(1, rc); + result = buffer.write(-1); + EXPECT_EQ(1, std::get<0>(result)); EXPECT_EQ(0, buffer.length()); EXPECT_CALL(os_sys_calls, writev(_, _, _)).Times(0); - rc = buffer.write(-1); - EXPECT_EQ(0, rc); + result = buffer.write(-1); + EXPECT_EQ(0, std::get<0>(result)); EXPECT_EQ(0, buffer.length()); } @@ -120,18 +120,18 @@ TEST_F(OwnedImplTest, Read) { Buffer::OwnedImpl buffer; EXPECT_CALL(os_sys_calls, readv(_, _, _)).WillOnce(Return(0)); - int rc = buffer.read(-1, 100); - EXPECT_EQ(0, rc); + std::tuple result = buffer.read(-1, 100); + EXPECT_EQ(0, std::get<0>(result)); EXPECT_EQ(0, buffer.length()); EXPECT_CALL(os_sys_calls, readv(_, _, _)).WillOnce(Return(-1)); - rc = buffer.read(-1, 100); - EXPECT_EQ(-1, rc); + result = buffer.read(-1, 100); + EXPECT_EQ(-1, std::get<0>(result)); EXPECT_EQ(0, buffer.length()); EXPECT_CALL(os_sys_calls, readv(_, _, _)).Times(0); - rc = buffer.read(-1, 0); - EXPECT_EQ(0, rc); + result = buffer.read(-1, 0); + EXPECT_EQ(0, std::get<0>(result)); EXPECT_EQ(0, buffer.length()); } diff --git a/test/common/buffer/watermark_buffer_test.cc b/test/common/buffer/watermark_buffer_test.cc index 1514de59a15a..4fe667ee5e3d 100644 --- a/test/common/buffer/watermark_buffer_test.cc +++ b/test/common/buffer/watermark_buffer_test.cc @@ -131,7 +131,8 @@ TEST_F(WatermarkBufferTest, WatermarkFdFunctions) { int bytes_written_total = 0; while (bytes_written_total < 20) { - int bytes_written = buffer_.write(pipe_fds[1]); + std::tuple result = buffer_.write(pipe_fds[1]); + int bytes_written = std::get<0>(result); if (bytes_written < 0) { ASSERT_EQ(EAGAIN, errno); } else { @@ -144,7 +145,8 @@ TEST_F(WatermarkBufferTest, WatermarkFdFunctions) { int bytes_read_total = 0; while (bytes_read_total < 20) { - bytes_read_total += buffer_.read(pipe_fds[0], 20); + std::tuple result = buffer_.read(pipe_fds[0], 20); + bytes_read_total += std::get<0>(result); } EXPECT_EQ(2, times_high_watermark_called_); EXPECT_EQ(20, buffer_.length()); diff --git a/test/common/network/connection_impl_test.cc b/test/common/network/connection_impl_test.cc index d19c552b91e6..1258534038c2 100644 --- a/test/common/network/connection_impl_test.cc +++ b/test/common/network/connection_impl_test.cc @@ -678,7 +678,7 @@ TEST_P(ConnectionImplTest, WriteWithWatermarks) { EXPECT_CALL(*client_write_buffer_, move(_)) .WillRepeatedly(DoAll(AddBufferToStringWithoutDraining(&data_written), Invoke(client_write_buffer_, &MockWatermarkBuffer::baseMove))); - EXPECT_CALL(*client_write_buffer_, write(_)).WillOnce(Invoke([&](int fd) -> int { + EXPECT_CALL(*client_write_buffer_, write(_)).WillOnce(Invoke([&](int fd) -> std::tuple { dispatcher_->exit(); return client_write_buffer_->failWrite(fd); })); @@ -764,7 +764,7 @@ TEST_P(ConnectionImplTest, WatermarkFuzzing) { .WillOnce(Invoke(client_write_buffer_, &MockWatermarkBuffer::baseMove)); EXPECT_CALL(*client_write_buffer_, write(_)) .WillOnce(DoAll(Invoke([&](int) -> void { client_write_buffer_->drain(bytes_to_flush); }), - Return(bytes_to_flush))) + Return(std::make_tuple(bytes_to_flush, 0)))) .WillRepeatedly(testing::Invoke(client_write_buffer_, &MockWatermarkBuffer::failWrite)); client_connection_->write(buffer_to_write, false); dispatcher_->run(Event::Dispatcher::RunType::NonBlock); diff --git a/test/mocks/buffer/mocks.h b/test/mocks/buffer/mocks.h index cc0182374391..51d503dd67b6 100644 --- a/test/mocks/buffer/mocks.h +++ b/test/mocks/buffer/mocks.h @@ -16,7 +16,7 @@ template class MockBufferBase : public BaseClass { MockBufferBase(); MockBufferBase(std::function below_low, std::function above_high); - MOCK_METHOD1(write, int(int fd)); + MOCK_METHOD1(write, std::tuple(int fd)); MOCK_METHOD1(move, void(Buffer::Instance& rhs)); MOCK_METHOD2(move, void(Buffer::Instance& rhs, uint64_t length)); MOCK_METHOD1(drain, void(uint64_t size)); @@ -24,12 +24,13 @@ template class MockBufferBase : public BaseClass { void baseMove(Buffer::Instance& rhs) { BaseClass::move(rhs); } void baseDrain(uint64_t size) { BaseClass::drain(size); } - int trackWrites(int fd) { - int bytes_written = BaseClass::write(fd); + std::tuple trackWrites(int fd) { + std::tuple result = BaseClass::write(fd); + int bytes_written = std::get<0>(result); if (bytes_written > 0) { bytes_written_ += bytes_written; } - return bytes_written; + return result; } void trackDrains(uint64_t size) { @@ -38,9 +39,9 @@ template class MockBufferBase : public BaseClass { } // A convenience function to invoke on write() which fails the write with EAGAIN. - int failWrite(int) { + std::tuple failWrite(int) { errno = EAGAIN; - return -1; + return std::make_tuple(-1, errno); } int bytes_written() const { return bytes_written_; } From 36d2f52b2c396ddc47d3f74633e7d674be55e664 Mon Sep 17 00:00:00 2001 From: Venil Noronha Date: Wed, 18 Jul 2018 10:33:34 -0700 Subject: [PATCH 2/3] Address review comments * Update API comments * Remove explicit errno setting in mock Signed-off-by: Venil Noronha --- include/envoy/buffer/buffer.h | 6 ++++-- test/mocks/buffer/mocks.h | 5 +---- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/include/envoy/buffer/buffer.h b/include/envoy/buffer/buffer.h index 39157d425ab4..164f4374bf9c 100644 --- a/include/envoy/buffer/buffer.h +++ b/include/envoy/buffer/buffer.h @@ -143,7 +143,8 @@ class Instance { * Read from a file descriptor directly into the buffer. * @param fd supplies the descriptor to read from. * @param max_length supplies the maximum length to read. - * @return a tuple with the number of bytes read or -1 if there was an error, and the errno. + * @return a tuple with the number of bytes read and the errno. If an error occurs, the number + * of bytes read would indicate -1 and the errno would be non-zero. */ virtual std::tuple read(int fd, uint64_t max_length) PURE; @@ -174,7 +175,8 @@ class Instance { /** * Write the buffer out to a file descriptor. * @param fd supplies the descriptor to write to. - * @return a tuple with the number of bytes written or -1 if there was an error, and the errno. + * @return a tuple with the number of bytes written and the errno. If an error occurs, the + * number of bytes written would indicate -1 and the errno would be non-zero. */ virtual std::tuple write(int fd) PURE; }; diff --git a/test/mocks/buffer/mocks.h b/test/mocks/buffer/mocks.h index 51d503dd67b6..2be1903c621c 100644 --- a/test/mocks/buffer/mocks.h +++ b/test/mocks/buffer/mocks.h @@ -39,10 +39,7 @@ template class MockBufferBase : public BaseClass { } // A convenience function to invoke on write() which fails the write with EAGAIN. - std::tuple failWrite(int) { - errno = EAGAIN; - return std::make_tuple(-1, errno); - } + std::tuple failWrite(int) { return std::make_tuple(-1, EAGAIN); } int bytes_written() const { return bytes_written_; } uint64_t bytes_drained() const { return bytes_drained_; } From e5c8ed1e29332bb679025bafe55d396c5c2cdfdb Mon Sep 17 00:00:00 2001 From: Venil Noronha Date: Wed, 18 Jul 2018 11:21:51 -0700 Subject: [PATCH 3/3] Update buffer API comments Signed-off-by: Venil Noronha --- include/envoy/buffer/buffer.h | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/include/envoy/buffer/buffer.h b/include/envoy/buffer/buffer.h index 164f4374bf9c..eb4d3fc0f400 100644 --- a/include/envoy/buffer/buffer.h +++ b/include/envoy/buffer/buffer.h @@ -143,8 +143,9 @@ class Instance { * Read from a file descriptor directly into the buffer. * @param fd supplies the descriptor to read from. * @param max_length supplies the maximum length to read. - * @return a tuple with the number of bytes read and the errno. If an error occurs, the number - * of bytes read would indicate -1 and the errno would be non-zero. + * @return a tuple with the number of bytes read and the errno. If an error occurred, the + * number of bytes read would indicate -1 and the errno would be non-zero. Otherwise, if + * bytes were read, errno shouldn't be used. */ virtual std::tuple read(int fd, uint64_t max_length) PURE; @@ -175,8 +176,9 @@ class Instance { /** * Write the buffer out to a file descriptor. * @param fd supplies the descriptor to write to. - * @return a tuple with the number of bytes written and the errno. If an error occurs, the - * number of bytes written would indicate -1 and the errno would be non-zero. + * @return a tuple with the number of bytes written and the errno. If an error occurred, the + * number of bytes written would indicate -1 and the errno would be non-zero. Otherwise, if + * bytes were written, errno shouldn't be used. */ virtual std::tuple write(int fd) PURE; };