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.

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

src,stream: improve DoWrite() and Write() #44434

Merged
merged 2 commits into from Oct 10, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/node_http2.cc
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);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RafaelGSS After investigation, I find out that this if statement should be turned to an assertion. Because the JS code make sure that whenever we do a write on a stream, the stream is writable and not destroyed.

Copy link
Member

Choose a reason for hiding this comment

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

Changing it to an assertion (CHECK) will crash the application whenever someone tries to write in a destroyed stream, is that expected?

Copy link
Member

Choose a reason for hiding this comment

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

no.

Copy link
Contributor Author

@ywave620 ywave620 Aug 31, 2022

Choose a reason for hiding this comment

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

JS code in /lib will not let this happens. Without misusing the internals, the case that captured by this if can not happen. If you insist keeping this if, then apparently, this function should return an error code immediately and never invoke the cb, because this is what every asynchronous API should do in case an immediate failure.

Copy link
Member

Choose a reason for hiding this comment

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

If that happens when misusing the internals, adding the CHECK make sense to me.

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
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) {
mcollina marked this conversation as resolved.
Show resolved Hide resolved
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
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);
mcollina marked this conversation as resolved.
Show resolved Hide resolved
res.bytes += synchronously_written;

SetWriteResult(res);
Expand Down
27 changes: 18 additions & 9 deletions src/stream_base.h
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