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.

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

Jump to bottom

inspector: Use default uv_listen backlog size #20254

Merged
merged 1 commit into from
Apr 26, 2018
Merged

inspector: Use default uv_listen backlog size #20254

merged 1 commit into from
Apr 26, 2018

Conversation

eugeneo
Copy link
Contributor

@eugeneo eugeneo commented Apr 24, 2018

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

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v4.x inspector Issues and PRs related to the V8 inspector protocol labels Apr 24, 2018
@eugeneo
Copy link
Contributor Author

eugeneo commented Apr 24, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/14478/

So far, this issue was only hit by a test case but there's a potential for it to be hit if some tool sends too many requests too fast.

@eugeneo eugeneo mentioned this pull request Apr 24, 2018
Closed
3 tasks
@Trott
Copy link
Member

Trott commented Apr 24, 2018

The provided test doesn't fail when node is compiled without the source change. Shouldn't it? Or is it a "sometimes it fails, sometimes it doesn't" kind of test?

@Trott
Copy link
Member

Trott commented Apr 24, 2018

The provided test doesn't fail when node is compiled without the source change. Shouldn't it? Or is it a "sometimes it fails, sometimes it doesn't" kind of test?

The problem is that if it starts failing that way, then no one is going to think it's a bug in the code. They're going to think the test is unreliable and "fix" it rather than the bug in the code.

@Trott
Copy link
Member

Trott commented Apr 24, 2018

Although I did just confirm that this un-breaks parallel/test-inspector-multisession-ws from #20137 on macOS. So, maybe that's the "real" test for this. My question above is non-blocking (at least, non-blocking for me).

@Trott
Copy link
Member

Trott commented Apr 24, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/14479/

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 24, 2018
@Trott
Copy link
Member

Trott commented Apr 24, 2018

@nodejs/v8-inspector

@eugeneo
Copy link
Contributor Author

eugeneo commented Apr 24, 2018

The provided test doesn't fail when node is compiled without the source change. Shouldn't it? Or is it a "sometimes it fails, sometimes it doesn't" kind of test?

It reliably fails for me on macOS. But it is timing related - new client needs to attempt connection while the inspector still have not accepted the previous connection.

@eugeneo
Copy link
Contributor Author

eugeneo commented Apr 24, 2018

Looks like the test fails on Docker bots. I will reduce the number of connection attempts to 10 and will try again. I tried 10 connections on my Mac and I see failures roughly 8 runs out of 10.

@ofrobots
Copy link
Contributor

I am trying to find out how the value 0 ends up picking the uv default size. AFAICT, uv_listen simply passes this onto listen(2) here. I see no indication that 0 is special as far as uv is concerned.

In the man pages for either mac or on Linux, I cannot find reference to the value 0 being special either. Am I missing something?

@eugeneo
Copy link
Contributor Author

eugeneo commented Apr 24, 2018

@ofrobots you are right, that seems to be a bad value. I simply tried it while debugging the test value, it worked on Mac but seems to be a bad value for Linux. I changed to 511 as that's what the net module uses - https://github.com/nodejs/node/blob/master/lib/net.js#L1327

bnoordhuis
bnoordhuis approved these changes Apr 24, 2018
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM with a comment about the test.

In the man pages for either mac or on Linux, I cannot find reference to the value 0 being special either.

It is on macos; 0 makes it default to kern.ipc.somaxconn (normally 128.)

test/parallel/test-inspector-stress-http.js Outdated
console.log(`Interation ${number} done`);
return response;
} catch (e) {
console.error(`Iteration ${number} failed`, e);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the exception bubble up? Also, there seems to be no point in returning response.

Trott
Trott previously requested changes Apr 24, 2018
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Many relevant CI failures along the lines of:

16:51:36 not ok 1862 parallel/test-inspector-stress-http
16:51:36   ---
16:51:36   duration_ms: 120.69
16:51:36   severity: fail
16:51:36   exitcode: -15
16:51:36   stack: |-
16:51:36     timeout

Moving the test to sequential rather than parallel may fix it, or at least help a bit.

@eugeneo
Copy link
Contributor Author

eugeneo commented Apr 24, 2018

@Trott - I believe that was an old run. That timeout was a result of the 0 backlog on Linux.

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

@Trott Trott dismissed their stale review April 24, 2018 23:54

happy to be wrong!

@eugeneo
inspector: Use default uv_listen backlog size
71059fb 
PR-URL: #20254
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@eugeneo
Copy link
Contributor Author

eugeneo commented Apr 26, 2018

Test was updated, the CI is still green: https://ci.nodejs.org/job/node-test-pull-request/14529/

Will land now

@eugeneo eugeneo merged commit 71059fb into nodejs:master Apr 26, 2018
@eugeneo
Copy link
Contributor Author

eugeneo commented Apr 26, 2018

Landed as 71059fb

@eugeneo eugeneo deleted the http-backlog branch April 26, 2018 17:21
MylesBorins pushed a commit that referenced this pull request May 4, 2018
@MylesBorins
inspector: Use default uv_listen backlog size
375994f 
PR-URL: #20254
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@MylesBorins MylesBorins mentioned this pull request May 8, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trivikr trivikr trivikr approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@Trott Trott Trott approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@ofrobots ofrobots ofrobots approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@eugeneo @Trott @ofrobots @bnoordhuis @TimothyGu @trivikr @nodejs-github-bot