Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tls: simplify write mechanism #17883

Closed
wants to merge 2 commits into from
Closed

tls: simplify write mechanism #17883

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d5c3cfe
tls: refactor write queues away
addaleax Dec 27, 2017
4cbd571
tls: remove cleartext input data queue
addaleax Dec 27, 2017
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
81 changes: 33 additions & 48 deletions src/tls_wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@ TLSWrap::TLSWrap(Environment* env,
stream_(stream),
enc_in_(nullptr),
enc_out_(nullptr),
clear_in_(nullptr),
write_size_(0),
started_(false),
established_(false),
Expand Down Expand Up @@ -95,8 +94,6 @@ TLSWrap::TLSWrap(Environment* env,
TLSWrap::~TLSWrap() {
enc_in_ = nullptr;
enc_out_ = nullptr;
delete clear_in_;
clear_in_ = nullptr;

sc_ = nullptr;

Expand All @@ -119,21 +116,14 @@ TLSWrap::~TLSWrap() {
}


void TLSWrap::MakePending() {
write_item_queue_.MoveBack(&pending_write_items_);
}


bool TLSWrap::InvokeQueued(int status, const char* error_str) {
if (pending_write_items_.IsEmpty())
if (!write_callback_scheduled_)
return false;

// Process old queue
WriteItemList queue;
pending_write_items_.MoveBack(&queue);
while (WriteItem* wi = queue.PopFront()) {
wi->w_->Done(status, error_str);
delete wi;
if (current_write_ != nullptr) {
WriteWrap* w = current_write_;
current_write_ = nullptr;
w->Done(status, error_str);
}

return true;
Expand Down Expand Up @@ -185,10 +175,6 @@ void TLSWrap::InitSSL() {
// Unexpected
ABORT();
}

// Initialize ring for queud clear data
clear_in_ = new crypto::NodeBIO();
clear_in_->AssignEnvironment(env());
}


Expand Down Expand Up @@ -303,15 +289,15 @@ void TLSWrap::EncOut() {
return;

// Split-off queue
if (established_ && !write_item_queue_.IsEmpty())
MakePending();
if (established_ && current_write_ != nullptr)
write_callback_scheduled_ = true;

if (ssl_ == nullptr)
return;

// No data to write
if (BIO_pending(enc_out_) == 0) {
if (clear_in_->Length() == 0)
if (pending_cleartext_input_.empty())
InvokeQueued(0);
return;
}
Expand Down Expand Up @@ -498,21 +484,24 @@ bool TLSWrap::ClearIn() {
if (ssl_ == nullptr)
return false;

std::vector<uv_buf_t> buffers;
buffers.swap(pending_cleartext_input_);

crypto::MarkPopErrorOnReturn mark_pop_error_on_return;

size_t i;
int written = 0;
while (clear_in_->Length() > 0) {
size_t avail = 0;
char* data = clear_in_->Peek(&avail);
for (i = 0; i < buffers.size(); ++i) {
size_t avail = buffers[i].len;
char* data = buffers[i].base;
written = SSL_write(ssl_, data, avail);
CHECK(written == -1 || written == static_cast<int>(avail));
if (written == -1)
break;
clear_in_->Read(nullptr, avail);
}

// All written
if (clear_in_->Length() == 0) {
if (i == buffers.size()) {
CHECK_GE(written, 0);
return true;
}
Expand All @@ -522,9 +511,15 @@ bool TLSWrap::ClearIn() {
std::string error_str;
Local<Value> arg = GetSSLError(written, &err, &error_str);
if (!arg.IsEmpty()) {
MakePending();
write_callback_scheduled_ = true;
InvokeQueued(UV_EPROTO, error_str.c_str());
clear_in_->Reset();
} else {
// Push back the not-yet-written pending buffers into their queue.
// This can be skipped in the error case because no further writes
// would succeed anyway.
pending_cleartext_input_.insert(pending_cleartext_input_.end(),
&buffers[i],
&buffers[buffers.size()]);
}

return false;
Expand Down Expand Up @@ -606,8 +601,9 @@ int TLSWrap::DoWrite(WriteWrap* w,
}
}

// Queue callback to execute it on next tick
write_item_queue_.PushBack(new WriteItem(w));
// Store the current write wrap
CHECK_EQ(current_write_, nullptr);
current_write_ = w;
w->Dispatched();

// Write queued data
Expand All @@ -616,14 +612,6 @@ int TLSWrap::DoWrite(WriteWrap* w,
return 0;
}

// Process enqueued data first
if (!ClearIn()) {
// If there're still data to process - enqueue current one
for (i = 0; i < count; i++)
clear_in_->Write(bufs[i].base, bufs[i].len);
return 0;
}

if (ssl_ == nullptr) {
ClearError();
error_ = "Write after DestroySSL";
Expand All @@ -646,9 +634,9 @@ int TLSWrap::DoWrite(WriteWrap* w,
if (!arg.IsEmpty())
return UV_EPROTO;

// No errors, queue rest
for (; i < count; i++)
clear_in_->Write(bufs[i].base, bufs[i].len);
pending_cleartext_input_.insert(pending_cleartext_input_.end(),
&bufs[i],
&bufs[count]);
}

// Try writing data immediately
Expand Down Expand Up @@ -818,17 +806,14 @@ void TLSWrap::DestroySSL(const FunctionCallbackInfo<Value>& args) {
TLSWrap* wrap;
ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder());

// Move all writes to pending
wrap->MakePending();
// If there is a write happening, mark it as finished.
wrap->write_callback_scheduled_ = true;

// And destroy
wrap->InvokeQueued(UV_ECANCELED, "Canceled because of SSL destruction");

// Destroy the SSL structure and friends
wrap->SSLWrap<TLSWrap>::DestroySSL();

delete wrap->clear_in_;
wrap->clear_in_ = nullptr;
}


Expand Down Expand Up @@ -928,7 +913,7 @@ void TLSWrap::GetWriteQueueSize(const FunctionCallbackInfo<Value>& info) {
TLSWrap* wrap;
ASSIGN_OR_RETURN_UNWRAP(&wrap, info.This());

if (wrap->clear_in_ == nullptr) {
if (wrap->ssl_ == nullptr) {
info.GetReturnValue().Set(0);
return;
}
Expand Down
21 changes: 3 additions & 18 deletions src/tls_wrap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,19 +90,6 @@ class TLSWrap : public AsyncWrap,
// Maximum number of buffers passed to uv_write()
static const int kSimultaneousBufferCount = 10;

// Write callback queue's item
class WriteItem {
public:
explicit WriteItem(WriteWrap* w) : w_(w) {
}
~WriteItem() {
w_ = nullptr;
}

WriteWrap* w_;
ListNode<WriteItem> member_;
};

TLSWrap(Environment* env,
Kind kind,
StreamBase* stream,
Expand All @@ -114,7 +101,6 @@ class TLSWrap : public AsyncWrap,
void EncOutAfterWrite(WriteWrap* req_wrap, int status);
bool ClearIn();
void ClearOut();
void MakePending();
bool InvokeQueued(int status, const char* error_str = nullptr);

inline void Cycle() {
Expand Down Expand Up @@ -171,11 +157,10 @@ class TLSWrap : public AsyncWrap,
StreamBase* stream_;
BIO* enc_in_;
BIO* enc_out_;
crypto::NodeBIO* clear_in_;
std::vector<uv_buf_t> pending_cleartext_input_;
Copy link
Member

Choose a reason for hiding this comment

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

Would a std::list be more appropriate here?

Copy link
Member Author

Choose a reason for hiding this comment

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

@TimothyGu Why? uv_buf_t is a pretty small structure, so what I would guess is that the overhead of possibly having a small number of them being unused is more or less negligible compared to allocating a list node for every buffer?

Copy link
Member

Choose a reason for hiding this comment

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

based on the variations I tried with http2, the performance difference is inconsequential. Using std::vector here is just fine.

size_t write_size_;
typedef ListHead<WriteItem, &WriteItem::member_> WriteItemList;
WriteItemList write_item_queue_;
WriteItemList pending_write_items_;
WriteWrap* current_write_ = nullptr;
bool write_callback_scheduled_ = false;
bool started_;
bool established_;
bool shutdown_;
Expand Down
13 changes: 0 additions & 13 deletions src/util-inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,19 +99,6 @@ ListHead<T, M>::~ListHead() {
head_.next_->Remove();
}

template <typename T, ListNode<T> (T::*M)>
void ListHead<T, M>::MoveBack(ListHead* that) {
if (IsEmpty())
return;
ListNode<T>* to = &that->head_;
head_.next_->prev_ = to->prev_;
to->prev_->next_ = head_.next_;
head_.prev_->next_ = to;
to->prev_ = head_.prev_;
head_.prev_ = &head_;
head_.next_ = &head_;
}

template <typename T, ListNode<T> (T::*M)>
void ListHead<T, M>::PushBack(T* element) {
ListNode<T>* that = &(element->*M);
Expand Down
1 change: 0 additions & 1 deletion src/util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,6 @@ class ListHead {

inline ListHead() = default;
inline ~ListHead();
inline void MoveBack(ListHead* that);
inline void PushBack(T* element);
inline void PushFront(T* element);
inline bool IsEmpty() const;
Expand Down