Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http2: minor C++ cleanups #16461

Merged
merged 5 commits into from
Oct 30, 2017
Merged

http2: minor C++ cleanups #16461

merged 5 commits into from
Oct 30, 2017

Conversation

addaleax
Copy link
Member

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

http2

@addaleax addaleax added c++ Issues and PRs that require attention from people who are familiar with C++. http2 Issues or PRs related to the http2 subsystem. labels Oct 24, 2017
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Oct 24, 2017
// 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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 – 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…

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: the todo....

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

  1. 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
  2. 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

HandleScope scope(session->env()->isolate());
Context::Scope context_scope(session->env()->context());

// Sending data may call arbitrary JS code, so for keep track of
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jasnell
Copy link
Member

jasnell commented Oct 24, 2017

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style nit: prep_close

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you set prep_ = nullptr; afterwards?

src/node_http2.h Outdated
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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Add an utility constructor for `AsyncWrap` classes that wish to
leverage `InternalCallbackScope`s.

PR-URL: nodejs#16461
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Sending pending data may involve running arbitrary JavaScript code.
Therefore, it should involve a callback scope.

PR-URL: nodejs#16461
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
When assigning to a union twice, the first assignment is a no-op.

PR-URL: nodejs#16461
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Use weak handles to track the lifetime of an `Http2Session` instance.

Refs: nodejs/http2#140
PR-URL: nodejs#16461
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
@addaleax
Copy link
Member Author

@addaleax
Copy link
Member Author

Windows failures are due to #16622

Landed in e567402...c8a00fd

@addaleax addaleax closed this Oct 30, 2017
@addaleax addaleax merged commit c8a00fd into nodejs:master Oct 30, 2017
@addaleax addaleax deleted the http2-prepare branch October 30, 2017 22:18
MylesBorins pushed a commit that referenced this pull request Oct 31, 2017
When assigning to a union twice, the first assignment is a no-op.

PR-URL: #16461
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 31, 2017
Use weak handles to track the lifetime of an `Http2Session` instance.

Refs: nodejs/http2#140
PR-URL: #16461
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
@gibfahn gibfahn mentioned this pull request Oct 31, 2017
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
Add an utility constructor for `AsyncWrap` classes that wish to
leverage `InternalCallbackScope`s.

PR-URL: nodejs/node#16461
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
When assigning to a union twice, the first assignment is a no-op.

PR-URL: nodejs/node#16461
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
Use weak handles to track the lifetime of an `Http2Session` instance.

Refs: nodejs/http2#140
PR-URL: nodejs/node#16461
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
Add an utility constructor for `AsyncWrap` classes that wish to
leverage `InternalCallbackScope`s.

PR-URL: nodejs/node#16461
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
When assigning to a union twice, the first assignment is a no-op.

PR-URL: nodejs/node#16461
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
Use weak handles to track the lifetime of an `Http2Session` instance.

Refs: nodejs/http2#140
PR-URL: nodejs/node#16461
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
cjihrig pushed a commit to cjihrig/node that referenced this pull request Nov 6, 2017
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
cjihrig pushed a commit to cjihrig/node that referenced this pull request Nov 6, 2017
Add an utility constructor for `AsyncWrap` classes that wish to
leverage `InternalCallbackScope`s.

PR-URL: nodejs#16461
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
cjihrig pushed a commit to cjihrig/node that referenced this pull request Nov 6, 2017
Sending pending data may involve running arbitrary JavaScript code.
Therefore, it should involve a callback scope.

PR-URL: nodejs#16461
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
cjihrig pushed a commit to cjihrig/node that referenced this pull request Nov 6, 2017
When assigning to a union twice, the first assignment is a no-op.

PR-URL: nodejs#16461
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
cjihrig pushed a commit to cjihrig/node that referenced this pull request Nov 6, 2017
Use weak handles to track the lifetime of an `Http2Session` instance.

Refs: nodejs/http2#140
PR-URL: nodejs#16461
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
@cjihrig cjihrig mentioned this pull request Nov 6, 2017
addaleax added a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
addaleax added a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
Add an utility constructor for `AsyncWrap` classes that wish to
leverage `InternalCallbackScope`s.

PR-URL: nodejs/node#16461
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
addaleax added a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
addaleax added a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
When assigning to a union twice, the first assignment is a no-op.

PR-URL: nodejs/node#16461
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
addaleax added a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
Use weak handles to track the lifetime of an `Http2Session` instance.

Refs: nodejs/http2#140
PR-URL: nodejs/node#16461
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. http2 Issues or PRs related to the http2 subsystem. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants