Skip to content
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 13 additions & 31 deletions include/envoy/api/io_error.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,6 @@
namespace Envoy {
namespace Api {

class IoError;

// IoErrorCode::Again is used frequently. Define it to be a distinguishable address to avoid
// frequent memory allocation of IoError instance.
// If this is used, IoCallResult has to be instantiated with a deleter that does not
// deallocate memory for this error.
// TODO: This is probably not the best way to avoid allocations in the case of
// EAGAIN. This will be fixed as a part of #6037.
#define ENVOY_ERROR_AGAIN reinterpret_cast<Api::IoError*>(0x01)

/**
* Base class for any I/O error.
*/
Expand All @@ -27,6 +17,8 @@ class IoError {
enum class IoErrorCode {
// No data available right now, try again later.
Again,
// The IoHandle is in invalid state.
BadHandle,
// Not supported.
NoSupport,
// Address family not supported.
Expand All @@ -35,32 +27,13 @@ class IoError {
InProgress,
// Permission denied.
Permission,
// Bad handle
BadHandle,
// Other error codes cannot be mapped to any one above in getErrorCode().
UnknownError
};
virtual ~IoError() {}

// Map platform specific error into IoErrorCode.
// Needed to hide errorCode() in case of ENVOY_ERROR_AGAIN.
static IoErrorCode getErrorCode(const IoError& err) {
if (&err == ENVOY_ERROR_AGAIN) {
return IoErrorCode::Again;
}
return err.errorCode();
}

static std::string getErrorDetails(const IoError& err) {
if (&err == ENVOY_ERROR_AGAIN) {
return "Try again later";
}
return err.errorDetails();
}

protected:
virtual IoErrorCode errorCode() const PURE;
virtual std::string errorDetails() const PURE;
virtual IoErrorCode getErrorCode() const PURE;
virtual std::string getErrorDetails() const PURE;
};

using IoErrorDeleterType = void (*)(IoError*);
Expand All @@ -69,6 +42,9 @@ using IoErrorPtr = std::unique_ptr<IoError, IoErrorDeleterType>;
/**
* Basic type for return result which has a return code and error code defined
* according to different implementations.
* If the call succeeds, |err_| is nullptr and |rc_| is valid. Otherwise |err_|
* can be passed into IoError::getErrorCode() to extract the error. In this
* case, |rc_| is invalid.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given this interface can you add method:

 bool ok() const { return err_ == nullptr; }

I had to also read below at usages to understand that rc is really the interface-dependent return value. Can you call it return_value_ instead of rc_ (which sounds like it might be errno or something), and use ReturnValue rather than T as your template param?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and of course use that method at call-sites rather than directly dereferencing err_.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given this interface can you add method:

 bool ok() const { return err_ == nullptr; }

I had to also read below at usages to understand that rc is really the interface-dependent return value. Can you call it return_value_ instead of rc_ (which sounds like it might be errno or something), and use ReturnValue rather than T as your template param?

This renaming will touch even larger scope of files which is not related to socket read/write, since IoError is used in filesystem now. I would prefer do it separately so that this PR doesn't looks confusing.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add the ok() method now and change usage in files already in this PR, and follow up with the file-system changes later?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding ok() sounds good to me

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

*/
template <typename T> struct IoCallResult {
IoCallResult(T rc, IoErrorPtr err) : rc_(rc), err_(std::move(err)) {}
Expand All @@ -77,6 +53,12 @@ template <typename T> struct IoCallResult {

virtual ~IoCallResult() {}

IoCallResult& operator=(IoCallResult&& result) {
rc_ = result.rc_;
err_ = std::move(result.err_);
return *this;
}

T rc_;
IoErrorPtr err_;
};
Expand Down
1 change: 1 addition & 0 deletions include/envoy/buffer/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ envoy_cc_library(
hdrs = ["buffer.h"],
deps = [
"//include/envoy/api:os_sys_calls_interface",
"//include/envoy/network:io_handle_interface",
"//source/common/common:byte_order_lib",
],
)
18 changes: 10 additions & 8 deletions include/envoy/buffer/buffer.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include "envoy/api/os_sys_calls.h"
#include "envoy/common/exception.h"
#include "envoy/common/pure.h"
#include "envoy/network/io_handle.h"

#include "common/common/byte_order.h"

Expand Down Expand Up @@ -165,12 +166,12 @@ class Instance {

/**
* Read from a file descriptor directly into the buffer.
* @param fd supplies the descriptor to read from.
* @param io_handle supplies the io handle to read from.
* @param max_length supplies the maximum length to read.
* @return a Api::SysCallIntResult with rc_ = the number of bytes read if successful, or rc_ = -1
* for failure. If the call is successful, errno_ shouldn't be used.
* @return a IoCallUintResult with err_ = nullptr and rc_ = the number of bytes
* read if successful, or err_ = some IoError for failure. If call failed, rc_ shouldn't be used.
*/
virtual Api::SysCallIntResult read(int fd, uint64_t max_length) PURE;
virtual Api::IoCallUintResult read(Network::IoHandle& io_handle, uint64_t max_length) PURE;

/**
* Reserve space in the buffer.
Expand Down Expand Up @@ -198,11 +199,12 @@ class Instance {

/**
* Write the buffer out to a file descriptor.
* @param fd supplies the descriptor to write to.
* @return a Api::SysCallIntResult with rc_ = the number of bytes written if successful, or rc_ =
* -1 for failure. If the call is successful, errno_ shouldn't be used.
* @param io_handle supplies the io_handle to write to.
* @return a IoCallUintResult with err_ = nullptr and rc_ = the number of bytes
* written if successful, or err_ = some IoError for failure. If call failed, rc_ shouldn't be
* used.
*/
virtual Api::SysCallIntResult write(int fd) PURE;
virtual Api::IoCallUintResult write(Network::IoHandle& io_handle) PURE;

/**
* Copy an integer out of the buffer.
Expand Down
14 changes: 12 additions & 2 deletions include/envoy/network/io_handle.h
Original file line number Diff line number Diff line change
@@ -1,12 +1,17 @@
#pragma once

#include <memory>

#include "envoy/api/io_error.h"
#include "envoy/common/pure.h"

namespace Envoy {
namespace Buffer {
struct RawSlice;
} // namespace Buffer

namespace Network {
namespace Address {
class Instance;
} // namespace Address

/**
* IoHandle: an abstract interface for all I/O operations
Expand All @@ -32,6 +37,11 @@ class IoHandle {
* Return true if close() hasn't been called.
*/
virtual bool isOpen() const PURE;

virtual Api::IoCallUintResult readv(uint64_t max_length, Buffer::RawSlice* slices,
uint64_t num_slice) PURE;

virtual Api::IoCallUintResult writev(const Buffer::RawSlice* slices, uint64_t num_slice) PURE;
};

typedef std::unique_ptr<IoHandle> IoHandlePtr;
Expand Down
1 change: 0 additions & 1 deletion source/common/buffer/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ envoy_cc_library(
hdrs = ["buffer_impl.h"],
deps = [
"//include/envoy/buffer:buffer_interface",
"//source/common/api:os_sys_calls_lib",
"//source/common/common:non_copyable",
"//source/common/common:stack_array",
"//source/common/event:libevent_lib",
Expand Down
72 changes: 22 additions & 50 deletions source/common/buffer/buffer_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
#include <cstdint>
#include <string>

#include "common/api/os_sys_calls_impl.h"
#include "common/common/assert.h"
#include "common/common/stack_array.h"

Expand Down Expand Up @@ -124,44 +123,30 @@ void OwnedImpl::move(Instance& rhs, uint64_t length) {
static_cast<LibEventInstance&>(rhs).postProcess();
}

Api::SysCallIntResult OwnedImpl::read(int fd, uint64_t max_length) {
Api::IoCallUintResult OwnedImpl::read(Network::IoHandle& io_handle, uint64_t max_length) {
if (max_length == 0) {
return {0, 0};
return Api::IoCallUintResult(0, Api::IoErrorPtr(nullptr, [](Api::IoError*) {}));
}
constexpr uint64_t MaxSlices = 2;
RawSlice slices[MaxSlices];
const uint64_t num_slices = reserve(max_length, slices, MaxSlices);
STACK_ARRAY(iov, iovec, num_slices);
uint64_t num_slices_to_read = 0;
uint64_t num_bytes_to_read = 0;
for (; num_slices_to_read < num_slices && num_bytes_to_read < max_length; num_slices_to_read++) {
iov[num_slices_to_read].iov_base = slices[num_slices_to_read].mem_;
const size_t slice_length = std::min(slices[num_slices_to_read].len_,
static_cast<size_t>(max_length - num_bytes_to_read));
iov[num_slices_to_read].iov_len = slice_length;
num_bytes_to_read += slice_length;
}
ASSERT(num_slices_to_read <= MaxSlices);
ASSERT(num_bytes_to_read <= max_length);
auto& os_syscalls = Api::OsSysCallsSingleton::get();
const Api::SysCallSizeResult result =
os_syscalls.readv(fd, iov.begin(), static_cast<int>(num_slices_to_read));
if (result.rc_ < 0) {
return {static_cast<int>(result.rc_), result.errno_};
}
uint64_t num_slices_to_commit = 0;
uint64_t bytes_to_commit = result.rc_;
ASSERT(bytes_to_commit <= max_length);
while (bytes_to_commit != 0) {
slices[num_slices_to_commit].len_ =
std::min(slices[num_slices_to_commit].len_, static_cast<size_t>(bytes_to_commit));
ASSERT(bytes_to_commit >= slices[num_slices_to_commit].len_);
bytes_to_commit -= slices[num_slices_to_commit].len_;
num_slices_to_commit++;
Api::IoCallUintResult result = io_handle.readv(max_length, slices, num_slices);
if (result.err_ == nullptr) {
// Read succeeded.
uint64_t num_slices_to_commit = 0;
uint64_t bytes_to_commit = result.rc_;
ASSERT(bytes_to_commit <= max_length);
while (bytes_to_commit != 0) {
slices[num_slices_to_commit].len_ =
std::min(slices[num_slices_to_commit].len_, static_cast<size_t>(bytes_to_commit));
ASSERT(bytes_to_commit >= slices[num_slices_to_commit].len_);
bytes_to_commit -= slices[num_slices_to_commit].len_;
num_slices_to_commit++;
}
ASSERT(num_slices_to_commit <= num_slices);
commit(slices, num_slices_to_commit);
}
ASSERT(num_slices_to_commit <= num_slices);
commit(slices, num_slices_to_commit);
return {static_cast<int>(result.rc_), result.errno_};
return result;
}

uint64_t OwnedImpl::reserve(uint64_t length, RawSlice* iovecs, uint64_t num_iovecs) {
Expand All @@ -184,28 +169,15 @@ ssize_t OwnedImpl::search(const void* data, uint64_t size, size_t start) const {
return result_ptr.pos;
}

Api::SysCallIntResult OwnedImpl::write(int fd) {
Api::IoCallUintResult OwnedImpl::write(Network::IoHandle& io_handle) {
constexpr uint64_t MaxSlices = 16;
RawSlice slices[MaxSlices];
const uint64_t num_slices = std::min(getRawSlices(slices, MaxSlices), MaxSlices);
STACK_ARRAY(iov, iovec, num_slices);
uint64_t num_slices_to_write = 0;
for (uint64_t i = 0; i < num_slices; i++) {
if (slices[i].mem_ != nullptr && slices[i].len_ != 0) {
iov[num_slices_to_write].iov_base = slices[i].mem_;
iov[num_slices_to_write].iov_len = slices[i].len_;
num_slices_to_write++;
}
}
if (num_slices_to_write == 0) {
return {0, 0};
}
auto& os_syscalls = Api::OsSysCallsSingleton::get();
const Api::SysCallSizeResult result = os_syscalls.writev(fd, iov.begin(), num_slices_to_write);
if (result.rc_ > 0) {
Api::IoCallUintResult result = io_handle.writev(slices, num_slices);
if (result.err_ == nullptr && result.rc_ > 0) {
drain(static_cast<uint64_t>(result.rc_));
}
return {static_cast<int>(result.rc_), result.errno_};
return result;
}

OwnedImpl::OwnedImpl() : buffer_(evbuffer_new()) {}
Expand Down
5 changes: 3 additions & 2 deletions source/common/buffer/buffer_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include <string>

#include "envoy/buffer/buffer.h"
#include "envoy/network/io_handle.h"

#include "common/common/non_copyable.h"
#include "common/event/libevent.h"
Expand Down Expand Up @@ -82,10 +83,10 @@ class OwnedImpl : public LibEventInstance {
void* linearize(uint32_t size) override;
void move(Instance& rhs) override;
void move(Instance& rhs, uint64_t length) override;
Api::SysCallIntResult read(int fd, uint64_t max_length) override;
Api::IoCallUintResult read(Network::IoHandle& io_handle, 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;
Api::SysCallIntResult write(int fd) override;
Api::IoCallUintResult write(Network::IoHandle& io_handle) override;
void postProcess() override {}
std::string toString() const override;

Expand Down
8 changes: 4 additions & 4 deletions source/common/buffer/watermark_buffer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,8 @@ void WatermarkBuffer::move(Instance& rhs, uint64_t length) {
checkHighWatermark();
}

Api::SysCallIntResult WatermarkBuffer::read(int fd, uint64_t max_length) {
Api::SysCallIntResult result = OwnedImpl::read(fd, max_length);
Api::IoCallUintResult WatermarkBuffer::read(Network::IoHandle& io_handle, uint64_t max_length) {
Api::IoCallUintResult result = OwnedImpl::read(io_handle, max_length);
checkHighWatermark();
return result;
}
Expand All @@ -62,8 +62,8 @@ uint64_t WatermarkBuffer::reserve(uint64_t length, RawSlice* iovecs, uint64_t nu
return bytes_reserved;
}

Api::SysCallIntResult WatermarkBuffer::write(int fd) {
Api::SysCallIntResult result = OwnedImpl::write(fd);
Api::IoCallUintResult WatermarkBuffer::write(Network::IoHandle& io_handle) {
Api::IoCallUintResult result = OwnedImpl::write(io_handle);
checkLowWatermark();
return result;
}
Expand Down
4 changes: 2 additions & 2 deletions source/common/buffer/watermark_buffer.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,9 @@ class WatermarkBuffer : public OwnedImpl {
void drain(uint64_t size) override;
void move(Instance& rhs) override;
void move(Instance& rhs, uint64_t length) override;
Api::SysCallIntResult read(int fd, uint64_t max_length) override;
Api::IoCallUintResult read(Network::IoHandle& io_handle, uint64_t max_length) override;
uint64_t reserve(uint64_t length, RawSlice* iovecs, uint64_t num_iovecs) override;
Api::SysCallIntResult write(int fd) override;
Api::IoCallUintResult write(Network::IoHandle& io_handle) override;
void postProcess() override { checkLowWatermark(); }

void setWatermarks(uint32_t watermark) { setWatermarks(watermark / 2, watermark); }
Expand Down
13 changes: 13 additions & 0 deletions source/common/network/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -102,14 +102,27 @@ envoy_cc_library(
],
)

envoy_cc_library(
name = "io_socket_error_lib",
srcs = ["io_socket_error_impl.cc"],
hdrs = ["io_socket_error_impl.h"],
deps = [
"//include/envoy/api:io_error_interface",
"//source/common/common:assert_lib",
],
)

envoy_cc_library(
name = "io_socket_handle_lib",
srcs = ["io_socket_handle_impl.cc"],
hdrs = ["io_socket_handle_impl.h"],
deps = [
":io_socket_error_lib",
"//include/envoy/buffer:buffer_interface",
"//include/envoy/network:io_handle_interface",
"//source/common/api:os_sys_calls_lib",
"//source/common/common:assert_lib",
"//source/common/common:stack_array",
],
)

Expand Down
32 changes: 32 additions & 0 deletions source/common/network/io_socket_error_impl.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
#include "common/network/io_socket_error_impl.h"

#include "common/common/assert.h"

namespace Envoy {
namespace Network {

Api::IoError::IoErrorCode IoSocketError::getErrorCode() const {
switch (errno_) {
case EAGAIN:
RELEASE_ASSERT(this == getIoSocketEagainInstance(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be an ASSERT right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the diff between ASSERT and RELEASE_ASSERT?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RELEASE_ASSERT is enabled in all builds, ASSERT is for fastbuild/dbg builds only.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for normal ASSERT

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

"Didn't use IoSocketEagain to represent EAGAIN.");
return IoErrorCode::Again;
case EBADF:
return IoErrorCode::BadHandle;
case ENOTSUP:
return IoErrorCode::NoSupport;
case EAFNOSUPPORT:
return IoErrorCode::AddressFamilyNoSupport;
case EINPROGRESS:
return IoErrorCode::InProgress;
case EPERM:
return IoErrorCode::Permission;
default:
return IoErrorCode::UnknownError;
}
}

std::string IoSocketError::getErrorDetails() const { return ::strerror(errno_); }

} // namespace Network
} // namespace Envoy
Loading