http2: refactor how trailers are done

Rather than an option, introduce a method and an event...

```js
server.on('stream', (stream) => {
  stream.respond(undefined, { waitForTrailers: true });
  stream.on('wantTrailers', () => {
    stream.sendTrailers({ abc: 'xyz'});
  });
  stream.end('hello world');
});
```

This is a breaking change in the API such that the prior
`options.getTrailers` is no longer supported at all.
Ordinarily this would be semver-major and require a
deprecation but the http2 stuff is still experimental.

PR-URL: https://github.com/nodejs/node/pull/19959
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

2 files changed, 45 insertions, 81 deletions

diff --git a/src/node_http2.cc b/src/node_http2.cc
index 6c4b812e03..1c5c68f094 100644
--- a/src/node_http2.cc
+++ b/src/node_http2.cc
@@ -1066,16 +1066,6 @@ int Http2Session::OnNghttpError(nghttp2_session* handle,
   return 0;
 }
 
-// Once all of the DATA frames for a Stream have been sent, the GetTrailers
-// method calls out to JavaScript to fetch the trailing headers that need
-// to be sent.
-void Http2Session::GetTrailers(Http2Stream* stream, uint32_t* flags) {
-  if (!stream->IsDestroyed() && stream->HasTrailers()) {
-    Http2Stream::SubmitTrailers submit_trailers{this, stream, flags};
-    stream->OnTrailers(submit_trailers);
-  }
-}
-
 uv_buf_t Http2StreamListener::OnStreamAlloc(size_t size) {
   // See the comments in Http2Session::OnDataChunkReceived
   // (which is the only possible call site for this method).
@@ -1111,25 +1101,6 @@ void Http2StreamListener::OnStreamRead(ssize_t nread, const uv_buf_t& buf) {
   stream->CallJSOnreadMethod(nread, buffer);
 }
 
-Http2Stream::SubmitTrailers::SubmitTrailers(
-    Http2Session* session,
-    Http2Stream* stream,
-    uint32_t* flags)
-  : session_(session), stream_(stream), flags_(flags) { }
-
-
-void Http2Stream::SubmitTrailers::Submit(nghttp2_nv* trailers,
-                                         size_t length) const {
-  Http2Scope h2scope(session_);
-  if (length == 0)
-    return;
-  DEBUG_HTTP2SESSION2(session_, "sending trailers for stream %d, count: %d",
-                      stream_->id(), length);
-  *flags_ |= NGHTTP2_DATA_FLAG_NO_END_STREAM;
-  CHECK_EQ(
-      nghttp2_submit_trailer(**session_, stream_->id(), trailers, length), 0);
-}
-
 
 // Called by OnFrameReceived to notify JavaScript land that a complete
 // HEADERS frame has been received and processed. This method converts the
@@ -1725,30 +1696,6 @@ nghttp2_stream* Http2Stream::operator*() {
   return nghttp2_session_find_stream(**session_, id_);
 }
 
-
-// Calls out to JavaScript land to fetch the actual trailer headers to send
-// for this stream.
-void Http2Stream::OnTrailers(const SubmitTrailers& submit_trailers) {
-  DEBUG_HTTP2STREAM(this, "prompting for trailers");
-  CHECK(!this->IsDestroyed());
-  Isolate* isolate = env()->isolate();
-  HandleScope scope(isolate);
-  Local<Context> context = env()->context();
-  Context::Scope context_scope(context);
-
-  Local<Value> ret =
-      MakeCallback(env()->ontrailers_string(), 0, nullptr).ToLocalChecked();
-  if (!ret.IsEmpty() && !IsDestroyed()) {
-    if (ret->IsArray()) {
-      Local<Array> headers = ret.As<Array>();
-      if (headers->Length() > 0) {
-        Headers trailers(isolate, context, headers);
-        submit_trailers.Submit(*trailers, trailers.length());
-      }
-    }
-  }
-}
-
 void Http2Stream::Close(int32_t code) {
   CHECK(!this->IsDestroyed());
   flags_ |= NGHTTP2_STREAM_FLAG_CLOSED;
@@ -1843,6 +1790,26 @@ int Http2Stream::SubmitInfo(nghttp2_nv* nva, size_t len) {
   return ret;
 }
 
+void Http2Stream::OnTrailers() {
+  DEBUG_HTTP2STREAM(this, "let javascript know we are ready for trailers");
+  CHECK(!this->IsDestroyed());
+  Isolate* isolate = env()->isolate();
+  HandleScope scope(isolate);
+  Local<Context> context = env()->context();
+  Context::Scope context_scope(context);
+  MakeCallback(env()->ontrailers_string(), 0, nullptr);
+}
+
+// Submit informational headers for a stream.
+int Http2Stream::SubmitTrailers(nghttp2_nv* nva, size_t len) {
+  CHECK(!this->IsDestroyed());
+  Http2Scope h2scope(this);
+  DEBUG_HTTP2STREAM2(this, "sending %d trailers", len);
+  int ret = nghttp2_submit_trailer(**session_, id_, nva, len);
+  CHECK_NE(ret, NGHTTP2_ERR_NOMEM);
+  return ret;
+}
+
 // Submit a PRIORITY frame to the connected peer.
 int Http2Stream::SubmitPriority(nghttp2_priority_spec* prispec,
                                 bool silent) {
@@ -2068,13 +2035,10 @@ ssize_t Http2Stream::Provider::Stream::OnRead(nghttp2_session* handle,
   if (stream->queue_.empty() && !stream->IsWritable()) {
     DEBUG_HTTP2SESSION2(session, "no more data for stream %d", id);
     *flags |= NGHTTP2_DATA_FLAG_EOF;
-    session->GetTrailers(stream, flags);
-    // If the stream or session gets destroyed during the GetTrailers
-    // callback, check that here and close down the stream
-    if (stream->IsDestroyed())
-      return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE;
-    if (session->IsDestroyed())
-      return NGHTTP2_ERR_CALLBACK_FAILURE;
+    if (stream->HasTrailers()) {
+      *flags |= NGHTTP2_DATA_FLAG_NO_END_STREAM;
+      stream->OnTrailers();
+    }
   }
 
   stream->statistics_.sent_bytes += amount;
@@ -2361,6 +2325,21 @@ void Http2Stream::Info(const FunctionCallbackInfo<Value>& args) {
                      headers->Length());
 }
 
+// Submits trailing headers on the Http2Stream
+void Http2Stream::Trailers(const FunctionCallbackInfo<Value>& args) {
+  Environment* env = Environment::GetCurrent(args);
+  Local<Context> context = env->context();
+  Isolate* isolate = env->isolate();
+  Http2Stream* stream;
+  ASSIGN_OR_RETURN_UNWRAP(&stream, args.Holder());
+
+  Local<Array> headers = args[0].As<Array>();
+
+  Headers list(isolate, context, headers);
+  args.GetReturnValue().Set(stream->SubmitTrailers(*list, list.length()));
+  DEBUG_HTTP2STREAM2(stream, "%d trailing headers sent", headers->Length());
+}
+
 // Grab the numeric id of the Http2Stream
 void Http2Stream::GetID(const FunctionCallbackInfo<Value>& args) {
   Http2Stream* stream;
@@ -2706,6 +2685,7 @@ void Initialize(Local<Object> target,
   env->SetProtoMethod(stream, "priority", Http2Stream::Priority);
   env->SetProtoMethod(stream, "pushPromise", Http2Stream::PushPromise);
   env->SetProtoMethod(stream, "info", Http2Stream::Info);
+  env->SetProtoMethod(stream, "trailers", Http2Stream::Trailers);
   env->SetProtoMethod(stream, "respond", Http2Stream::Respond);
   env->SetProtoMethod(stream, "rstStream", Http2Stream::RstStream);
   env->SetProtoMethod(stream, "refreshState", Http2Stream::RefreshState);
diff --git a/src/node_http2.h b/src/node_http2.h
index 9dd65667f9..87c929cdea 100644
--- a/src/node_http2.h
+++ b/src/node_http2.h
@@ -581,6 +581,10 @@ class Http2Stream : public AsyncWrap,
   // Submit informational headers for this stream
   int SubmitInfo(nghttp2_nv* nva, size_t len);
 
+  // Submit trailing headers for this stream
+  int SubmitTrailers(nghttp2_nv* nva, size_t len);
+  void OnTrailers();
+
   // Submit a PRIORITY frame for this stream
   int SubmitPriority(nghttp2_priority_spec* prispec, bool silent = false);
 
@@ -670,25 +674,6 @@ class Http2Stream : public AsyncWrap,
 
   size_t self_size() const override { return sizeof(*this); }
 
-  // Handling Trailer Headers
-  class SubmitTrailers {
-   public:
-    void Submit(nghttp2_nv* trailers, size_t length) const;
-
-    SubmitTrailers(Http2Session* sesion,
-                   Http2Stream* stream,
-                   uint32_t* flags);
-
-   private:
-    Http2Session* const session_;
-    Http2Stream* const stream_;
-    uint32_t* const flags_;
-
-    friend class Http2Stream;
-  };
-
-  void OnTrailers(const SubmitTrailers& submit_trailers);
-
   // JavaScript API
   static void GetID(const FunctionCallbackInfo<Value>& args);
   static void Destroy(const FunctionCallbackInfo<Value>& args);
@@ -697,6 +682,7 @@ class Http2Stream : public AsyncWrap,
   static void PushPromise(const FunctionCallbackInfo<Value>& args);
   static void RefreshState(const FunctionCallbackInfo<Value>& args);
   static void Info(const FunctionCallbackInfo<Value>& args);
+  static void Trailers(const FunctionCallbackInfo<Value>& args);
   static void Respond(const FunctionCallbackInfo<Value>& args);
   static void RstStream(const FunctionCallbackInfo<Value>& args);
 
@@ -859,8 +845,6 @@ class Http2Session : public AsyncWrap, public StreamListener {
 
   size_t self_size() const override { return sizeof(*this); }
 
-  void GetTrailers(Http2Stream* stream, uint32_t* flags);
-
   // Handle reads/writes from the underlying network transport.
   void OnStreamRead(ssize_t nread, const uv_buf_t& buf) override;
   void OnStreamAfterWrite(WriteWrap* w, int status) override;