Skip to content

tls_wrap: reach error reporting for UV_EPROTO#4885

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
Closed
indutny wants to merge 2 commits into from
Closed

tls_wrap: reach error reporting for UV_EPROTO #4885

indutny wants to merge 2 commits into from
+55 −11

Conversation

@indutny
Copy link
Member

@indutnyindutny commented Jan 26, 2016

Do not swallow error details when reporting UV_EPROTO asynchronously,
and when creating artificial errors.

Fix: #3692

R = @bnoordhuis or @shigeki

cc @nodejs/crypto

@indutny
tls_wrap: reach error reporting for UV_EPROTO
80364ef
Do not swallow error details when reporting UV_EPROTO asynchronously, and when creating artificial errors. Fix: nodejs#3692
@indutnyindutny mentioned this pull request Jan 26, 2016
Closed
@indutny
Copy link
MemberAuthor

indutny commented Jan 26, 2016

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

@indutnyindutny added tls Issues and PRs related to the tls subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. lts-watch-v4.x labels Jan 26, 2016
@shigeki
Copy link
Contributor

shigeki commented Jan 27, 2016

LGTM

bnoordhuis
bnoordhuis reviewed Jan 27, 2016
View reviewed changes
src/tls_wrap.cc Outdated
Copy link
Member

@bnoordhuisbnoordhuisJan 27, 2016

Choose a reason for hiding this comment

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

I'd add a CHECK_EQ(error_, nullptr), otherwise it's a memory leak waiting to happen.

EDIT: Or call ClearError() first.

Copy link
MemberAuthor

@indutnyindutnyJan 27, 2016

Choose a reason for hiding this comment

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

Ack.

@indutny
Copy link
MemberAuthor

indutny commented Jan 27, 2016

@bnoordhuis replied and fixed.

@jasnell
Copy link
Member

jasnell commented Jan 27, 2016

LGTM

@indutny
Copy link
MemberAuthor

indutny commented Jan 27, 2016

@bnoordhuis PTAL ;)

@bnoordhuis
Copy link
Member

bnoordhuis commented Jan 27, 2016

LGTM

@indutny
Copy link
MemberAuthor

indutny commented Jan 27, 2016

Landed in ff4006c, thank you everyone!

@indutnyindutny closed this Jan 27, 2016
indutny added a commit that referenced this pull request Jan 27, 2016
@indutny
tls_wrap: reach error reporting for UV_EPROTO
ff4006c
Do not swallow error details when reporting UV_EPROTO asynchronously, and when creating artificial errors. Fix: #3692 PR-URL: #4885 Reviewed-By: Shigeki Ohtsu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
rvagg pushed a commit that referenced this pull request Jan 28, 2016
@indutny@rvagg
tls_wrap: reach error reporting for UV_EPROTO
fd2a95b
Do not swallow error details when reporting UV_EPROTO asynchronously, and when creating artificial errors. Fix: #3692 PR-URL: #4885 Reviewed-By: Shigeki Ohtsu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
rvagg pushed a commit that referenced this pull request Feb 8, 2016
@indutny@rvagg
tls_wrap: reach error reporting for UV_EPROTO
d6fbd81
Do not swallow error details when reporting UV_EPROTO asynchronously, and when creating artificial errors. Fix: #3692 PR-URL: #4885 Reviewed-By: Shigeki Ohtsu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
@rvaggrvagg mentioned this pull request Feb 8, 2016
Closed
MylesBorins pushed a commit that referenced this pull request Feb 17, 2016
@indutny
tls_wrap: reach error reporting for UV_EPROTO
2778ccb
Do not swallow error details when reporting UV_EPROTO asynchronously, and when creating artificial errors. Fix: #3692 PR-URL: #4885 Reviewed-By: Shigeki Ohtsu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 18, 2016
@indutny
tls_wrap: reach error reporting for UV_EPROTO
c908ff3
Do not swallow error details when reporting UV_EPROTO asynchronously, and when creating artificial errors. Fix: #3692 PR-URL: #4885 Reviewed-By: Shigeki Ohtsu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Feb 18, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
@indutny
tls_wrap: reach error reporting for UV_EPROTO
03944fe
Do not swallow error details when reporting UV_EPROTO asynchronously, and when creating artificial errors. Fix: #3692 PR-URL: #4885 Reviewed-By: Shigeki Ohtsu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
@indutny
tls_wrap: reach error reporting for UV_EPROTO
8cece20
Do not swallow error details when reporting UV_EPROTO asynchronously, and when creating artificial errors. Fix: nodejs#3692 PR-URL: nodejs#4885 Reviewed-By: Shigeki Ohtsu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

c++Issues and PRs that require attention from people who are familiar with C++.tlsIssues and PRs related to the tls subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@indutny@shigeki@jasnell@bnoordhuis@MylesBorins