-
Notifications
You must be signed in to change notification settings - Fork 30k
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-net-bytes-per-incoming-chunk-overhead
failing on Fedora 38
#48490
Comments
@ShogunPanda I've applied the fix on top of |
I see. If you disable network family autoselection does it work? |
It still doesn't work with |
Ok, I see. At least I know it's unrelated to my changes. |
This also failed on macOS 11. See https://ci.nodejs.org/job/node-test-pull-request/52510/.
|
Not the same error, though. |
cc: @nodejs/libuv |
Not a libuv issue. The test makes a shaky assumption about RSS: node/test/pummel/test-net-bytes-per-incoming-chunk-overhead.js Lines 45 to 49 in 951da52
|
@bnoordhuis I pinged libuv maintainers for this failed assertion. I think it is not related to the test but I guess it shouldn't happen.
|
Ah, right. That should have been fixed by #48078 but I guess not? cc @trevnorris |
Want to confirm, does this still happen with the recent update to libuv v1.46.0? And does it happen consistently? |
@trevnorris The last time seems to be Jun 27, 2023 as per https://ci.nodejs.org/job/node-test-commit-osx/52949/. I'm not sure if libuv v1.46.0 was already merged. |
It seems libuv@1.46.0 landed on main on Jul 3, 2023 so it did not happen again. |
@lpinca I'm not convinced it's been fully fixed. Did that crash only happen with |
Match the implementation for linux.c to kqueue.c in the code around the calls to kevent and epoll. In linux.c the code was made more DRY by moving the nfds check up (including a comment of why it's possible) and combining two if checks into one. In kqueue.c the EINTR check was moved to match linux.c and an assert that was added in 42cc412 has been moved to be called directly after the EINTR check. Since it should always be true regardless. Ref: libuv#3893 Ref: nodejs/node#48490
I don't know, I only saw it with that test. |
@lpinca do you know the version of |
Match the implementation for linux.c to kqueue.c in the code around the calls to kevent and epoll. In linux.c the code was made more DRY by moving the nfds check up (including a comment of why it's possible) and combining two if checks into one. In kqueue.c the EINTR check was moved to match linux.c and an assert that was added in 42cc412 has been moved to be called directly after the EINTR check. Since it should always be true regardless. Ref: libuv#3893 Ref: nodejs/node#48490
Match the implementation for linux.c to kqueue.c in the code around the calls to kevent and epoll. In linux.c the code was made more DRY by moving the nfds check up (including a comment of why it's possible) and combining two if checks into one. In kqueue.c the EINTR check was moved to match linux.c and an assert that was added in 42cc412 has been moved to be called directly after the EINTR check. Since it should always be true regardless. Ref: libuv#3893 Ref: nodejs/node#48490
Match the implementation for linux.c to kqueue.c in the code around the calls to kevent and epoll. In linux.c the code was made more DRY by moving the nfds check up (including a comment of why it's possible) and combining two if checks into one. In kqueue.c the assert to check the timeout when nfds == 0 has been moved to be called directly after the EINTR check. Since it should always be true regardless. Ref: libuv#3893 Ref: nodejs/node#48490
Match the implementation for linux.c to kqueue.c in the code around the calls to kevent and epoll. In linux.c the code was made more DRY by moving the nfds check up (including a comment of why it's possible) and combining two if checks into one. In kqueue.c the assert to check the timeout when nfds == 0 has been moved to be called directly after the EINTR check. Since it should always be true regardless. Ref: libuv#3893 Ref: nodejs/node#48490
Match the implementation for linux.c to kqueue.c in the code around the calls to kevent and epoll. In linux.c the code was made more DRY by moving the nfds check up (including a comment of why it's possible) and combining two if checks into one. In kqueue.c the assert to check the timeout when nfds == 0 has been moved to be called directly after the EINTR check. Since it should always be true regardless. Ref: libuv#3893 Ref: nodejs/node#48490
@bnoordhuis Would you agree to just delete this test? I don't know what else to do with it. |
Yes. Bad assumptions are bad. |
The test's assumptions about RSS are no longer valid, at least with Fedora 38. Closes: nodejs#48490
Match the implementation for linux.c to kqueue.c in the code around the calls to kevent and epoll. In linux.c the code was made more DRY by moving the nfds check up (including a comment of why it's possible) and combining two if checks into one. In kqueue.c the assert to check the timeout when nfds == 0 has been moved to be called directly after the EINTR check. Since it should always be true regardless. Ref: libuv#3893 Ref: nodejs/node#48490
Match the implementation for linux.c to kqueue.c in the code around the calls to kevent and epoll. In linux.c the code was made more DRY by moving the nfds check up (including a comment of why it's possible) and combining two if checks into one. In kqueue.c the assert to check the timeout when nfds == 0 has been moved to be called directly after the EINTR check. Since it should always be true regardless. Ref: libuv#3893 Ref: nodejs/node#48490
The test's assumptions about RSS are no longer valid, at least with Fedora 38. Closes: nodejs#48490 PR-URL: nodejs#48811 Fixes: nodejs#48490 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>
The test's assumptions about RSS are no longer valid, at least with Fedora 38. Closes: nodejs#48490 PR-URL: nodejs#48811 Fixes: nodejs#48490 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>
The test's assumptions about RSS are no longer valid, at least with Fedora 38. Closes: nodejs#48490 PR-URL: nodejs#48811 Fixes: nodejs#48490 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>
The test's assumptions about RSS are no longer valid, at least with Fedora 38. Closes: nodejs#48490 PR-URL: nodejs#48811 Fixes: nodejs#48490 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>
The test's assumptions about RSS are no longer valid, at least with Fedora 38. Closes: nodejs#48490 PR-URL: nodejs#48811 Fixes: nodejs#48490 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>
Refs: nodejs/build#3350 (comment)
@nodejs/net
The text was updated successfully, but these errors were encountered: