Skip to content

http2: use and support non-empty DATA frame with END_STREAM flag #33875

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.

Sign up for GitHub

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

Closed
clshortfuse wants to merge 14 commits into from
Closed

http2: use and support non-empty DATA frame with END_STREAM flag #33875

Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
ca21bd1
http2: use and support non-empty DATA frame with END_STREAM flag
clshortfuse Jun 14, 2020
1e35afd
remove whitespace
clshortfuse Jun 14, 2020
30b1c06
Update node_http2.cc
clshortfuse Jun 14, 2020
6f7c361
Update node_http2.cc
clshortfuse Jun 15, 2020
cbddeb6
Update node_http2.cc
clshortfuse Jun 15, 2020
acb5999
Update node_http2.cc
clshortfuse Jun 15, 2020
85cf722
Update node_http2.cc
clshortfuse Jun 15, 2020
7bae5a5
http2: add "receive paused" debug native line
clshortfuse Jun 16, 2020
789cc1d
http2: remove empty DATA frame
clshortfuse Jun 17, 2020
0c110d0
http2: update tests
clshortfuse Jun 17, 2020
563a310
fix trailers flag detection
clshortfuse Jun 18, 2020
327e9e8
fix tests
clshortfuse Jun 18, 2020
58a3885
add test
clshortfuse Jun 18, 2020
1778fc6
add state.shutdownWritableCalled
clshortfuse Jun 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Update node_http2.cc
  • Loading branch information
@clshortfuse
clshortfuse committed Jun 18, 2020
commit 30b1c06ae04c20b7da32ef6498c9f0de16d66664
8 changes: 1 addition & 7 deletions src/node_http2.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -755,7 +755,7 @@ ssize_t Http2Session::ConsumeHTTP2Data() {
CHECK_GT(ret, 0);
CHECK_LE(static_cast<size_t>(ret), read_len);

if (static_cast<size_t>(ret) < read_len) {
if (static_cast<size_t>(ret) <= read_len) {

@addaleax addaleax Jun 14, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hm – the CHECK_LE above this verifies that this is always the case, right?

(Also, thanks for digging into this so deeply!)

@clshortfuse clshortfuse Jun 15, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I spotted that too, but I wasn't sure. It's very possible this all comes from a single missing character!

@clshortfuse clshortfuse Jun 15, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In reality, the whole conditional of if (static_cast<size_t>(ret) < read_len) { is suspect. I don't think we need it, but I leaned on <= instead because it replicates the CHECK_LE line.

@addaleax addaleax Jun 15, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hm … trying to think this through: If reading is paused but we still consumed the entire buffer, doesn’t that mean that we should clear the Buffer in the code below (i.e. stream_buf_offset_ = 0; etc)?

@clshortfuse clshortfuse Jun 15, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We don't need the buffer, no. But we can't reset stream_buf_offset_ because we need to recall ConsumeHTTP2Data() later:

https://github.com/nodejs/node/blob/f46ca0fd29361215ac8a26cd6647dc45221eb9d7/src/node_http2.cc#L1457-1459

I was thinking (stream_buf_offset_ > 0 || is_receive_paused()), but then it'll try to run ConsumeHTTP2Data() every time you write something, even if the readable side is completed.

@addaleax addaleax Jun 15, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@clshortfuse So, just to make sure I understand this correctly: We should also call nghttp2_session_mem_recv() even if no new data is available from the socket, and the previous nghttp2_session_mem_recv() call reported all data as read, if we entered pause state during that previous call?

@addaleax addaleax Jun 15, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe we just shouldn’t pause in the first place if NGHTTP2_FLAG_END_STREAM is set?

@clshortfuse clshortfuse Jun 15, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@addaleax Yes. I tried to keep the comment to the point, since I'm known for blabbing 🙃, but the nghttp2 documentation states:

If the application uses nghttp2_session_mem_recv(), it can return NGHTTP2_ERR_PAUSE to make nghttp2_session_mem_recv() return without processing further input bytes. The memory by pointed by the data is retained until nghttp2_session_mem_recv() or nghttp2_session_recv() is called. The application must retain the input bytes which was used to produce the data parameter, because it may refer to the memory region included in the input bytes.

https://nghttp2.org/documentation/types.html#c.nghttp2_on_data_chunk_recv_callback

@clshortfuse clshortfuse Jun 16, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Maybe we just shouldn’t pause in the first place if NGHTTP2_FLAG_END_STREAM is set?

Sorry, I missed this. That is entirely possible, but it would be less efficient memory-wise. The sooner we pause to send out an output chunk, the sooner it's released from RAM. Also, we have no way of knowing how many times the current data frame (though it has an END_STREAM flag) will trigger a read callback. Frames can be upto 16MiB in size. So, hypothetically, if nghttp2 is sending us things in 1KB chunks, it could take 16,384 iterations before we send out whatever data we want. That could be any outbound frame type, including PING responses.

// Mark the remainder of the data as available for later consumption.
stream_buf_offset_ += ret;
return ret;
Expand Down Expand Up @@ -1090,12 +1090,6 @@ int Http2Session::OnDataChunkReceived(nghttp2_session* handle,
}
} while (len != 0);

// If end-stream flag is set, there is nothing more to read
if (flags & NGHTTP2_FLAG_END_STREAM) {
stream->EmitRead(UV_EOF);
return 0;
}

// If we are currently waiting for a write operation to finish, we should
// tell nghttp2 that we want to wait before we process more input data.
if (session->is_write_in_progress()) {
Expand Down