src: remove duplicate env field from CryptoJob

Add a `ThreadPoolWork::env()` getter and use that. To fix the diamond problem in class CompressionStream that inherits from both AsyncWrap and ThreadPoolWork, we add a `CompressionStream::env()` getter that delegates to `AsyncWrap::env()`. Refs: nodejs#31554 (comment)
bnoordhuis · Jan 31, 2020 · 94935b2 · 94935b2
1 parent a171314
commit 94935b2
Show file tree

Hide file tree

Showing 3 changed files with 24 additions and 21 deletions.
diff --git a/src/node_crypto.cc b/src/node_crypto.cc
@@ -6194,9 +6194,8 @@ bool ECDH::IsKeyPairValid() {
 // TODO(addaleax): If there is an `AsyncWrap`, it currently has no access to
 // this object. This makes proper reporting of memory usage impossible.
 struct CryptoJob : public ThreadPoolWork {
-  Environment* const env;
   std::unique_ptr<AsyncWrap> async_wrap;
-  inline explicit CryptoJob(Environment* env) : ThreadPoolWork(env), env(env) {}
+  inline explicit CryptoJob(Environment* env) : ThreadPoolWork(env) {}
   inline void AfterThreadPoolWork(int status) final;
   virtual void AfterThreadPoolWork() = 0;
   static inline void Run(std::unique_ptr<CryptoJob> job, Local<Value> wrap);
@@ -6207,8 +6206,8 @@ void CryptoJob::AfterThreadPoolWork(int status) {
   CHECK(status == 0 || status == UV_ECANCELED);
   std::unique_ptr<CryptoJob> job(this);
   if (status == UV_ECANCELED) return;
-  HandleScope handle_scope(env->isolate());
-  Context::Scope context_scope(env->context());
+  HandleScope handle_scope(env()->isolate());
+  Context::Scope context_scope(env()->context());
   CHECK_EQ(false, async_wrap->persistent().IsWeak());
   AfterThreadPoolWork();
 }
@@ -6249,12 +6248,12 @@ struct RandomBytesJob : public CryptoJob {
 
   inline void AfterThreadPoolWork() override {
     Local<Value> arg = ToResult();
-    async_wrap->MakeCallback(env->ondone_string(), 1, &arg);
+    async_wrap->MakeCallback(env()->ondone_string(), 1, &arg);
   }
 
   inline Local<Value> ToResult() const {
-    if (errors.empty()) return Undefined(env->isolate());
-    return errors.ToException(env).ToLocalChecked();
+    if (errors.empty()) return Undefined(env()->isolate());
+    return errors.ToException(env()).ToLocalChecked();
   }
 };
 
@@ -6306,11 +6305,11 @@ struct PBKDF2Job : public CryptoJob {
 
   inline void AfterThreadPoolWork() override {
     Local<Value> arg = ToResult();
-    async_wrap->MakeCallback(env->ondone_string(), 1, &arg);
+    async_wrap->MakeCallback(env()->ondone_string(), 1, &arg);
   }
 
   inline Local<Value> ToResult() const {
-    return Boolean::New(env->isolate(), success.FromJust());
+    return Boolean::New(env()->isolate(), success.FromJust());
   }
 
   inline void Cleanse() {
@@ -6386,12 +6385,12 @@ struct ScryptJob : public CryptoJob {
 
   inline void AfterThreadPoolWork() override {
     Local<Value> arg = ToResult();
-    async_wrap->MakeCallback(env->ondone_string(), 1, &arg);
+    async_wrap->MakeCallback(env()->ondone_string(), 1, &arg);
   }
 
   inline Local<Value> ToResult() const {
-    if (errors.empty()) return Undefined(env->isolate());
-    return errors.ToException(env).ToLocalChecked();
+    if (errors.empty()) return Undefined(env()->isolate());
+    return errors.ToException(env()).ToLocalChecked();
   }
 
   inline void Cleanse() {
@@ -6720,22 +6719,22 @@ class GenerateKeyPairJob : public CryptoJob {
   inline void AfterThreadPoolWork() override {
     Local<Value> args[3];
     ToResult(&args[0], &args[1], &args[2]);
-    async_wrap->MakeCallback(env->ondone_string(), 3, args);
+    async_wrap->MakeCallback(env()->ondone_string(), 3, args);
   }
 
   inline void ToResult(Local<Value>* err,
                        Local<Value>* pubkey,
                        Local<Value>* privkey) {
     if (pkey_ && EncodeKeys(pubkey, privkey)) {
       CHECK(errors_.empty());
-      *err = Undefined(env->isolate());
+      *err = Undefined(env()->isolate());
     } else {
       if (errors_.empty())
         errors_.Capture();
       CHECK(!errors_.empty());
-      *err = errors_.ToException(env).ToLocalChecked();
-      *pubkey = Undefined(env->isolate());
-      *privkey = Undefined(env->isolate());
+      *err = errors_.ToException(env()).ToLocalChecked();
+      *pubkey = Undefined(env()->isolate());
+      *privkey = Undefined(env()->isolate());
     }
   }
 
@@ -6744,20 +6743,20 @@ class GenerateKeyPairJob : public CryptoJob {
     if (public_key_encoding_.output_key_object_) {
       // Note that this has the downside of containing sensitive data of the
       // private key.
-      if (!KeyObject::Create(env, kKeyTypePublic, pkey_).ToLocal(pubkey))
+      if (!KeyObject::Create(env(), kKeyTypePublic, pkey_).ToLocal(pubkey))
         return false;
     } else {
-      if (!WritePublicKey(env, pkey_.get(), public_key_encoding_)
+      if (!WritePublicKey(env(), pkey_.get(), public_key_encoding_)
                .ToLocal(pubkey))
         return false;
     }
 
     // Now do the same for the private key.
     if (private_key_encoding_.output_key_object_) {
-      if (!KeyObject::Create(env, kKeyTypePrivate, pkey_).ToLocal(privkey))
+      if (!KeyObject::Create(env(), kKeyTypePrivate, pkey_).ToLocal(privkey))
         return false;
     } else {
-      if (!WritePrivateKey(env, pkey_.get(), private_key_encoding_)
+      if (!WritePrivateKey(env(), pkey_.get(), private_key_encoding_)
                .ToLocal(privkey))
         return false;
     }

diff --git a/src/node_internals.h b/src/node_internals.h
@@ -268,6 +268,8 @@ class ThreadPoolWork {
   virtual void DoThreadPoolWork() = 0;
   virtual void AfterThreadPoolWork(int status) = 0;
 
+  inline Environment* env() const { return env_; }
+
  private:
   Environment* env_;
   uv_work_t work_req_;

diff --git a/src/node_zlib.cc b/src/node_zlib.cc
@@ -256,6 +256,8 @@ class CompressionStream : public AsyncWrap, public ThreadPoolWork {
     CHECK_EQ(unreported_allocations_, 0);
   }
 
+  Environment* env() const { return AsyncWrap::env(); }
+
   void Close() {
     if (write_in_progress_) {
       pending_close_ = true;