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

flaky: parallel.test-child-process-fork-net #51813

Closed
mhdawson opened this issue Feb 20, 2024 · 3 comments
Closed

flaky: parallel.test-child-process-fork-net #51813

mhdawson opened this issue Feb 20, 2024 · 3 comments
Labels
flaky-test Issues and PRs related to the tests with unstable failures on the CI. windows Issues and PRs related to the Windows platform.

Comments

@mhdawson
Copy link
Member

Test

parallel.test-child-process-fork-net

Platform

Windows

Console output

Add description
Error Message
fail (1)
Stacktrace
---
duration_ms: 567.994
exitcode: 1
severity: fail
...

Build links

Additional information

No response
@mhdawson mhdawson added the flaky-test Issues and PRs related to the tests with unstable failures on the CI. label Feb 20, 2024
@github-actions github-actions bot added the windows Issues and PRs related to the Windows platform. label Feb 20, 2024
@mhdawson mhdawson mentioned this issue Feb 20, 2024
Merged
@legendecas legendecas mentioned this issue Feb 21, 2024
Merged
@joyeecheung
Copy link
Member

This has failed 14 PRs out of the last 100 CI runs.

From a glance the test it's hard to tell what it is trying to do. It was added by dceebbf#diff-53a1141f9d431cc7d603359ba604d660819f5560b68895fb639dc988a34644c8 so I suppose the original intention was just to test that the socket can be sent over to the child. The messages based on connection order seems to be very brittle, I don't think we guarantee that if you fork multiple child processes, they are going to be ready to connect to the parent in the exact same order that you start them. Perhaps the test can be split so that each child process gets its own test. But that's just a guess, I am not 100% that's not breaking what the test was trying to check.

@joyeecheung
Copy link
Member

joyeecheung commented Feb 23, 2024 •
edited
Loading

Actually I have a different theory. See #51841

@joyeecheung joyeecheung mentioned this issue Feb 23, 2024
Closed
joyeecheung added a commit that referenced this issue Feb 25, 2024
@joyeecheung
test: fix test-child-process-fork-net
ec7a268 
The child processes are supposed to get 4 messages (2 ends, 2 writes).
Previously the mustCall() wrapping the message listener attempts
to match exactly 1 invocation which is bound to fail but could be
swallowed if the child happens to be killed before the exit event
is fired for the mustCall() check to work. In the CI, on some
machines the kill() could happen after the child process finishes
with the mustCall() check, resulting in EPERM errors in kill().

This patch fixes the mustCall() checks (updating the expected
invocation count to 4) and swallow the errors when kill() fails,
which should be fine because they are only there for cleanup.

PR-URL: #51841
Refs: #51813
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
marco-ippolito pushed a commit that referenced this issue Feb 26, 2024
@joyeecheung @marco-ippolito
test: fix test-child-process-fork-net
cfa9e21 
The child processes are supposed to get 4 messages (2 ends, 2 writes).
Previously the mustCall() wrapping the message listener attempts
to match exactly 1 invocation which is bound to fail but could be
swallowed if the child happens to be killed before the exit event
is fired for the mustCall() check to work. In the CI, on some
machines the kill() could happen after the child process finishes
with the mustCall() check, resulting in EPERM errors in kill().

This patch fixes the mustCall() checks (updating the expected
invocation count to 4) and swallow the errors when kill() fails,
which should be fine because they are only there for cleanup.

PR-URL: #51841
Refs: #51813
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
marco-ippolito pushed a commit that referenced this issue Feb 26, 2024
@joyeecheung @marco-ippolito
test: fix test-child-process-fork-net
01d4f57 
The child processes are supposed to get 4 messages (2 ends, 2 writes).
Previously the mustCall() wrapping the message listener attempts
to match exactly 1 invocation which is bound to fail but could be
swallowed if the child happens to be killed before the exit event
is fired for the mustCall() check to work. In the CI, on some
machines the kill() could happen after the child process finishes
with the mustCall() check, resulting in EPERM errors in kill().

This patch fixes the mustCall() checks (updating the expected
invocation count to 4) and swallow the errors when kill() fails,
which should be fine because they are only there for cleanup.

PR-URL: #51841
Refs: #51813
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
marco-ippolito pushed a commit that referenced this issue Feb 27, 2024
@joyeecheung @marco-ippolito
test: fix test-child-process-fork-net
1f6551d 
The child processes are supposed to get 4 messages (2 ends, 2 writes).
Previously the mustCall() wrapping the message listener attempts
to match exactly 1 invocation which is bound to fail but could be
swallowed if the child happens to be killed before the exit event
is fired for the mustCall() check to work. In the CI, on some
machines the kill() could happen after the child process finishes
with the mustCall() check, resulting in EPERM errors in kill().

This patch fixes the mustCall() checks (updating the expected
invocation count to 4) and swallow the errors when kill() fails,
which should be fine because they are only there for cleanup.

PR-URL: #51841
Refs: #51813
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
richardlau pushed a commit that referenced this issue Mar 25, 2024
@joyeecheung @richardlau
test: fix test-child-process-fork-net
a992b25 
The child processes are supposed to get 4 messages (2 ends, 2 writes).
Previously the mustCall() wrapping the message listener attempts
to match exactly 1 invocation which is bound to fail but could be
swallowed if the child happens to be killed before the exit event
is fired for the mustCall() check to work. In the CI, on some
machines the kill() could happen after the child process finishes
with the mustCall() check, resulting in EPERM errors in kill().

This patch fixes the mustCall() checks (updating the expected
invocation count to 4) and swallow the errors when kill() fails,
which should be fine because they are only there for cleanup.

PR-URL: #51841
Refs: #51813
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
richardlau pushed a commit that referenced this issue Mar 25, 2024
@joyeecheung @richardlau
test: fix test-child-process-fork-net
963d7d7 
The child processes are supposed to get 4 messages (2 ends, 2 writes).
Previously the mustCall() wrapping the message listener attempts
to match exactly 1 invocation which is bound to fail but could be
swallowed if the child happens to be killed before the exit event
is fired for the mustCall() check to work. In the CI, on some
machines the kill() could happen after the child process finishes
with the mustCall() check, resulting in EPERM errors in kill().

This patch fixes the mustCall() checks (updating the expected
invocation count to 4) and swallow the errors when kill() fails,
which should be fine because they are only there for cleanup.

PR-URL: #51841
Refs: #51813
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
rdw-msft pushed a commit to rdw-msft/node that referenced this issue Mar 26, 2024
@joyeecheung @rdw-msft
test: fix test-child-process-fork-net
4be5687 
The child processes are supposed to get 4 messages (2 ends, 2 writes).
Previously the mustCall() wrapping the message listener attempts
to match exactly 1 invocation which is bound to fail but could be
swallowed if the child happens to be killed before the exit event
is fired for the mustCall() check to work. In the CI, on some
machines the kill() could happen after the child process finishes
with the mustCall() check, resulting in EPERM errors in kill().

This patch fixes the mustCall() checks (updating the expected
invocation count to 4) and swallow the errors when kill() fails,
which should be fine because they are only there for cleanup.

PR-URL: nodejs#51841
Refs: nodejs#51813
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@lpinca
Copy link
Member

lpinca commented Sep 7, 2024

Fixed in #51841.

@lpinca lpinca closed this as completed Sep 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
flaky-test Issues and PRs related to the tests with unstable failures on the CI. windows Issues and PRs related to the Windows platform.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@lpinca @joyeecheung @mhdawson