From f4b620f83c9fcd33854a50db13ad43c7ee9340c7 Mon Sep 17 00:00:00 2001 From: Rafael Gonzaga Date: Wed, 14 Sep 2022 09:47:40 -0300 Subject: [PATCH] src: make ReqWrap weak This commit allows throwing an exception after creating `FSReqCallback` Co-authored-by: Anna Henningsen PR-URL: https://github.com/nodejs/node/pull/44074 Reviewed-By: Anna Henningsen Reviewed-By: James M Snell --- lib/fs.js | 4 ---- src/cares_wrap.cc | 4 ++-- src/connection_wrap.cc | 5 ++--- src/node_file.cc | 4 ++-- src/req_wrap-inl.h | 13 +++++++------ src/tcp_wrap.cc | 1 + src/udp_wrap.cc | 2 +- 7 files changed, 15 insertions(+), 18 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index c618bd2cac89cc..c46e23061cbecd 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -24,10 +24,6 @@ 'use strict'; -// When using FSReqCallback, make sure to create the object only *after* all -// parameter validation has happened, so that the objects are not kept in memory -// in case they are created but never used due to an exception. - const { ArrayPrototypePush, BigIntPrototypeToString, diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc index 1757d56d09ab4d..d19705f94e7f6e 100644 --- a/src/cares_wrap.cc +++ b/src/cares_wrap.cc @@ -1429,7 +1429,7 @@ static void Query(const FunctionCallbackInfo& args) { void AfterGetAddrInfo(uv_getaddrinfo_t* req, int status, struct addrinfo* res) { auto cleanup = OnScopeLeave([&]() { uv_freeaddrinfo(res); }); - std::unique_ptr req_wrap { + BaseObjectPtr req_wrap{ static_cast(req->data)}; Environment* env = req_wrap->env(); @@ -1502,7 +1502,7 @@ void AfterGetNameInfo(uv_getnameinfo_t* req, int status, const char* hostname, const char* service) { - std::unique_ptr req_wrap { + BaseObjectPtr req_wrap{ static_cast(req->data)}; Environment* env = req_wrap->env(); diff --git a/src/connection_wrap.cc b/src/connection_wrap.cc index 29815eee4381a0..8cb6b8e66cec75 100644 --- a/src/connection_wrap.cc +++ b/src/connection_wrap.cc @@ -77,9 +77,8 @@ void ConnectionWrap::OnConnection(uv_stream_t* handle, template void ConnectionWrap::AfterConnect(uv_connect_t* req, int status) { - std::unique_ptr req_wrap - (static_cast(req->data)); - CHECK_NOT_NULL(req_wrap); + BaseObjectPtr req_wrap{static_cast(req->data)}; + CHECK(req_wrap); WrapType* wrap = static_cast(req->handle->data); CHECK_EQ(req_wrap->env(), wrap->env()); Environment* env = wrap->env(); diff --git a/src/node_file.cc b/src/node_file.cc index 2bbb3791a4388c..31f58d61c0f251 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -460,8 +460,8 @@ MaybeLocal FileHandle::ClosePromise() { CloseReq* req = new CloseReq(env(), close_req_obj, promise, object()); auto AfterClose = uv_fs_callback_t{[](uv_fs_t* req) { - std::unique_ptr close(CloseReq::from_req(req)); - CHECK_NOT_NULL(close); + BaseObjectPtr close(CloseReq::from_req(req)); + CHECK(close); close->file_handle()->AfterClose(); if (!close->env()->can_call_into_js()) return; Isolate* isolate = close->env()->isolate(); diff --git a/src/req_wrap-inl.h b/src/req_wrap-inl.h index c905b605cbde97..6bb5a58cb85494 100644 --- a/src/req_wrap-inl.h +++ b/src/req_wrap-inl.h @@ -20,13 +20,12 @@ ReqWrap::ReqWrap(Environment* env, AsyncWrap::ProviderType provider) : AsyncWrap(env, object, provider), ReqWrapBase(env) { + MakeWeak(); Reset(); } template -ReqWrap::~ReqWrap() { - CHECK_EQ(false, persistent().IsEmpty()); -} +ReqWrap::~ReqWrap() {} template void ReqWrap::Dispatched() { @@ -120,7 +119,8 @@ struct MakeLibuvRequestCallback { using F = void(*)(ReqT* req, Args... args); static void Wrapper(ReqT* req, Args... args) { - ReqWrap* req_wrap = ReqWrap::from_req(req); + BaseObjectPtr> req_wrap{ReqWrap::from_req(req)}; + req_wrap->Detach(); req_wrap->env()->DecreaseWaitingRequestCounter(); F original_callback = reinterpret_cast(req_wrap->original_callback_); original_callback(req, args...); @@ -138,7 +138,6 @@ template template int ReqWrap::Dispatch(LibuvFunction fn, Args... args) { Dispatched(); - // This expands as: // // int err = fn(env()->event_loop(), req(), arg1, arg2, Wrapper, arg3, ...) @@ -158,8 +157,10 @@ int ReqWrap::Dispatch(LibuvFunction fn, Args... args) { env()->event_loop(), req(), MakeLibuvRequestCallback::For(this, args)...); - if (err >= 0) + if (err >= 0) { + ClearWeak(); env()->IncreaseWaitingRequestCounter(); + } return err; } diff --git a/src/tcp_wrap.cc b/src/tcp_wrap.cc index eda5aedf5a17b6..307f2d31f02b01 100644 --- a/src/tcp_wrap.cc +++ b/src/tcp_wrap.cc @@ -344,6 +344,7 @@ void TCPWrap::Connect(const FunctionCallbackInfo& args, if (err) { delete req_wrap; } else { + CHECK(args[2]->Uint32Value(env->context()).IsJust()); int port = args[2]->Uint32Value(env->context()).FromJust(); TRACE_EVENT_NESTABLE_ASYNC_BEGIN2(TRACING_CATEGORY_NODE2(net, native), "connect", diff --git a/src/udp_wrap.cc b/src/udp_wrap.cc index db1548470fab1c..5ea38f6732d9f4 100644 --- a/src/udp_wrap.cc +++ b/src/udp_wrap.cc @@ -669,7 +669,7 @@ int UDPWrap::RecvStop() { void UDPWrap::OnSendDone(ReqWrap* req, int status) { - std::unique_ptr req_wrap{static_cast(req)}; + BaseObjectPtr req_wrap{static_cast(req)}; if (req_wrap->have_callback()) { Environment* env = req_wrap->env(); HandleScope handle_scope(env->isolate());