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

Node 20 fixes #4473

Merged
merged 3 commits into from
Jan 29, 2024
Merged

Node 20 fixes #4473

merged 3 commits into from
Jan 29, 2024

Conversation

kanongil
Copy link
Contributor

This fixes the test suite for node v18, v20 and v21 on my mac.

Wreck.post() fails with "socket hang up"
This includes a fix to immediately destroy incoming requests when in the stopping phase.

Node 20+ implicitly does this, so a coverage exception is needed.
@kanongil kanongil added the test Test or coverage label Nov 15, 2023
@kanongil
Copy link
Contributor Author

Hmm, strange that the macOS runners fail, when they work locally (on M1 macOS 14.1.1).

Looking at the failing tests, it seems that request.active() never returns false.
The "Request active() exits handler early when request is no longer active" test case seems central.

The "Request _postCycle() skips onPreResponse when validation terminates request" test does not directly use request.active() but it might still rely on the internal version of this.

I don't have a macOS 12 machine to test on, so I'm not really able to debug further.

Even stranger, the tests passes on node 18, but fails again on node 20.

@Nargonath
Copy link
Member

Looks like we have some flakiness at play here. I restarted the 3 failed macos jobs and now macos node@14 passes whereas it didn't before. This might explain why it works on your machine @kanongil but not in the CI.

Copy link
Member

@Nargonath Nargonath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code LGTM, I had some questions for my own curiosity.

lib/transmit.js Show resolved Hide resolved
test/transmit.js Show resolved Hide resolved
@kanongil
Copy link
Contributor Author

Looks like we have some flakiness at play here. I restarted the 3 failed macos jobs and now macos node@14 passes whereas it didn't before. This might explain why it works on your machine @kanongil but not in the CI.

Ah, it seems that it only fails on runners using macOS 12.6.9, while runners on macOS 12.7 works. I guess not all macos-latest runners are identical…

I have been doing some experiments, and the tests seem to pass when using the macos-11 or macos-13 (beta) runners, so that might be a way to fix this for now.

@Nargonath
Copy link
Member

Thanks for the investigation on the CI. We probably shouldn't block this PR for this and can handle it in another PR.

@Marsup Marsup added this to the 21.3.3 milestone Jan 29, 2024
@Marsup Marsup merged commit d59bdc6 into hapijs:master Jan 29, 2024
7 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Test or coverage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants