http2: minor C++ cleanups #16461

addaleax · 2017-10-24T22:14:01Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

http2

jasnell · 2017-10-24T22:28:08Z

src/node_http2.cc

+  // For every node::Http2Session instance, there is a uv_prepare_t handle
+  // whose callback is triggered on every tick of the event loop. When
+  // run, nghttp2 is prompted to send any queued data it may have stored.
+  prep_ = new uv_prepare_t();


btw, definitely not for this PR, but down the road... I plan to refactor this so that there is one uv_prepare_t for N session instances, and loop through each on every uv_prepare_t tick... rather than registering one uv_prepare_t for each session. Will need to benchmark that tho...

👍 – I can leave a TODO if you want.

I’ve also wondered if it might worth hopping onto the same underlying mechanism that setImmediate uses…

nah, don't need a todo.

... the same underlying mechanism that setImmediate uses...

I've considered that also but haven't looked into it yet.

jasnell · 2017-10-24T22:33:29Z

src/node_http2_core-inl.h

@@ -193,7 +193,8 @@ inline ssize_t Nghttp2Session::OnStreamReadFD(nghttp2_session* session,
  uv_fs_t read_req;

  if (length > 0) {
-    numchars = uv_fs_read(handle->loop_,
+    // TODO(addaleax): Never use synchronous I/O on the main thread.


re: the todo....

just keep in mind that nghttp2 requires that the data be provided synchronously here...

Hm – from the nghttp2 header:

If the application wants to postpone DATA frames (e.g.,
asynchronous I/O, or reading data blocks for long time), it is
achieved by returning :enum:NGHTTP2_ERR_DEFERRED without reading
any data in this invocation. The library removes DATA frame from
the outgoing queue temporarily. To move back deferred DATA frame
to outgoing queue, call nghttp2_session_resume_data().

It even comes with instructions, so that sounds like it’s definitely supported?

yes and no. I was being a bit simplistic. nghttp2_session_resume_data triggers this callback to be invoked again to resume the data flow. Within this method, when the data is available, it must be copied in synchronously. So to achieve what you're looking for it would require a bit more mechanism... e.g.

attempt read,

no data? initiate uv_fs_read, return DEFER ... (wait for Step 2) ... keeping in mind that the buffer pointer nghttp2 hands us will be invalid when this function exits, so we'd need to allocate our own buffer for uv_fs_read.

got data? copy into nghttp2 provided buffer ...

all done? set EOF flag and return.

not all done? repeat step 1

uv_fs_read callback calls session_resume_data ... flow returns back to step 1

This can be further complicated using the NO_COPY flag, which causes another callback to be called telling our code to send our buffers without the need to memcpy, but there are some complications with that also.

So, yes, it's certainly possible, and likely desirable, just non-trivial.

A bit more background... nghttp2 calls this method while it is attempting to serialize a DATA frame for transmission. Calls to this method are synchronous when ng's send or send_mem function is called. Essentially, nghttp2 goes through it's queue of pending frames and serializes each. When it comes to this method, it will synchronously call it over and over until it receives either a DEFER, an error, or an EOF response. If it receives a DEFER, it takes that stream temporarily out of it's send queue. Calling session_resume_data inserts it back into the queue, so that the next time send or mem_send is called, serialization of the DATA frame can be attempted again.

jasnell

💯

apapirovski

apapirovski · 2017-10-24T22:23:28Z

src/node_http2.cc

+    HandleScope scope(session->env()->isolate());
+    Context::Scope context_scope(session->env()->context());
+
+    // Sending data may call arbitrary JS code, so for keep track of


nit: extra "for"? unless I'm misunderstanding :)

jasnell · 2017-10-24T22:59:11Z

(btw @addaleax ... I love that you're looking at this code :-) ...)

bnoordhuis · 2017-10-25T13:01:25Z

src/node_http2.cc

+  this->Nghttp2Session::Close();
+  // Stop the loop
+  CHECK_EQ(uv_prepare_stop(prep_), 0);
+  auto PrepClose = [](uv_handle_t* handle) {


Style nit: prep_close

bnoordhuis · 2017-10-25T13:02:16Z

src/node_http2.cc

+  auto PrepClose = [](uv_handle_t* handle) {
+    delete reinterpret_cast<uv_prepare_t*>(handle);
+  };
+  uv_close(reinterpret_cast<uv_handle_t*>(prep_), PrepClose);


Can you set prep_ = nullptr; afterwards?

bnoordhuis · 2017-10-25T13:06:18Z

src/node_http2.h

 private:
  StreamBase* stream_;
  StreamResource::Callback<StreamResource::AllocCb> prev_alloc_cb_;
  StreamResource::Callback<StreamResource::ReadCb> prev_read_cb_;
  padding_strategy_type padding_strategy_ = PADDING_STRATEGY_NONE;

  char stream_buf_[kAllocBufferSize];
+
+  uv_prepare_t* prep_ = nullptr;


For reasons of mechanical sympathy, I'd put it before the 64 kB stream_buf_ buffer.

As far as I understand it, `Nghttp2Session` should not be concerned about how its consumers handle I/O. PR-URL: nodejs#16461 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

Add an utility constructor for `AsyncWrap` classes that wish to leverage `InternalCallbackScope`s. PR-URL: nodejs#16461 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

Sending pending data may involve running arbitrary JavaScript code. Therefore, it should involve a callback scope. PR-URL: nodejs#16461 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

When assigning to a union twice, the first assignment is a no-op. PR-URL: nodejs#16461 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

Use weak handles to track the lifetime of an `Http2Session` instance. Refs: nodejs/http2#140 PR-URL: nodejs#16461 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

addaleax · 2017-10-30T20:59:44Z

CI: https://ci.nodejs.org/job/node-test-commit/13613/

addaleax · 2017-10-30T22:18:37Z

Windows failures are due to #16622

Landed in e567402...c8a00fd

When assigning to a union twice, the first assignment is a no-op. PR-URL: #16461 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

Use weak handles to track the lifetime of an `Http2Session` instance. Refs: nodejs/http2#140 PR-URL: #16461 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

As far as I understand it, `Nghttp2Session` should not be concerned about how its consumers handle I/O. PR-URL: nodejs/node#16461 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

Add an utility constructor for `AsyncWrap` classes that wish to leverage `InternalCallbackScope`s. PR-URL: nodejs/node#16461 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

Sending pending data may involve running arbitrary JavaScript code. Therefore, it should involve a callback scope. PR-URL: nodejs/node#16461 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

When assigning to a union twice, the first assignment is a no-op. PR-URL: nodejs/node#16461 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

Use weak handles to track the lifetime of an `Http2Session` instance. Refs: nodejs/http2#140 PR-URL: nodejs/node#16461 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>