http: simplify parser lifetime tracking

Instead of providing a separate class for keeping the parser alive during its own call back, just delay a possible `.close()` call until the stack has cleared completely. PR-URL: #18135 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
nodejs · Jan 30, 2018 · 8e084d8 · 8e084d8
1 parent 4513cbb
commit 8e084d8
Show file tree

Hide file tree

Showing 2 changed files with 5 additions and 24 deletions.
diff --git a/lib/_http_common.js b/lib/_http_common.js
@@ -190,6 +190,7 @@ var parsers = new FreeList('parsers', 1000, function() {
   return parser;
 });
 
+function closeParserInstance(parser) { parser.close(); }
 
 // Free the parser and also break any links that it
 // might have to any other things.
@@ -212,7 +213,9 @@ function freeParser(parser, req, socket) {
     parser.outgoing = null;
     parser[kOnExecute] = null;
     if (parsers.free(parser) === false) {
-      parser.close();
+      // Make sure the parser's stack has unwound before deleting the
+      // corresponding C++ object through .close().
+      setImmediate(closeParserInstance, parser);
     } else {
       // Since the Parser destructor isn't going to run the destroy() callbacks
       // it needs to be triggered manually.

diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc
@@ -376,8 +376,7 @@ class Parser : public AsyncWrap {
     Parser* parser;
     ASSIGN_OR_RETURN_UNWRAP(&parser, args.Holder());
 
-    if (--parser->refcount_ == 0)
-      delete parser;
+    delete parser;
   }
 
 
@@ -543,22 +542,6 @@ class Parser : public AsyncWrap {
   }
 
  protected:
-  class ScopedRetainParser {
-   public:
-    explicit ScopedRetainParser(Parser* p) : p_(p) {
-      CHECK_GT(p_->refcount_, 0);
-      p_->refcount_++;
-    }
-
-    ~ScopedRetainParser() {
-      if (0 == --p_->refcount_)
-        delete p_;
-    }
-
-   private:
-    Parser* const p_;
-  };
-
   static const size_t kAllocBufferSize = 64 * 1024;
 
   static void OnAllocImpl(size_t suggested_size, uv_buf_t* buf, void* ctx) {
@@ -595,8 +578,6 @@ class Parser : public AsyncWrap {
     if (nread == 0)
       return;
 
-    ScopedRetainParser retain(parser);
-
     parser->current_buffer_.Clear();
     Local<Value> ret = parser->Execute(buf->base, nread);
 
@@ -734,7 +715,6 @@ class Parser : public AsyncWrap {
   char* current_buffer_data_;
   StreamResource::Callback<StreamResource::AllocCb> prev_alloc_cb_;
   StreamResource::Callback<StreamResource::ReadCb> prev_read_cb_;
-  int refcount_ = 1;
 
   // These are helper functions for filling `http_parser_settings`, which turn
   // a member function of Parser into a C-style HTTP parser callback.
@@ -751,8 +731,6 @@ class Parser : public AsyncWrap {
   typedef int (Parser::*DataCall)(const char* at, size_t length);
 
   static const struct http_parser_settings settings;
-
-  friend class ScopedRetainParser;
 };
 
 const struct http_parser_settings Parser::settings = {