src: make ReqWrap weak

This commit allows throwing an exception after creating `FSReqCallback` Co-authored-by: Anna Henningsen <anna@addaleax.net> PR-URL: #44074 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs · Sep 26, 2022 · 75cfb13 · 75cfb13
1 parent 9da1142
commit 75cfb13
Show file tree

Hide file tree

Showing 7 changed files with 15 additions and 18 deletions.
diff --git 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
@@ -1429,7 +1429,7 @@ static void Query(const FunctionCallbackInfo<Value>& args) {
 
 void AfterGetAddrInfo(uv_getaddrinfo_t* req, int status, struct addrinfo* res) {
   auto cleanup = OnScopeLeave([&]() { uv_freeaddrinfo(res); });
-  std::unique_ptr<GetAddrInfoReqWrap> req_wrap {
+  BaseObjectPtr<GetAddrInfoReqWrap> req_wrap{
       static_cast<GetAddrInfoReqWrap*>(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<GetNameInfoReqWrap> req_wrap {
+  BaseObjectPtr<GetNameInfoReqWrap> req_wrap{
       static_cast<GetNameInfoReqWrap*>(req->data)};
   Environment* env = req_wrap->env();
 

diff --git a/src/connection_wrap.cc b/src/connection_wrap.cc
@@ -77,9 +77,8 @@ void ConnectionWrap<WrapType, UVType>::OnConnection(uv_stream_t* handle,
 template <typename WrapType, typename UVType>
 void ConnectionWrap<WrapType, UVType>::AfterConnect(uv_connect_t* req,
                                                     int status) {
-  std::unique_ptr<ConnectWrap> req_wrap
-    (static_cast<ConnectWrap*>(req->data));
-  CHECK_NOT_NULL(req_wrap);
+  BaseObjectPtr<ConnectWrap> req_wrap{static_cast<ConnectWrap*>(req->data)};
+  CHECK(req_wrap);
   WrapType* wrap = static_cast<WrapType*>(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
@@ -460,8 +460,8 @@ MaybeLocal<Promise> 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<CloseReq> close(CloseReq::from_req(req));
-    CHECK_NOT_NULL(close);
+    BaseObjectPtr<CloseReq> 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
@@ -20,13 +20,12 @@ ReqWrap<T>::ReqWrap(Environment* env,
                     AsyncWrap::ProviderType provider)
     : AsyncWrap(env, object, provider),
       ReqWrapBase(env) {
+  MakeWeak();
   Reset();
 }
 
 template <typename T>
-ReqWrap<T>::~ReqWrap() {
-  CHECK_EQ(false, persistent().IsEmpty());
-}
+ReqWrap<T>::~ReqWrap() {}
 
 template <typename T>
 void ReqWrap<T>::Dispatched() {
@@ -120,7 +119,8 @@ struct MakeLibuvRequestCallback<ReqT, void(*)(ReqT*, Args...)> {
   using F = void(*)(ReqT* req, Args... args);
 
   static void Wrapper(ReqT* req, Args... args) {
-    ReqWrap<ReqT>* req_wrap = ReqWrap<ReqT>::from_req(req);
+    BaseObjectPtr<ReqWrap<ReqT>> req_wrap{ReqWrap<ReqT>::from_req(req)};
+    req_wrap->Detach();
     req_wrap->env()->DecreaseWaitingRequestCounter();
     F original_callback = reinterpret_cast<F>(req_wrap->original_callback_);
     original_callback(req, args...);
@@ -138,7 +138,6 @@ template <typename T>
 template <typename LibuvFunction, typename... Args>
 int ReqWrap<T>::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<T>::Dispatch(LibuvFunction fn, Args... args) {
       env()->event_loop(),
       req(),
       MakeLibuvRequestCallback<T, Args>::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
@@ -344,6 +344,7 @@ void TCPWrap::Connect(const FunctionCallbackInfo<Value>& 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
@@ -669,7 +669,7 @@ int UDPWrap::RecvStop() {
 
 
 void UDPWrap::OnSendDone(ReqWrap<uv_udp_send_t>* req, int status) {
-  std::unique_ptr<SendWrap> req_wrap{static_cast<SendWrap*>(req)};
+  BaseObjectPtr<SendWrap> req_wrap{static_cast<SendWrap*>(req)};
   if (req_wrap->have_callback()) {
     Environment* env = req_wrap->env();
     HandleScope handle_scope(env->isolate());