Skip to content

Commit

Permalink
src,stream: improve DoWrite() and Write()
Browse files Browse the repository at this point in the history
Clarify StreamResource::DoWrite() interface definition.
Fix error-prone Http2Stream::DoWrite().
Change StreamBase::Write() to allow caller to avoid
inefficient write behavior.

PR-URL: #44434
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
  • Loading branch information
ywave620 authored and danielleadams committed Oct 11, 2022
1 parent 99a2c16 commit 281fd7a
Show file tree
Hide file tree
Showing 4 changed files with 26 additions and 18 deletions.
3 changes: 1 addition & 2 deletions src/node_http2.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2375,8 +2375,7 @@ int Http2Stream::DoWrite(WriteWrap* req_wrap,
CHECK_NULL(send_handle);
Http2Scope h2scope(this);
if (!is_writable() || is_destroyed()) {
req_wrap->Done(UV_EOF);
return 0;
return UV_EOF;
}
Debug(this, "queuing %d buffers to send", nbufs);
for (size_t i = 0; i < nbufs; ++i) {
Expand Down
12 changes: 6 additions & 6 deletions src/stream_base-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -160,11 +160,11 @@ int StreamBase::Shutdown(v8::Local<v8::Object> req_wrap_obj) {
return err;
}

StreamWriteResult StreamBase::Write(
uv_buf_t* bufs,
size_t count,
uv_stream_t* send_handle,
v8::Local<v8::Object> req_wrap_obj) {
StreamWriteResult StreamBase::Write(uv_buf_t* bufs,
size_t count,
uv_stream_t* send_handle,
v8::Local<v8::Object> req_wrap_obj,
bool skip_try_write) {
Environment* env = stream_env();
int err;

Expand All @@ -173,7 +173,7 @@ StreamWriteResult StreamBase::Write(
total_bytes += bufs[i].len;
bytes_written_ += total_bytes;

if (send_handle == nullptr) {
if (send_handle == nullptr && !skip_try_write) {
err = DoTryWrite(&bufs, &count);
if (err != 0 || count == 0) {
return StreamWriteResult { false, err, nullptr, total_bytes, {} };
Expand Down
2 changes: 1 addition & 1 deletion src/stream_base.cc
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,7 @@ int StreamBase::WriteString(const FunctionCallbackInfo<Value>& args) {
}
}

StreamWriteResult res = Write(&buf, 1, send_handle, req_wrap_obj);
StreamWriteResult res = Write(&buf, 1, send_handle, req_wrap_obj, try_write);
res.bytes += synchronously_written;

SetWriteResult(res);
Expand Down
27 changes: 18 additions & 9 deletions src/stream_base.h
Original file line number Diff line number Diff line change
Expand Up @@ -244,14 +244,19 @@ class StreamResource {
// `*bufs` and `*count` accordingly. This is a no-op by default.
// Return 0 for success and a libuv error code for failures.
virtual int DoTryWrite(uv_buf_t** bufs, size_t* count);
// Initiate a write of data. If the write completes synchronously, return 0 on
// success (with bufs modified to indicate how much data was consumed) or a
// libuv error code on failure. If the write will complete asynchronously,
// return 0. When the write completes asynchronously, call req_wrap->Done()
// with 0 on success (with bufs modified to indicate how much data was
// consumed) or a libuv error code on failure. Do not call req_wrap->Done() if
// the write completes synchronously, that is, it should never be called
// before DoWrite() has returned.
// Initiate a write of data.
// Upon an immediate failure, a libuv error code is returned,
// w->Done() will never be called and caller should free `bufs`.
// Otherwise, 0 is returned and w->Done(status) will be called
// with status set to either
// (1) 0 after all data are written, or
// (2) a libuv error code when an error occurs
// in either case, w->Done() will never be called before DoWrite() returns.
// When 0 is returned:
// (1) memory specified by `bufs` and `count` must remain valid until
// w->Done() gets called.
// (2) `bufs` might or might not be changed, caller should not rely on this.
// (3) `bufs` should be freed after w->Done() gets called.
virtual int DoWrite(WriteWrap* w,
uv_buf_t* bufs,
size_t count,
Expand Down Expand Up @@ -343,13 +348,17 @@ class StreamBase : public StreamResource {
// WriteWrap object (that was created in JS), or a new one will be created.
// This will first try to write synchronously using `DoTryWrite()`, then
// asynchronously using `DoWrite()`.
// Caller can pass `skip_try_write` as true if it has already called
// `DoTryWrite()` and ends up with a partial write, or it knows that the
// write is too large to finish synchronously.
// If the return value indicates a synchronous completion, no callback will
// be invoked.
inline StreamWriteResult Write(
uv_buf_t* bufs,
size_t count,
uv_stream_t* send_handle = nullptr,
v8::Local<v8::Object> req_wrap_obj = v8::Local<v8::Object>());
v8::Local<v8::Object> req_wrap_obj = v8::Local<v8::Object>(),
bool skip_try_write = false);

// These can be overridden by subclasses to get more specific wrap instances.
// For example, a subclass Foo could create a FooWriteWrap or FooShutdownWrap
Expand Down

0 comments on commit 281fd7a

Please sign in to comment.