FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

inspector: Use default uv_listen backlog size by eugeneo · Pull Request #20254 · nodejs/node · GitHub

/ node Public

inspector: Use default uv_listen backlog size - #20254

Merged
eugeneo merged 1 commit into
nodejs:masterfrom
eugeneo:http-backlog
Apr 26, 2018
ConversationCommits (1)ChecksFiles changed
Merged

inspector: Use default uv_listen backlog size#20254
eugeneo merged 1 commit into
nodejs:masterfrom
eugeneo:http-backlog

Conversation

eugeneo commented Apr 24, 2018

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

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 commented Apr 24, 2018

Copy link
Copy Markdown
Contributor Author

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 mentioned this pull request Apr 24, 2018
Closed
3 tasks

Trott commented Apr 24, 2018

Copy link
Copy Markdown
Member

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 commented Apr 24, 2018

Copy link
Copy Markdown
Member

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 commented Apr 24, 2018

Copy link
Copy Markdown
Member

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 commented Apr 24, 2018

Copy link
Copy Markdown
Member

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

Trott added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Apr 24, 2018

Trott commented Apr 24, 2018

Copy link
Copy Markdown
Member

@nodejs/v8-inspector

eugeneo commented Apr 24, 2018

Copy link
Copy Markdown
Contributor Author

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 commented Apr 24, 2018

Copy link
Copy Markdown
Contributor Author

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 commented Apr 24, 2018

Copy link
Copy Markdown
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 commented Apr 24, 2018

Copy link
Copy Markdown
Contributor Author

@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 approved these changes Apr 24, 2018
View reviewed changes

bnoordhuis left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread test/parallel/test-inspector-stress-http.js Outdated

bnoordhuis Apr 24, 2018

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Trott previously requested changes Apr 24, 2018
View reviewed changes

Trott left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Apr 24, 2018

Copy link
Copy Markdown
Contributor Author

@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 dismissed their stale review April 24, 2018 23:54

happy to be wrong!

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 commented Apr 26, 2018

Copy link
Copy Markdown
Contributor Author

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

Will land now

eugeneo merged commit 71059fb into nodejs:master Apr 26, 2018

eugeneo commented Apr 26, 2018

Copy link
Copy Markdown
Contributor Author

Landed as 71059fb

eugeneo deleted the http-backlog branch April 26, 2018 17:21
MylesBorins pushed a commit that referenced this pull request May 4, 2018
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 mentioned this pull request May 8, 2018
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

bnoordhuis bnoordhuis approved these changes
Trott Trott approved these changes
trivikr trivikr approved these changes
+2 more reviewers
ofrobots ofrobots approved these changes
TimothyGu TimothyGu approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, 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

Back | FazBrowse Home | New Git URL