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.

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 test-cluster-worker-exit.js for AIX #3666

Closed
wants to merge 1 commit into from
Closed

test: Fix test-cluster-worker-exit.js for AIX #3666

wants to merge 1 commit into from

Conversation

imran-iq
Copy link
Contributor

@imran-iq imran-iq commented Nov 4, 2015

test fails intermittently due to the assertion that the 'disconnect'
event should come before the 'exit' event. This is caused be the
non-deteministic behaviour of pollset_poll[1] on AIX
(see deps/uv/src/unix/aix.c). This API makes no garauntee for the order
in which file descriptors are returned. On linux epoll_wait[2] is used,
which also does not make a garauntee on order of file descriptors
returned. In the failing case we recieve our file descriptor with a
callback of uv__signal_event (which causes JavaScript to receive the
exit event) before our file descriptor with uv__stream_io as its
callback (which in turn causes JavaScript receive the disconnect event).
This change simply removes the assertion that the disconnect event
happens before exit event and processes the test regardless of which
event comes first.

[1] https://www-01.ibm.com/support/knowledgecenter/ssw_aix_71/com.ibm.aix.basetrf1/pollset.htm
[2] http://linux.die.net/man/2/epoll_pwait

test fails intermittently due to the assertion that the 'disconnect'
event should come before the 'exit' event. This is caused be the
non-deteministic behaviour of pollset_poll[1] on AIX
(see deps/uv/src/unix/aix.c). This API makes no garauntee for the order
in which file descriptors are returned. On linux epoll_wait[2] is used,
which also does not make a garauntee on order of file descriptors
returned. In the failing case we recieve our file descriptor with a
callback of uv__signal_event (which causes JavaScript to receive the
exit event) before our file descriptor with uv__stream_io as its
callback (which in turn causes JavaScript receive the disconnect event).
This change simply removes the assertion that the disconnect event
happens before exit event and processes the test regardless of which
event comes first.

[1] https://www-01.ibm.com/support/knowledgecenter/ssw_aix_71/com.ibm.ai
x.basetrf1/pollset.htm
[2] http://linux.die.net/man/2/epoll_pwait
@jasnell
Copy link
Member

jasnell commented Nov 4, 2015

LGTM

@mhdawson
Copy link
Member

mhdawson commented Nov 4, 2015

This fix is appropriate provided everybody agrees that there is no guarantee in respect how the events should occur. If that is not the case, a change to Node would be required to provide the expected behavior.

@bnoordhuis can you chime in.

@jasnell
Copy link
Member

jasnell commented Nov 4, 2015

Ah yes, good point. My assumption is that would be a semver-major change

@mscdex mscdex added cluster Issues and PRs related to the cluster subsystem. test Issues and PRs related to the tests. labels Nov 5, 2015
@jasnell
Copy link
Member

jasnell commented Nov 5, 2015

@nodejs/ctc

@imran-iq
Copy link
Contributor Author

imran-iq commented Nov 5, 2015

From https://nodejs.org/api/cluster.html#cluster_cluster_workers

"A worker is removed from cluster.workers after the worker has disconnected and exited. The order between these two events cannot be determined in advance."

I believe this to be a faulty test by design. There should be no reason why we should assert that a 'disconnect' happens before an 'exit' event

@jasnell
Copy link
Member

jasnell commented Nov 13, 2015

@trevnorris @mscdex @chrisdickinson ... any thoughts on this one?

@cjihrig
Copy link
Contributor

cjihrig commented Nov 13, 2015

Makes sense. If the CI is happy, LGTM

@jasnell
Copy link
Member

jasnell commented Nov 14, 2015

CI appears to be terminally unhappy these days. Hopefully it'll be kicked back into shape soon. Looking through the code, I agree with @iwuzhere's interpretation. I'll go ahead and land. We can revisit if necessary if CI issues pop up once the CI is stable again.

jasnell pushed a commit that referenced this pull request Nov 14, 2015
test fails intermittently due to the assertion that the 'disconnect'
event should come before the 'exit' event. This is caused be the
non-deteministic behaviour of pollset_poll[1] on AIX
(see deps/uv/src/unix/aix.c). This API makes no garauntee for the order
in which file descriptors are returned. On linux epoll_wait[2] is used,
which also does not make a garauntee on order of file descriptors
returned. In the failing case we recieve our file descriptor with a
callback of uv__signal_event (which causes JavaScript to receive the
exit event) before our file descriptor with uv__stream_io as its
callback (which in turn causes JavaScript receive the disconnect event).
This change simply removes the assertion that the disconnect event
happens before exit event and processes the test regardless of which
event comes first.

[1] https://www-01.ibm.com/support/knowledgecenter/ssw_aix_71/com.ibm.ai
x.basetrf1/pollset.htm
[2] http://linux.die.net/man/2/epoll_pwait

PR-URL: #3666
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Nov 14, 2015

Landed in 8b57b31

@MylesBorins
Copy link
Contributor

landed in v4.x-staging in 0fb40b4

MylesBorins pushed a commit that referenced this pull request Nov 17, 2015
test fails intermittently due to the assertion that the 'disconnect'
event should come before the 'exit' event. This is caused be the
non-deteministic behaviour of pollset_poll[1] on AIX
(see deps/uv/src/unix/aix.c). This API makes no garauntee for the order
in which file descriptors are returned. On linux epoll_wait[2] is used,
which also does not make a garauntee on order of file descriptors
returned. In the failing case we recieve our file descriptor with a
callback of uv__signal_event (which causes JavaScript to receive the
exit event) before our file descriptor with uv__stream_io as its
callback (which in turn causes JavaScript receive the disconnect event).
This change simply removes the assertion that the disconnect event
happens before exit event and processes the test regardless of which
event comes first.

[1] https://www-01.ibm.com/support/knowledgecenter/ssw_aix_71/com.ibm.ai
x.basetrf1/pollset.htm
[2] http://linux.die.net/man/2/epoll_pwait

PR-URL: #3666
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Nov 17, 2015
test fails intermittently due to the assertion that the 'disconnect'
event should come before the 'exit' event. This is caused be the
non-deteministic behaviour of pollset_poll[1] on AIX
(see deps/uv/src/unix/aix.c). This API makes no garauntee for the order
in which file descriptors are returned. On linux epoll_wait[2] is used,
which also does not make a garauntee on order of file descriptors
returned. In the failing case we recieve our file descriptor with a
callback of uv__signal_event (which causes JavaScript to receive the
exit event) before our file descriptor with uv__stream_io as its
callback (which in turn causes JavaScript receive the disconnect event).
This change simply removes the assertion that the disconnect event
happens before exit event and processes the test regardless of which
event comes first.

[1] https://www-01.ibm.com/support/knowledgecenter/ssw_aix_71/com.ibm.ai
x.basetrf1/pollset.htm
[2] http://linux.die.net/man/2/epoll_pwait

PR-URL: #3666
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
rvagg pushed a commit that referenced this pull request Dec 4, 2015
test fails intermittently due to the assertion that the 'disconnect'
event should come before the 'exit' event. This is caused be the
non-deteministic behaviour of pollset_poll[1] on AIX
(see deps/uv/src/unix/aix.c). This API makes no garauntee for the order
in which file descriptors are returned. On linux epoll_wait[2] is used,
which also does not make a garauntee on order of file descriptors
returned. In the failing case we recieve our file descriptor with a
callback of uv__signal_event (which causes JavaScript to receive the
exit event) before our file descriptor with uv__stream_io as its
callback (which in turn causes JavaScript receive the disconnect event).
This change simply removes the assertion that the disconnect event
happens before exit event and processes the test regardless of which
event comes first.

[1] https://www-01.ibm.com/support/knowledgecenter/ssw_aix_71/com.ibm.ai
x.basetrf1/pollset.htm
[2] http://linux.die.net/man/2/epoll_pwait

PR-URL: #3666
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell jasnell mentioned this pull request Dec 17, 2015
jasnell pushed a commit that referenced this pull request Dec 17, 2015
test fails intermittently due to the assertion that the 'disconnect'
event should come before the 'exit' event. This is caused be the
non-deteministic behaviour of pollset_poll[1] on AIX
(see deps/uv/src/unix/aix.c). This API makes no garauntee for the order
in which file descriptors are returned. On linux epoll_wait[2] is used,
which also does not make a garauntee on order of file descriptors
returned. In the failing case we recieve our file descriptor with a
callback of uv__signal_event (which causes JavaScript to receive the
exit event) before our file descriptor with uv__stream_io as its
callback (which in turn causes JavaScript receive the disconnect event).
This change simply removes the assertion that the disconnect event
happens before exit event and processes the test regardless of which
event comes first.

[1] https://www-01.ibm.com/support/knowledgecenter/ssw_aix_71/com.ibm.ai
x.basetrf1/pollset.htm
[2] http://linux.die.net/man/2/epoll_pwait

PR-URL: #3666
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit that referenced this pull request Dec 23, 2015
test fails intermittently due to the assertion that the 'disconnect'
event should come before the 'exit' event. This is caused be the
non-deteministic behaviour of pollset_poll[1] on AIX
(see deps/uv/src/unix/aix.c). This API makes no garauntee for the order
in which file descriptors are returned. On linux epoll_wait[2] is used,
which also does not make a garauntee on order of file descriptors
returned. In the failing case we recieve our file descriptor with a
callback of uv__signal_event (which causes JavaScript to receive the
exit event) before our file descriptor with uv__stream_io as its
callback (which in turn causes JavaScript receive the disconnect event).
This change simply removes the assertion that the disconnect event
happens before exit event and processes the test regardless of which
event comes first.

[1] https://www-01.ibm.com/support/knowledgecenter/ssw_aix_71/com.ibm.ai
x.basetrf1/pollset.htm
[2] http://linux.die.net/man/2/epoll_pwait

PR-URL: #3666
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jBarz added a commit to ibmruntimes/node that referenced this pull request Feb 19, 2017
cherry-pick following from master

Author: Imran Iqbal <[email protected]>
Date:   Wed Nov 4 17:08:07 2015 -0500

    test: Fix test-cluster-worker-exit.js for AIX

    test fails intermittently due to the assertion that the 'disconnect'
    event should come before the 'exit' event. This is caused be the
    non-deteministic behaviour of pollset_poll[1] on AIX
    (see deps/uv/src/unix/aix.c). This API makes no garauntee for the
order
    in which file descriptors are returned. On linux epoll_wait[2] is
used,
    which also does not make a garauntee on order of file descriptors
    returned. In the failing case we recieve our file descriptor with a
    callback of uv__signal_event (which causes JavaScript to receive the
    exit event) before our file descriptor with uv__stream_io as its
    callback (which in turn causes JavaScript receive the disconnect
event).
    This change simply removes the assertion that the disconnect event
    happens before exit event and processes the test regardless of which
    event comes first.

    [1]
https://www-01.ibm.com/support/knowledgecenter/ssw_aix_71/com.ibm.ai
    x.basetrf1/pollset.htm
    [2] http://linux.die.net/man/2/epoll_pwait

    PR-URL: nodejs/node#3666
    Reviewed-By: Colin Ihrig <[email protected]>
    Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cluster Issues and PRs related to the cluster subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants