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

test: http2 client socket operations #16211

Closed

Conversation

trivikr
Copy link
Member

@trivikr trivikr commented Oct 15, 2017

This commit tests pause, resume, drain operations on client.socket

Refs: #14985

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test, http2

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 15, 2017
@hiroppy hiroppy added the http2 Issues or PRs related to the http2 subsystem. label Oct 15, 2017
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.

Thank you for starting this work @trivikr. Some general feedback: I understand that prettier is currently a popular code formatter but it goes against established style within the node project and also has a tendency to really bloat the indent level of code. It also tends to obscure git history for purely stylistic reasons.

common.skip('missing crypto');
const h2 = require('http2');

// test client.socket.destroy()
Copy link
Member

Choose a reason for hiding this comment

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

I don't think much is gained by moving this here instead of having it in a separate file. The server still has to be instantiated separately and the two test cases aren't really connected. We also lose the history of the other file. I would strongly prefer to have the other test restored and have this taken out.

Copy link
Member Author

Choose a reason for hiding this comment

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

Restored the code in test-http2-client-socket-destroy.js in new commit

// Force 'resume' when _handle.reading is true
client.socket.emit('resume');

client.socket.emit('drain');
Copy link
Member

Choose a reason for hiding this comment

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

I'll leave this up to @jasnell & @mcollina but I don't think this is actually testing what it claims to. Feels like a way to bump up code coverage without truly testing the functionality which is worse than not having a test since it hides potential problem areas.

Unfortunately I don't really have great suggestion for how to properly test it given how the socket is used in h2.

const server = h2.createServer();
server.on(
'stream',
common.mustCall((stream) => {
Copy link
Member

Choose a reason for hiding this comment

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

Ideally this would be on one line with server.on('stream', common.mustCall((stream) => {. See my general note re: using prettier.

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved it to one line in new commit

@trivikr trivikr force-pushed the test-http2-client-socket-operations branch from 648e021 to 7b8f50d Compare October 15, 2017 07:20
server.on('stream', common.mustCall((stream) => {
stream.respond();
stream.end();
})
Copy link
Member

Choose a reason for hiding this comment

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

Indent will need to be adjusted throughout to pass the linter.

Copy link
Member Author

Choose a reason for hiding this comment

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

Adjusted indentation and confirmed that make lint-js passes in new commit


req.on(
'response',
common.mustCall(() => {
Copy link
Member

@apapirovski apapirovski Oct 15, 2017

Choose a reason for hiding this comment

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

Same as above, indent level is off now. I don't think this will pass the linter.

Nit: this would normally be one line in node req.on('response', common.mustCall(() => {. Same below for end. (Which also reduces the indentation by one level inside.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Adjusted indentation and confirmed that make lint-js passes in new commit

@trivikr trivikr force-pushed the test-http2-client-socket-operations branch from 7b8f50d to e700986 Compare October 15, 2017 07:28
common.mustCall(() => {
client.socket.pause();
// Force 'pause' when _handle.reading is false
client.socket.emit('pause');
Copy link
Member

Choose a reason for hiding this comment

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

Reposting this since github incorrectly hides it.

I'll leave this up to @jasnell & @mcollina but I don't think this is actually testing what it claims to. Feels like a way to bump up code coverage without truly testing the functionality which is worse than not having a test since it hides potential problem areas.

Unfortunately I don't really have great suggestion for how to properly test it given how the socket is used in h2.

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'd tried mocking calls to readStart and readStop the same way as submitSettings is mocked as discussed in #16083 (comment)

However, the call was not reaching the mock. I'll wait for inputs from @jasnell / @mcollina
I agree that current commit doesn't confirm that pause, resume and drain methods were called.

Copy link
Member Author

@trivikr trivikr Oct 15, 2017

Choose a reason for hiding this comment

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

One solution I can think of is ensuring that operations were called on client.socket and testing value stored in client.socket._handle.reading as follows:

client.socket.on('pause', common.mustCall(() => {
  assert(!client.socket._handle.reading);
}, 2));
client.socket.on('resume', common.mustCall(() => {
  assert(client.socket._handle.reading);
}, 2));
client.socket.on('drain', common.mustCall());

@jasnell jasnell requested a review from mcollina October 15, 2017 22:27
@mcollina
Copy link
Member

The socket on the session should really not be used, because it is managed in C++. So whatever this is testing is having no net effect on the flow of data.

@apapirovski do you think we should move your Proxy technique to the session level? It would avoid this to come up. I'm not sure it won't slow things down.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

To be clear, I think this should not land.

@apapirovski
Copy link
Member

The code this is dealing with, which I believe @jasnell added in his original implementation, is here:

function socketOnResume() {
if (this._paused)
return this.pause();
if (this._handle && !this._handle.reading) {
this._handle.reading = true;
this._handle.readStart();
}
}
function socketOnPause() {
if (this._handle && this._handle.reading) {
this._handle.reading = false;
this._handle.readStop();
}
}
function socketOnDrain() {
const needPause = 0 > this._writableState.highWaterMark;
if (this._paused && !needPause) {
this._paused = false;
this.resume();
}
}

I'll let him speak to its purpose but as far as I'm concerned, using the proxy socket and removing this code might be preferable. I can't really think of a use case to pausing or resuming the socket manually, even if there's code to potentially handle it.

When I tested the proxy performance on compat it seemed to have literally no impact so I can't imagine the overhead is that big.

@jasnell
Copy link
Member

jasnell commented Oct 16, 2017

This code was added defensively with the intent of circling back to see if it's even needed. It was copied nearly verbatim from the existing http module. Given how we are reading and writing data from the socket at the native layer, I doubt that this code would even be exercised in normal cases. Perhaps we should just remove these handlers and see if it makes any difference whatsoever :-)

@apapirovski
Copy link
Member

apapirovski commented Oct 16, 2017

Do you have strong feelings one way or another re: the Proxy socket implementation living on the session instead if in compatibility? It would give us the option of throwing when users try to directly write, read, etc. as we currently do in compatibility mode. (Assuming the performance turns out to be fine, of course.)

(The real socket would still live at kSocket and be used internally.)

@jasnell
Copy link
Member

jasnell commented Oct 16, 2017

I think that makes sense. At some point in the future it might make sense to have some kind of official escape hatch for getting to the actual socket, but for now, hiding it behind the proxy makes the most sense as long as the proxy is never used internally.

@trivikr
Copy link
Member Author

trivikr commented Oct 17, 2017

@mcollina @jasnell @apapirovski
Can one of you create an issue for discussion so that this pull request can be closed without landing?

@trivikr
Copy link
Member Author

trivikr commented Oct 17, 2017

Closing as issue is created at #16252

@trivikr trivikr closed this Oct 17, 2017
@trivikr trivikr deleted the test-http2-client-socket-operations branch October 22, 2017 22:02
apapirovski added a commit to apapirovski/node that referenced this pull request Oct 24, 2017
Because of the specific serialization and processing
requirements of HTTP/2, sockets should not be
directly manipulated. This forbids any interactions
with destroy, emit, end, once, on, pause, read,
resume and write methods of the socket. It also
redirects setTimeout to session instead of socket.

Fixes: nodejs#16252
Refs: nodejs#16211
apapirovski added a commit that referenced this pull request Oct 25, 2017
Because of the specific serialization and processing requirements
of HTTP/2, sockets should not be directly manipulated. This
forbids any interactions with destroy, emit, end, pause, read,
resume and write methods of the socket. It also redirects
setTimeout to session instead of socket.

PR-URL: #16330
Fixes: #16252
Refs: #16211
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
Because of the specific serialization and processing requirements
of HTTP/2, sockets should not be directly manipulated. This
forbids any interactions with destroy, emit, end, pause, read,
resume and write methods of the socket. It also redirects
setTimeout to session instead of socket.

PR-URL: nodejs/node#16330
Fixes: nodejs/node#16252
Refs: nodejs/node#16211
Reviewed-By: James M Snell <jasnell@gmail.com>
gibfahn pushed a commit that referenced this pull request Oct 30, 2017
Because of the specific serialization and processing requirements
of HTTP/2, sockets should not be directly manipulated. This
forbids any interactions with destroy, emit, end, pause, read,
resume and write methods of the socket. It also redirects
setTimeout to session instead of socket.

PR-URL: #16330
Fixes: #16252
Refs: #16211
Reviewed-By: James M Snell <jasnell@gmail.com>
gibfahn pushed a commit that referenced this pull request Oct 30, 2017
Because of the specific serialization and processing requirements
of HTTP/2, sockets should not be directly manipulated. This
forbids any interactions with destroy, emit, end, pause, read,
resume and write methods of the socket. It also redirects
setTimeout to session instead of socket.

PR-URL: #16330
Fixes: #16252
Refs: #16211
Reviewed-By: James M Snell <jasnell@gmail.com>
gibfahn pushed a commit that referenced this pull request Oct 31, 2017
Because of the specific serialization and processing requirements
of HTTP/2, sockets should not be directly manipulated. This
forbids any interactions with destroy, emit, end, pause, read,
resume and write methods of the socket. It also redirects
setTimeout to session instead of socket.

PR-URL: #16330
Fixes: #16252
Refs: #16211
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
Because of the specific serialization and processing requirements
of HTTP/2, sockets should not be directly manipulated. This
forbids any interactions with destroy, emit, end, pause, read,
resume and write methods of the socket. It also redirects
setTimeout to session instead of socket.

PR-URL: nodejs/node#16330
Fixes: nodejs/node#16252
Refs: nodejs/node#16211
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http2 Issues or PRs related to the http2 subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants