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

lookup: add hapi #436

Merged
merged 1 commit into from
Mar 16, 2021
Merged

lookup: add hapi #436

merged 1 commit into from
Mar 16, 2021

Conversation

gibfahn
Copy link
Member

@gibfahn gibfahn commented Jun 2, 2017

Once this is green we can land it (needs a CI run first).

Checklist
  • contribution guidelines followed here

@gibfahn
Copy link
Member Author

gibfahn commented Jun 2, 2017

Hard Requirements

  • Module source code must be on Github.
  • Published versions must include a tag on Github
  • The test process must be executable with only the commands npm install && npm test using the tarball downloaded from the Github tag mentioned above
  • The tests pass on supported major release lines
  • The maintainers of the module remain responsive when there are problems
  • At least one module maintainer must be added to the lookup maintainers field

Soft Requirements

At least one of:

  • The module must be actively used by the community
  • The module fits into a key category (e.g. Testing, Streams, Monitoring, etc.)

@gibfahn
Copy link
Member Author

gibfahn commented Jun 2, 2017

@gibfahn gibfahn force-pushed the hapi branch 3 times, most recently from 82de876 to 6fbb359 Compare June 3, 2017 20:07
@DavidTPate
Copy link

I see that there's a few builds failing on Node 8 which look to be related to the fixes that we have pending in hapijs/hapi#3507 around steams which once those changes land clear up the 3rd failing test for Node 8. Specifically the test 780) transmission transmit() does not leak stream data when request aborts before stream is returned.

The other two for Node 8 I'm able to reproduce successfully on my machine (Fedora 24) and I'll dig into them further.

As for the failures that we are seeing with some Node 6 tests I'm not able to reproduce them on either of my machines (Fedora 24 and Windows 10). Those will need to be investigated further to see what's amiss.

gdams

This comment was marked as off-topic.

@gibfahn
Copy link
Member Author

gibfahn commented Jul 18, 2017

Note that this shouldn't be landed until we have green CI runs.

@gibfahn gibfahn changed the title lookup: add hapi WIP - lookup: add hapi Jul 18, 2017
@hueniverse
Copy link

Note that master had some issues with a recent change that worked on node 8 but broke on 4 and 6. Basically, when calling res.destroy() in node 8, the stream finish event is fired. Not the case in node 4 and 6 when nothing fires. It is now fixed, but I am pointing it out in case this wasn't expected...

@mcollina
Copy link
Member

@hueniverse what's the tracking issue of that? I would like to take a look. There was a bug that was fixed in Node 8.1.3 (nodejs/node#13850), and there is another one in zlib that I am going to land this week if no one object (nodejs/node#14330).

@hueniverse
Copy link

@mcollina hapijs/hapi#2121

targos

This comment was marked as off-topic.

@targos
Copy link
Member

targos commented Aug 10, 2017

Tests are failing on some machines. It seems that lsof is not installed everywhere.

 Test script errors:
 Multiple callbacks or thrown errors received in test "Connection ext() onPreResponse cleans unused file stream when response is overridden" (done)
       at Immediate.immediate._onImmediate (timers.js:453:18)
       at processImmediate [as _immediateCallback] (timers.js:396:17)
 Multiple callbacks or thrown errors received in test "transmission marshal() closes file handlers when not reading file stream" (done)
       at Immediate.immediate._onImmediate (timers.js:453:18)
       at processImmediate [as _immediateCallback] (timers.js:396:17)
 Multiple callbacks or thrown errors received in test "transmission marshal() closes file handlers when not using a manually open file stream" (done)
       at Immediate.immediate._onImmediate (timers.js:453:18)
       at processImmediate [as _immediateCallback] (timers.js:396:17)
 There were 3 test script error(s).
 Failed tests:
   145) Connection ext() onPreResponse cleans unused file stream when response is overridden:
       spawn lsof ENOENT
       at exports._errnoException (util.js:907:11)
       at Process.ChildProcess._handle.onexit (internal/child_process.js:189:32)
       at onErrorNT (internal/child_process.js:363:16)
       at nextTickCallbackWith2Args (node.js:511:9)
       at process._tickDomainCallback (node.js:466:17)
   735) transmission marshal() closes file handlers when not reading file stream:
       spawn lsof ENOENT
       at exports._errnoException (util.js:907:11)
       at Process.ChildProcess._handle.onexit (internal/child_process.js:189:32)
       at onErrorNT (internal/child_process.js:363:16)
       at nextTickCallbackWith2Args (node.js:511:9)
       at process._tickDomainCallback (node.js:466:17)
   736) transmission marshal() closes file handlers when not using a manually open file stream:
       spawn lsof ENOENT
       at exports._errnoException (util.js:907:11)
       at Process.ChildProcess._handle.onexit (internal/child_process.js:189:32)
       at onErrorNT (internal/child_process.js:363:16)
       at nextTickCallbackWith2Args (node.js:511:9)
       at process._tickDomainCallback (node.js:466:17)
 3 of 916 tests failed

@gibfahn
Copy link
Member Author

gibfahn commented Aug 10, 2017

Do the hapi tests require lsof?

@cjihrig
Copy link

cjihrig commented Aug 10, 2017

Currently, yes. It looks like a few of them spawn it.

@MylesBorins
Copy link
Contributor

@nodejs/build are we able to get lsof on host machines?

@BridgeAR
Copy link
Member

What is the status here?

@gibfahn
Copy link
Member Author

gibfahn commented Feb 18, 2018

@nodejs/build are we able to get lsof on host machines?
What is the status here?

Looks like it needs an issue opening in the build repo to see if we can get lsof added.

@cjihrig
Copy link

cjihrig commented Feb 19, 2018

I've also opened a PR against hapi to allow the tests to pass when lsof does not exist.

hapijs/hapi#3744

@apapirovski
Copy link
Member

It seems like this could maybe land given that @cjihrig's patch is in hapi now? Or is there anything else outstanding? Would be nice to have hapi in CitGM.

@BridgeAR
Copy link
Member

@targos would you be so kind and start a new CITGM? Seems like it is now outdated.

@BridgeAR
Copy link
Member

And would someone be so kind and change the title to remove the WIP?

@al-k21 al-k21 changed the title WIP - lookup: add hapi lookup: add hapi May 21, 2018
@al-k21
Copy link
Contributor

al-k21 commented May 21, 2018

@targos
Copy link
Member

targos commented Jan 4, 2021

@targos
Copy link
Member

targos commented Jan 4, 2021

There's still the issue of tests depending on the presence of lsof:

   175) Core ext() onPreResponse cleans unused file stream when response is overridden:
       spawn lsof ENOENT
       at Process.ChildProcess._handle.onexit (internal/child_process.js:269:19)
       at onErrorNT (internal/child_process.js:465:16)
       at processTicksAndRejections (internal/process/task_queues.js:80:21)
   805) transmission marshal() closes file handlers when not reading file stream:
       spawn lsof ENOENT
       at Process.ChildProcess._handle.onexit (internal/child_process.js:269:19)
       at onErrorNT (internal/child_process.js:465:16)
       at processTicksAndRejections (internal/process/task_queues.js:80:21)
   806) transmission marshal() closes file handlers when not using a manually open file stream:
       spawn lsof ENOENT
       at Process.ChildProcess._handle.onexit (internal/child_process.js:269:19)
       at onErrorNT (internal/child_process.js:465:16)
       at processTicksAndRejections (internal/process/task_queues.js:80:21)

@AdriVanHoudt
Copy link

AdriVanHoudt commented Jan 7, 2021

I'm happy to make sure hapi gets into citgm and making sure that future failures get resolved/reported to hapi.

As for the latest errors.
There has been fixes specifically for this in the past hapijs/hapi#3744.
I'm not familiar with ChildProcess.spawn or how that error looks, it might be that the ignore is not correct?

@devinivy it might be good to decide on how to relay breakage, do I just ping (Slack or here) or open up an issue on the hapi repo?

@AdriVanHoudt
Copy link

Also on macOS and AIX where two tests fail:

 Failed tests:
   391) Request active() exits handler early when request is no longer active:
       Timed out (5000ms) - Request active() exits handler early when request is no longer active
   415) Request _postCycle() skips onPreResponse when validation terminates request:
       Timed out (5000ms) - Request _postCycle() skips onPreResponse when validation terminates request
 2 of 967 tests failed

I can confirm these failures on my MacOS 11.1 node 14.15.4.
MacOS 10.15.7 did pass on Github Actions CI with node 14.15.1 https://github.com/hapijs/hapi/runs/1622790060?check_suite_focus=true
What I get from Jenkins (not familiar with it and not super straightforward to navigate) these failures also happen on MacOS 10.15.4 node 14 https://ci.nodejs.org/job/citgm-smoker-nobuild/nodes=osx1015/994/injectedEnvVars/

@kanongil
Copy link

kanongil commented Jan 7, 2021

The lsof tests are not critical to the suite, and are already conditionally omitted on windows. We could revise the condition to check for lsof availability instead.

@devinivy
Copy link

For those following along, there are some proposed fixes for the osx, aix, and lsof-related failures here: hapijs/hapi#4221 🤞

@devinivy
Copy link

devinivy commented Feb 2, 2021

@targos those fixes landed in hapi, so we should be ready for another run. I wasn't able to reproduce the issues on all platforms because I do not have access to them, but I made an effort to reduce or eliminate timing dependence in these failing tests, and I cleaned-up the lsof-related problems. If after the run any issues remain on OSX or AIX, is there any way we could utilize the citgm CI to help debug them?

@targos
Copy link
Member

targos commented Feb 2, 2021

@targos
Copy link
Member

targos commented Feb 2, 2021

Is it still supposed to fail on Node.js 15 ?

Example: https://ci.nodejs.org/job/citgm-smoker-nobuild/1025/nodes=ubuntu1804-64/testReport/junit/(root)/citgm/_hapi_hapi_v20_1_0/

added 215 packages in 12s
 > @hapi/hapi@20.1.0 test
 > lab -a @hapi/code -t 100 -L -m 5000
   
   ..................................................
   ..................................................
   ..............................2...................
   ..................................................
   ..................................................
   ..................................................
   ..............................................2...
   ............................................2.....
   .2................2...............................
   ..................................................
   ..................................................
   ..................................................
   ..................................................
   ..................................................
   ..................................................
   ..................................................
   .................................x................
   ..................................................
   ..................................................
   .......................
 Failed tests:
   834) transmission marshal() does not crash when request is aborted:
       Timed out (5000ms) - transmission marshal() does not crash when request is aborted
 1 of 973 tests failed
 Test duration: 18291 ms
 Assertions count: 2039 (verbosity: 2.10)
 Leaks: No issues
 Coverage: 100.00%
 Lint: No issues
 npm ERR! code 1
 npm ERR! path /home/iojs/tmp/citgm_tmp/e4f6e596-ecf1-4a88-80e2-fa7490d61540/@hapi/hapi
 npm ERR! command failed
 npm ERR! command sh -c lab -a @hapi/code -t 100 -L -m 5000
 npm ERR! A complete log of this run can be found in:
 npm ERR!     /home/iojs/tmp/citgm_tmp/npm-cache/_logs/2021-02-02T06_44_06_314Z-debug.log

@devinivy
Copy link

devinivy commented Feb 2, 2021

In node 15.5.0 there was a change that broke hapi hapijs/hapi#4208 nodejs/node#33035. This change to node was reverted in 15.6.0, and hapi's test suite was fixed. Now on 15.7.0 it seems there's a new issue. I can reproduce it locally so I will check it out from the hapi perspective, but it does seem like it relates to some change in 15.7.0 (possibly nodejs/node#36821?).

@kanongil
Copy link

kanongil commented Feb 2, 2021

Seems very likely.

Hapi relies on the 'aborted' event triggering before 'end' / 'close' / 'error' to signal it: https://github.com/hapijs/hapi/blob/6a3452477de7a512fb0022f3a06d45d527b82e48/lib/request.js#L716-L722.

And the failing test relies on the signal.

@targos
Copy link
Member

targos commented Feb 2, 2021

@nodejs/http ^

@ronag
Copy link
Member

ronag commented Feb 2, 2021

Hapi relies on the 'aborted' event triggering before 'end' / 'close' / 'error' to signal it: hapijs/hapi@6a34524/lib/request.js#L716-L722.

That link looks at abort not aborted?

@kanongil
Copy link

kanongil commented Feb 2, 2021

Yeah, confusingly it registers on 'aborted', and forwards it as 'abort'.

@dnlup
Copy link

dnlup commented Feb 3, 2021

Just thinking out loud here.

The order in which the aborted event is emitted should not have changed by nodejs/node#36821.

AbortIncoming calls request.destroy which in turn emits aborted and then close/error (https://github.com/nxtedition/node/blob/43fc062f278c22f4284b55dbf842525bf464d5de/lib/_http_incoming.js#L172).

I'll try to verify on the failing test, though.

@ronag
Copy link
Member

ronag commented Feb 3, 2021

@dnlup: I think this has to do with server side, not client side.

@ronag
Copy link
Member

ronag commented Feb 3, 2021

I'd suggest we try a revert of nodejs/node@708728d to see if that resolves the issue.

@devinivy
Copy link

devinivy commented Feb 28, 2021

Just checking in: is there anything we can do on the hapi side to help move this forward, or is this being looked into on the node side to determine whether the cause of the new failure in 15.7.0 was nodejs/node@708728d?

@dnlup
Copy link

dnlup commented Feb 28, 2021

Apologies for the late reply. The failing test seems to be in test/transmit.js, and reverting nodejs/node@708728d from the v15.7.0 tag makes the test pass again, so your suspicion was right.

@dnlup
Copy link

dnlup commented Feb 28, 2021

Diggin a little bit further, before the change what happened was:

  1. socketOnEnd is called
  2. abortIncoming is called on req
  3. socketOnClose is called
  4. abortIncoming is called again

While with the changes:

  1. socketOnEnd is called
  2. socketOnClose is called
  3. abortIncoming is called on req

This makes sense to me on the Node side. I have to dig on the hapi side. You would be faster than me @devinivy on that part, assuming these pieces of information are useful in any way.

@devinivy
Copy link

@dnlup hey, no sweat. Will take a look on the hapi side, thanks!

@devinivy
Copy link

@dnlup node v15.7.0 did uncover a bug on our end! The issue for us wasn't the double emit of abort, but the timing between abort and close (they now seems to happen in the same tick, more info in hapijs/hapi#4234.) We were able to ship a fix in hapi v20.1.1. We are ready for another CI run 🤞 👍

@targos
Copy link
Member

targos commented Mar 16, 2021

@targos
Copy link
Member

targos commented Mar 16, 2021

@dnlup
Copy link

dnlup commented Mar 16, 2021

@devinivy Great news you were able to find the problem and fix it. Thank you!

@targos targos removed the WIP Work in progress label Mar 16, 2021
@targos targos merged commit d170bd6 into nodejs:main Mar 16, 2021
MylesBorins pushed a commit that referenced this pull request Mar 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.