Skip to content

Comments

src: write named pipe info in diagnostic report#38637

Closed
legendecas wants to merge 10 commits intonodejs:masterfrom
legendecas:node-report/pipe
Closed

src: write named pipe info in diagnostic report#38637
legendecas wants to merge 10 commits intonodejs:masterfrom
legendecas:node-report/pipe

Conversation

@legendecas
Copy link
Member

@legendecas legendecas commented May 11, 2021

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()`.
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
Member Author

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

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
const exit = () => process.exit(2);
process.on('disconnect', exit);
function pipeName(windowsExtended) {
Copy link
Member

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

nodejs-github-bot commented May 20, 2021

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

legendecas commented May 21, 2021

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

@addaleax addaleax addaleax approved these changes

@richardlau richardlau richardlau approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

+1 more reviewer

@mscdex mscdex mscdex 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-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