http: align parser with StreamBase interface changes

The `StreamBase` interface changed, so that `OnStreamRead()` and `OnStreamAlloc()` are not guaranteed to be emitted in the same tick any more. This means that, while it isn’t causing any trouble right now, we should not assume that it’s safe to return a static buffer in the HTTP parser’s `OnStreamAlloc()` method. PR-URL: #18936 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
nodejs · addaleax · Feb 24, 2018 · Feb 12, 2018 · Feb 22, 2018 · Mar 7, 2018
commit d8eb113a65555d3f688a897b1681e59b73b1f763
diff --git a/src/env-inl.h b/src/env-inl.h
@@ -469,6 +469,14 @@ inline void Environment::set_http_parser_buffer(char* buffer) {
   http_parser_buffer_ = buffer;
 }
 
+inline bool Environment::http_parser_buffer_in_use() const {
+  return http_parser_buffer_in_use_;
+}
+
+inline void Environment::set_http_parser_buffer_in_use(bool in_use) {
+  http_parser_buffer_in_use_ = in_use;
+}
+
 inline http2::http2_state* Environment::http2_state() const {
   return http2_state_.get();
 }

diff --git a/src/env.h b/src/env.h
@@ -646,6 +646,8 @@ class Environment {
 
   inline char* http_parser_buffer() const;
   inline void set_http_parser_buffer(char* buffer);
+  inline bool http_parser_buffer_in_use() const;
+  inline void set_http_parser_buffer_in_use(bool in_use);
 
   inline http2::http2_state* http2_state() const;
   inline void set_http2_state(std::unique_ptr<http2::http2_state> state);
@@ -828,6 +830,7 @@ class Environment {
   double* heap_space_statistics_buffer_ = nullptr;
 
   char* http_parser_buffer_;
+  bool http_parser_buffer_in_use_ = false;
   std::unique_ptr<http2::http2_state> http2_state_;
 
   // stat fields contains twice the number of entries because `fs.StatWatcher`

diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc
@@ -525,6 +525,14 @@ class Parser : public AsyncWrap, public StreamListener {
   static const size_t kAllocBufferSize = 64 * 1024;
 
   uv_buf_t OnStreamAlloc(size_t suggested_size) override {
+    // For most types of streams, OnStreamRead will be immediately after
+    // OnStreamAlloc, and will consume all data, so using a static buffer for
+    // reading is more efficient. For other streams, just use the default
+    // allocator, which uses Malloc().
+    if (env()->http_parser_buffer_in_use())
+      return StreamListener::OnStreamAlloc(suggested_size);
+    env()->set_http_parser_buffer_in_use(true);
+
     if (env()->http_parser_buffer() == nullptr)
       env()->set_http_parser_buffer(new char[kAllocBufferSize]);
 
@@ -534,6 +542,15 @@ class Parser : public AsyncWrap, public StreamListener {
 
   void OnStreamRead(ssize_t nread, const uv_buf_t& buf) override {
     HandleScope scope(env()->isolate());
+    // Once we’re done here, either indicate that the HTTP parser buffer
+    // is free for re-use, or free() the data if it didn’t come from there
+    // in the first place.
+    OnScopeLeave on_scope_leave([&]() {
+      if (buf.base == env()->http_parser_buffer())
+        env()->set_http_parser_buffer_in_use(false);
+      else
+        free(buf.base);
+    });
 
     if (nread < 0) {
       PassReadErrorToPreviousListener(nread);

diff --git a/src/util.h b/src/util.h
@@ -436,6 +436,14 @@ class BufferValue : public MaybeStackBuffer<char> {
 template <typename T> inline void USE(T&&) {}
 }  // namespace node
 
+// Run a function when exiting the current scope.
+struct OnScopeLeave {
+  std::function<void()> fn_;
+
+  explicit OnScopeLeave(std::function<void()> fn) : fn_(fn) {}
+  ~OnScopeLeave() { fn_(); }
+};
+
 #endif  // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
 
 #endif  // SRC_UTIL_H_