Skip to content

net: report uv_tcp_open() errors - #21428

Merged
cjihrig merged 1 commit into
nodejs:masterfrom
cjihrig:tcp-open
Jun 22, 2018
Merged

net: report uv_tcp_open() errors#21428
cjihrig merged 1 commit into
nodejs:masterfrom
cjihrig:tcp-open

Conversation

@cjihrig

Copy link
Copy Markdown
Contributor

uv_tcp_open() can fail. Prior to this commit, any error was being silently ignored. This commit raises the errors.

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

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. net Issues and PRs related to the net subsystem. labels Jun 20, 2018

@bnoordhuisbnoordhuis 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.

LGTM with a question.

Comment threadsrc/tcp_wrap.cc Outdated
int fd = static_cast<int>(args[0]->IntegerValue());
uv_tcp_open(&wrap->handle_, fd);
int err = uv_tcp_open(&wrap->handle_, fd);
wrap->set_fd(fd);

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.

Should this be if (err == 0) wrap->set_fd(fd);?

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.

It certainly could be. I'll update it. PipeWrap() does it this way, but it also throws from C++ instead of returning the error code.

uv_tcp_open() can fail. Prior to this commit, any error was
being silently ignored. This commit raises the errors.
PR-URL: nodejs#21428
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@cjihrig

Copy link
Copy Markdown
ContributorAuthor

Nit addressed. CI: https://ci.nodejs.org/job/node-test-pull-request/15565/. Only failure is #21425.

@cjihrig
cjihrig merged commit 49ea06f into nodejs:masterJun 22, 2018
@cjihrig
cjihrig deleted the tcp-open branch June 22, 2018 14:43
targos pushed a commit that referenced this pull request Jun 24, 2018
uv_tcp_open() can fail. Prior to this commit, any error was
being silently ignored. This commit raises the errors.
PR-URL: #21428
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targostargos mentioned this pull request Jul 3, 2018
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++.netIssues and PRs related to the net subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@cjihrig@nodejs-github-bot@bnoordhuis@jasnell