Skip to content

src: check whether inspector is doing io - #13504

Merged
sam-github merged 1 commit into
nodejs:masterfrom
sam-github:check-is-listening
Jun 9, 2017
Merged

src: check whether inspector is doing io#13504
sam-github merged 1 commit into
nodejs:masterfrom
sam-github:check-is-listening

Conversation

@sam-github

@sam-githubsam-github commented Jun 6, 2017

Copy link
Copy Markdown
Contributor

Inspector start means that it exists, but doesn't mean it is listening
on a port, that only happens if it is doing I/O (i.e. has an io object).

Fixes: #13499

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)

src,inspector

@nodejs-github-botnodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jun 6, 2017
@mscdexmscdex added the inspector Issues and PRs related to the V8 inspector protocol label Jun 6, 2017
@sam-github

Copy link
Copy Markdown
ContributorAuthor

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@eugeneo can we have some of these in #13478?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I added some tests there

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why not default arg port = ''

@sam-github

Copy link
Copy Markdown
ContributorAuthor

@refack Thanks for your suggestion, it was a good one.

@sam-github

Copy link
Copy Markdown
ContributorAuthor

Comment threadsrc/node.cc Outdated

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.

DRY: if (auto io = env->inspector_agent()->io()) port = io->port();?

Should it check io->IsStarted()? The inspector has a venerable multitude of IsStarted methods...

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

No. IsStarted() should be called IsInitialized(), and it always happens if the Agent was constructed, unless I very much misremember. I'll check again, but I'm pretty sure. And I agree, the word "start" in method names is a bit overused in node's c++ source.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

yes, IsStarted() means its newed a NodeInspectorClient client_, its the io_ that means its listening on a port.

@sam-github

Copy link
Copy Markdown
ContributorAuthor

I changed the C++, so ci again: https://ci.nodejs.org/job/node-test-pull-request/8565/

@sam-github

Copy link
Copy Markdown
ContributorAuthor

OS X failed on

not ok 174 async-hooks/test-callback-error

@addaleax (in the absence of an at-nodejs/async-hooks)

AIX failed on

not ok 13 async-hooks/test-fseventwrap

@gireeshpunathil would you know anything about that?

Neither look related.

@addaleax

Copy link
Copy Markdown
Member

OS X failed on

not ok 174 async-hooks/test-callback-error

@addaleax (in the absence of an at-nodejs/async-hooks)

we do have @nodejs/async_hooks ;) I don’t really know how that test would be flaky.

@gibfahn

Copy link
Copy Markdown
Member

not ok 13 async-hooks/test-fseventwrap

#13577 was just raised, so I believe you're okay there.

@sam-github

Copy link
Copy Markdown
ContributorAuthor

ci: https://ci.nodejs.org/job/node-test-pull-request/8578/ to see if the async hooks test always fails

@gibfahn

Copy link
Copy Markdown
Member

not ok 174 async-hooks/test-callback-error

1000 runs on my machine (macOS sierra) with no failures, I think this is good to go.

@AndreasMadsen

Copy link
Copy Markdown
Member

I don’t really know how that test would be flaky.

It is a real mystery. See: #13527

@sam-github

Copy link
Copy Markdown
ContributorAuthor

OK, everything passed this time except for

not ok 1432 inspector/test-inspector-port-zero-cluster # TODO : Fix flaky test

on centos: https://ci.nodejs.org/job/node-test-commit-linux/10482/nodes=centos7-64/

Inspector start means that it exists, but doesn't mean it is listening
on a port, that only happens if it is doing I/O (i.e. has an io object).
PR-URL: nodejs#13504Fixes: nodejs#13499
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@sam-github
sam-github merged commit c4a61b3 into nodejs:masterJun 9, 2017
@sam-github
sam-github deleted the check-is-listening branch June 9, 2017 16:13
@gibfahn

Copy link
Copy Markdown
Member

not ok 1432 inspector/test-inspector-port-zero-cluster # TODO : Fix flaky test

Should really have fixed that flaky test before landing this @sam-github 😉

@refack

Copy link
Copy Markdown
Contributor

not ok 174 async-hooks/test-callback-error

I don’t really know how that test would be flaky.

It is a real mystery. See: #13527

It just is 🤷‍♂️ (probably the process.abort taking too long)
#13559

addaleax pushed a commit that referenced this pull request Jun 10, 2017
Inspector start means that it exists, but doesn't mean it is listening
on a port, that only happens if it is doing I/O (i.e. has an io object).
PR-URL: #13504Fixes: #13499
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@addaleaxaddaleax mentioned this pull request Jun 10, 2017
@gibfahngibfahn mentioned this pull request Jun 15, 2017
3 tasks
Sign up for freeto 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++.inspectorIssues and PRs related to the V8 inspector protocol

Projects

None yet

Development

Successfully merging this pull request may close these issues.

inspector-port=0 edge case crash

11 participants

@sam-github@addaleax@gibfahn@AndreasMadsen@refack@bnoordhuis@eugeneo@cjihrig@mscdex@MylesBorins@nodejs-github-bot