Skip to content

src: write named pipe info in diagnostic report#38637

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
legendecas wants to merge 10 commits into from
Closed

src: write named pipe info in diagnostic report #38637

legendecas wants to merge 10 commits into from

Conversation

@legendecas
Copy link
Member

@legendecaslegendecas commented May 11, 2021

Writes pipe handles with uv_pipe_getsockname() and uv_pipe_getpeername().

Fixes: #38625

@legendecaslegendecas added the report Issues and PRs related to process.report. label May 11, 2021
@legendecaslegendecas changed the title src: write pipe info in diagnostic reportsrc: write named pipe info in diagnostic reportMay 11, 2021
@github-actionsgithub-actionsbot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels May 11, 2021
@legendecaslegendecasforce-pushed the node-report/pipe branch 2 times, most recently from 6c0cadd to 6ae29a1CompareMay 11, 2021 15:43
@legendecas
src: write named pipe info in diagnostic report
534db1a
Writes pipe handles with `uv_pipe_getsockname()` and `uv_pipe_getpeername()`.
gireeshpunathil
gireeshpunathil reviewed May 11, 2021
View reviewed changes
@nodejs-github-bot

This comment has been minimized.

Sign in to view
mscdex
mscdex reviewed May 11, 2021
View reviewed changes
richardlau
richardlau reviewed May 12, 2021
View reviewed changes
richardlau
richardlau reviewed May 12, 2021
View reviewed changes
@nodejs-github-bot

This comment has been minimized.

Sign in to view
addaleax
addaleax requested changes May 12, 2021
View reviewed changes
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@legendecas
Copy link
MemberAuthor

legendecas commented May 18, 2021

Just set up a windows environment to test around the test suite compatibility issue. I will update once the test gets fixed.

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@legendecas
Copy link
MemberAuthor

legendecas commented May 19, 2021

@addaleax@gireeshpunathil@richardlau updated to pass windows tests. PTAL :)

@gireeshpunathil
Copy link
Member

gireeshpunathil commented May 19, 2021

LGTM!

richardlau
richardlau reviewed May 19, 2021
View reviewed changes
test/report/test-report-uv-handles.js Outdated
// Exit on loss of parent process
constexit=()=>process.exit(2);
process.on('disconnect',exit);
functionpipeName(windowsExtended){
Copy link
Member

@richardlaurichardlauMay 19, 2021

Choose a reason for hiding this comment

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

This function is only called with windowsExtended set to true so the parameter is superfluous?

Copy link
MemberAuthor

@legendecaslegendecasMay 19, 2021•
edited
Loading

Choose a reason for hiding this comment

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

Updated common.PIPE to use '\\?\' prefix. Most likely there should not be any difference regarding the pipe API. However my local windows VM doesn't behave well and lots of unrelated test cases were falling for unknown reasons. So just pushed the updates and let's see what the CI says.

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@legendecas
fixup! test: windows fanned test failed
d5d6ee7
various pipe connect API failed in parallel/test-net-connect-options-path.
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 20, 2021•
edited by legendecas
Loading

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 20, 2021•
edited by legendecas
Loading

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 20, 2021•
edited by legendecas
Loading

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 20, 2021

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

@legendecaslegendecas added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 20, 2021
legendecas added a commit that referenced this pull request May 21, 2021
@legendecas
src: write named pipe info in diagnostic report
50f076c
Writes pipe handles with `uv_pipe_getsockname()` and `uv_pipe_getpeername()`. PR-URL: #38637Fixes: #38625 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
@legendecas
Copy link
MemberAuthor

legendecas commented May 21, 2021

Landed in 50f076c

@legendecaslegendecas deleted the node-report/pipe branch May 21, 2021 03:42
@legendecaslegendecas removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 21, 2021
danielleadams pushed a commit that referenced this pull request May 31, 2021
@legendecas@danielleadams
src: write named pipe info in diagnostic report
61c95f0
Writes pipe handles with `uv_pipe_getsockname()` and `uv_pipe_getpeername()`. PR-URL: #38637Fixes: #38625 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
@danielleadamsdanielleadams mentioned this pull request May 31, 2021
Merged
richardlau pushed a commit that referenced this pull request Jul 16, 2021
@legendecas@richardlau
src: write named pipe info in diagnostic report
d39cf8b
Writes pipe handles with `uv_pipe_getsockname()` and `uv_pipe_getpeername()`. PR-URL: #38637Fixes: #38625 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
richardlau pushed a commit that referenced this pull request Jul 16, 2021
@legendecas@richardlau
src: write named pipe info in diagnostic report
b3a3ea1
Writes pipe handles with `uv_pipe_getsockname()` and `uv_pipe_getpeername()`. PR-URL: #38637Fixes: #38625 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
richardlau pushed a commit that referenced this pull request Jul 19, 2021
@legendecas@richardlau
src: write named pipe info in diagnostic report
f9c7def
Writes pipe handles with `uv_pipe_getsockname()` and `uv_pipe_getpeername()`. PR-URL: #38637Fixes: #38625 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
richardlau pushed a commit that referenced this pull request Jul 20, 2021
@legendecas@richardlau
src: write named pipe info in diagnostic report
846a138
Writes pipe handles with `uv_pipe_getsockname()` and `uv_pipe_getpeername()`. PR-URL: #38637Fixes: #38625 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
@richardlaurichardlau mentioned this pull request Jul 20, 2021
Merged
foxxyz pushed a commit to foxxyz/node that referenced this pull request Oct 18, 2021
@legendecas@foxxyz
src: write named pipe info in diagnostic report
0b95ad5
Writes pipe handles with `uv_pipe_getsockname()` and `uv_pipe_getpeername()`. PR-URL: nodejs#38637Fixes: nodejs#38625 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleaxaddaleaxaddaleax approved these changes

@richardlaurichardlaurichardlau approved these changes

@gireeshpunathilgireeshpunathilgireeshpunathil approved these changes

+1 more reviewer

@mscdexmscdexmscdex left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++Issues and PRs that require attention from people who are familiar with C++.needs-ciPRs that need a full CI run.reportIssues and PRs related to process.report.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Feature Request: include missing pipe info in diagnostic report

6 participants

@legendecas@nodejs-github-bot@gireeshpunathil@mscdex@addaleax@richardlau