From 281fd7a09af1874298cdefcf41f055f17316ba75 Mon Sep 17 00:00:00 2001 From: ywave620 Date: Mon, 10 Oct 2022 19:00:23 +0800 Subject: [PATCH] src,stream: improve DoWrite() and Write() Clarify StreamResource::DoWrite() interface definition. Fix error-prone Http2Stream::DoWrite(). Change StreamBase::Write() to allow caller to avoid inefficient write behavior. PR-URL: https://github.com/nodejs/node/pull/44434 Reviewed-By: Matteo Collina Reviewed-By: Stephen Belanger --- src/node_http2.cc | 3 +-- src/stream_base-inl.h | 12 ++++++------ src/stream_base.cc | 2 +- src/stream_base.h | 27 ++++++++++++++++++--------- 4 files changed, 26 insertions(+), 18 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index 7a1e751929286d..cb44e3aec2e8e2 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -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) { diff --git a/src/stream_base-inl.h b/src/stream_base-inl.h index ef86587c0bd24e..6123dbcdac8e82 100644 --- a/src/stream_base-inl.h +++ b/src/stream_base-inl.h @@ -160,11 +160,11 @@ int StreamBase::Shutdown(v8::Local req_wrap_obj) { return err; } -StreamWriteResult StreamBase::Write( - uv_buf_t* bufs, - size_t count, - uv_stream_t* send_handle, - v8::Local req_wrap_obj) { +StreamWriteResult StreamBase::Write(uv_buf_t* bufs, + size_t count, + uv_stream_t* send_handle, + v8::Local req_wrap_obj, + bool skip_try_write) { Environment* env = stream_env(); int err; @@ -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, {} }; diff --git a/src/stream_base.cc b/src/stream_base.cc index 8701434c24fb26..389aab28c5adff 100644 --- a/src/stream_base.cc +++ b/src/stream_base.cc @@ -335,7 +335,7 @@ int StreamBase::WriteString(const FunctionCallbackInfo& 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); diff --git a/src/stream_base.h b/src/stream_base.h index 7513225362d3ec..c56508692260cc 100644 --- a/src/stream_base.h +++ b/src/stream_base.h @@ -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, @@ -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 req_wrap_obj = v8::Local()); + v8::Local req_wrap_obj = v8::Local(), + 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