-
Notifications
You must be signed in to change notification settings - Fork 30.4k
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.
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
test: fix fork-dgram on AIX #8549
Conversation
Failure on AIX (before)
Passing on AIX (after)
|
Not too sure. If the intention of the test was that messages reach both the parent and the child while both are listening, then probably the fix is not correct. If not, probably it is. |
@santigimeno Right, I agree. The strange thing is that I haven't managed to reproduce the issue I'm seeing on any other machines (except the community AIX ones). The test runs fine on all our AIX boxes. Looking at nodejs/node-v0.x-archive#8045 (comment), it seems that this change shouldn't make any difference. |
Agreed. |
Testing that UDP messages are distributed among all processes from a given set of processes whose members do not change over time and that share a UDP socket seems inherently brittle. We might be able to decrease the likeliness of such failures by tweaking various settings (number of messages, number of child processes, etc.) but it seems the results will always depend on the underlying platform. Changing the state (preventing them from being able to receive messages) of the processes as soon as they receive one message like this PR does changes the nature of this test, so it would not be acceptable if we wanted the test to keep its original purpose. I see two possible ways forward:
I think I have a preference for 2), and it seems in line with the current caveats of using the cluster module without using the round robin scheduler, where one cannot rely on the load being distributed evenly (or in any way) among worker processes. |
What I'm not sure about is whether we can expect the load to be at all distributed between the two processes. I'd suggest that there are two possibilities (mirroring those in #8549 (comment)):
@AndreasMadsen I wasn't sure from #8549 (comment) whether we can expect messages to be distributed to both child and parent. Could you clarify? I noticed that you said in nodejs/node-v0.x-archive#8045 (comment) that:
I also had a look at what
|
We can definitely expect that, that is what is being tested. But I guess it could be platform dependent.
I wouldn't trust the documentation for how cluster UDP acts, since it's a rarely used feature. To be honest I don't remember much, I worked with this stuff 4 years ago, but haven't since. Sorry :/ |
Somehow I missed this PR so I created #8271. Looking at the discussion here I think it does option 2) as suggested by @misterdjules which would also be my preference. It also fixes the issue of the test leaving processes hanging around. |
@gibfahn I don't think "which means that the parent isn't properly connecting sometimes, " as which my version we do see both getting at least one message. |
In terms of: "server.close() does, and (assuming I'm looking in the right place), server.close() will close the socket directly." I think that after a fork each process has its own handle to the share socket and can close their handle independently which would explain the behavior seen. |
Closing in favour of #8697 which does the same thing but closes the parent on received message as well (and stops processes being left around if the test fails). |
Checklist
make -j4 test
(UNIX), orvcbuild test nosign
(Windows) passesAffected core subsystem(s)
test
Description of change
Closes the server as soon as the child receives a message
I'm not sure that this is the correct fix, but I've tried this with some debugging information (see this gist) and both parent and child do seem to be receiving a message on AIX.
Fixes: #8271
Refs:
Previous PR (to fix on SmartOS): nodejs/node-v0.x-archive#8045
Previous Issue (ditto): nodejs/node-v0.x-archive#8046
Original Commit: bdf7ac2
Original PR: nodejs/node-v0.x-archive#4864