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

Run apollo-server-hapi versioned tests on Node 16 #81

Closed
bizob2828 opened this issue May 19, 2021 · 3 comments · Fixed by #115
Closed

Run apollo-server-hapi versioned tests on Node 16 #81

bizob2828 opened this issue May 19, 2021 · 3 comments · Fixed by #115

Comments

@bizob2828
Copy link
Member

Feature Description

During triage of #75 it turned out there was a bug with Node.js v16. In order to allow the PR to be merged, the tests for apollo-server-hapi were pinned to exclude v16: fix here. When Node.js v16 becomes LTS, the tests on should be revisited to see if the fix has been made.

Additional context

The issue surfaced in Node.js with this PR nodejs/node#33035. The effect for apollo-server-hapi versioned tests are they just hang: See affected CI run. This is because the readable stream(http request), prematurely is destroyed. hapi has a listener for this, handle close, which basically just stops processing the request. You can reproduce the issue with this apollo hapi app and make this curl request:

curl --location --request POST 'http://localhost:6000/graphql' \
--header 'Content-Type: application/json' \
--data-raw '{"query":"query GetRoids {\n  asteroids(date: \"2020-08-12\") {\n    id,\n    name,\n    miss_distance_km,\n    close_approach_date,\n    relative_velocity_km_per_hour,\n  }\n}","variables":{}}'

The code in Node.js has been reverted: revert PR, but was only made to v15. There is still an open issue in Node.js for fixing this nodejs/node#38657, which I assume will happen before v16 becomes LTS.

Priority

[Must Have]

@bizob2828 bizob2828 added this to Triage Needed: Unprioritized Features in Node.js Engineering Board via automation May 19, 2021
@bizob2828 bizob2828 moved this from Triage Needed: Unprioritized Features to To do: Features here are prioritized in Node.js Engineering Board May 24, 2021
@bizob2828
Copy link
Member Author

apollo-server fixed this but the release hasn't been shipped yet. This PR adds Node 16 support for apollo-server-hapi apollographql/apollo-server#5150. We will have to lock down the versioned tests regardless to account for this. It's worth noting we will be possibly dropping support for hapi < 20 for Node 16 for the agent newrelic/node-newrelic#771 but this is a separate plugin and worth a discussion on how to support this plugin in node 16

@michaelgoin
Copy link
Member

While we have other issues with v3, it looked like this fix had been in v3 so we can potentially tackle this for v3+.

@michaelgoin michaelgoin added this to To Do in 1.0.0 - Naming and Node 16 / Drop 10 via automation Jul 9, 2021
@michaelgoin
Copy link
Member

Likely requires this to be resolved first: #103

@bizob2828 bizob2828 moved this from To Do to In Progress in 1.0.0 - Naming and Node 16 / Drop 10 Jul 13, 2021
@bizob2828 bizob2828 self-assigned this Jul 13, 2021
@bizob2828 bizob2828 moved this from To do: Features here are prioritized to In progress: Issues being worked on in Node.js Engineering Board Jul 14, 2021
@bizob2828 bizob2828 moved this from In progress: Issues being worked on to Needs PR Review in Node.js Engineering Board Jul 14, 2021
@bizob2828 bizob2828 moved this from In Progress to Needs PR Review in 1.0.0 - Naming and Node 16 / Drop 10 Jul 14, 2021
Node.js Engineering Board automation moved this from Needs PR Review to Done: Issues recently completed Jul 14, 2021
1.0.0 - Naming and Node 16 / Drop 10 automation moved this from Needs PR Review to Done Jul 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Node.js Engineering Board
  
Done: Issues recently completed
Development

Successfully merging a pull request may close this issue.

3 participants