Skip to content
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

src: write named pipe info in diagnostic report #38637

Closed
wants to merge 10 commits into from
Closed

src: write named pipe info in diagnostic report #38637

wants to merge 10 commits into from

Conversation

legendecas
Copy link
Member

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

Fixes: #38625

@legendecas legendecas added the report Issues and PRs related to process.report. label May 11, 2021
@legendecas legendecas changed the title src: write pipe info in diagnostic report src: write named pipe info in diagnostic report May 11, 2021
@github-actions github-actions bot 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
@legendecas legendecas force-pushed the node-report/pipe branch 2 times, most recently from 6c0cadd to 6ae29a1 Compare May 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()`.
@nodejs-github-bot

This comment has been minimized.

Sign in to view
mscdex
mscdex reviewed May 11, 2021
View reviewed changes
src/node_report_utils.cc Outdated Show resolved Hide resolved
@nodejs-github-bot

This comment has been minimized.

Sign in to view
addaleax
addaleax requested changes May 12, 2021
View reviewed changes
src/node_report_utils.cc Show resolved Hide resolved
@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
Member Author

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
Member Author

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

@gireeshpunathil
Copy link
Member

LGTM!

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

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
Member Author

@legendecas legendecas May 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

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

@legendecas legendecas 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: #38637
Fixes: #38625
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
@legendecas
Copy link
Member Author

Landed in 50f076c

@legendecas legendecas closed this May 21, 2021
@legendecas legendecas deleted the node-report/pipe branch May 21, 2021 03:42
@legendecas legendecas 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: #38637
Fixes: #38625
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
@danielleadams danielleadams 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: #38637
Fixes: #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: #38637
Fixes: #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: #38637
Fixes: #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: #38637
Fixes: #38625
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
@richardlau richardlau 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#38637
Fixes: nodejs#38625
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mscdex mscdex mscdex left review comments

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@richardlau richardlau richardlau approved these changes

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. report Issues 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