lookup: add hapi #436

gibfahn · 2017-06-02T18:10:47Z

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

Checklist

contribution guidelines followed here

gibfahn · 2017-06-02T18:14:05Z

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 · 2017-06-02T18:15:34Z

CI on Node 6: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker-nobuild/150/
CI on Node 8: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker-nobuild/151/

DavidTPate · 2017-06-04T02:26:29Z

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.

gibfahn · 2017-07-18T05:30:17Z

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

hueniverse · 2017-07-18T07:29:58Z

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 · 2017-07-18T07:36:01Z

@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 · 2017-07-18T09:20:06Z

@mcollina hapijs/hapi#2121

targos · 2017-08-10T14:00:48Z

smoke test: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker-pipeline/2/
Node 4: https://ci.nodejs.org/job/citgm-smoker-nobuild/162/
Node 6: https://ci.nodejs.org/job/citgm-smoker-nobuild/163/
Node 8: https://ci.nodejs.org/job/citgm-smoker-nobuild/164/

targos · 2017-08-10T14:10:21Z

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 · 2017-08-10T16:30:46Z

Do the hapi tests require lsof?

cjihrig · 2017-08-10T19:52:58Z

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

MylesBorins · 2017-09-22T20:23:38Z

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

BridgeAR · 2018-02-17T05:51:22Z

What is the status here?

gibfahn · 2018-02-18T23:30:54Z

@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 · 2018-02-19T19:11:00Z

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

hapijs/hapi#3744

apapirovski · 2018-04-25T08:37:27Z

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.

targos · 2018-04-25T08:42:00Z

CI: https://ci.nodejs.org/blue/organizations/jenkins/citgm-smoker-pipeline/detail/citgm-smoker-pipeline/18/pipeline

BridgeAR · 2018-05-20T21:52:29Z

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

BridgeAR · 2018-05-21T01:50:30Z

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

al-k21 · 2018-05-21T17:42:54Z

CI: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker-pipeline/19/console

targos · 2021-01-04T18:59:04Z

New CI: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker-pipeline/132/

targos · 2021-01-04T19:03:58Z

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 · 2021-01-07T09:19:37Z

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 · 2021-01-07T09:33:57Z

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 · 2021-01-07T11:29:59Z

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 · 2021-01-19T05:17:15Z

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

devinivy · 2021-02-02T02:27:11Z

@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 · 2021-02-02T06:43:04Z

CI: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker-pipeline/136/console

targos · 2021-02-02T09:36:25Z

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/[email protected] 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 · 2021-02-02T14:22:14Z

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 · 2021-02-02T16:52:55Z

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 · 2021-02-02T16:58:01Z

@nodejs/http ^

ronag · 2021-02-02T17:06:40Z

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 · 2021-02-02T17:52:50Z

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

dnlup · 2021-02-03T10:39:49Z

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 · 2021-02-03T10:40:28Z

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

ronag · 2021-02-03T11:26:33Z

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

devinivy · 2021-02-28T16:30:54Z

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 · 2021-02-28T19:32:33Z

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 · 2021-02-28T20:55:31Z

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

socketOnEnd is called
abortIncoming is called on req
socketOnClose is called
abortIncoming is called again

While with the changes:

socketOnEnd is called
socketOnClose is called
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 · 2021-02-28T22:18:31Z

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

devinivy · 2021-03-16T02:18:05Z

@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 · 2021-03-16T07:14:50Z

GitHub CI: https://github.com/nodejs/citgm/actions/runs/656723779

targos · 2021-03-16T07:24:32Z

Jenkins CI: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker-pipeline/144/console

dnlup · 2021-03-16T09:42:58Z

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

Fixes: #8

gibfahn mentioned this pull request Jun 2, 2017

Add hapi support #8

Closed

gibfahn force-pushed the hapi branch from 824ddbd to 7b0515f Compare June 2, 2017 18:11

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

This comment was marked as off-topic.

Sign in to view

gibfahn changed the title ~~lookup: add hapi~~ WIP - lookup: add hapi Jul 18, 2017

This comment was marked as off-topic.

Sign in to view

cjihrig mentioned this pull request Feb 19, 2018

allow tests using lsof to pass on ENOENT hapijs/hapi#3744

Merged

al-k21 changed the title ~~WIP - lookup: add hapi~~ lookup: add hapi May 21, 2018

devinivy mentioned this pull request Jan 19, 2021

Fix tests for citgm hapijs/hapi#4221

Merged

devinivy mentioned this pull request Mar 3, 2021

Allow for res to have already closed during transmission hapijs/hapi#4234

Merged

targos removed the WIP Work in progress label Mar 16, 2021

targos merged commit d170bd6 into nodejs:main Mar 16, 2021

MylesBorins pushed a commit that referenced this pull request Mar 18, 2021

lookup: add hapi (#436)

1a385ec

Fixes: #8

lookup: add hapi #436

lookup: add hapi #436

Conversation

gibfahn commented Jun 2, 2017 • edited Loading

Checklist

gibfahn commented Jun 2, 2017 • edited by BridgeAR Loading

Hard Requirements

Soft Requirements

gibfahn commented Jun 2, 2017 • edited Loading

DavidTPate commented Jun 4, 2017

This comment was marked as off-topic.

gibfahn commented Jul 18, 2017

hueniverse commented Jul 18, 2017

mcollina commented Jul 18, 2017

hueniverse commented Jul 18, 2017

This comment was marked as off-topic.

targos commented Aug 10, 2017 • edited Loading

targos commented Aug 10, 2017

gibfahn commented Aug 10, 2017

cjihrig commented Aug 10, 2017

MylesBorins commented Sep 22, 2017

BridgeAR commented Feb 17, 2018

gibfahn commented Feb 18, 2018

cjihrig commented Feb 19, 2018

apapirovski commented Apr 25, 2018

targos commented Apr 25, 2018

BridgeAR commented May 20, 2018

BridgeAR commented May 21, 2018

al-k21 commented May 21, 2018 • edited Loading

targos commented Jan 4, 2021

targos commented Jan 4, 2021

AdriVanHoudt commented Jan 7, 2021 • edited Loading

AdriVanHoudt commented Jan 7, 2021

kanongil commented Jan 7, 2021

devinivy commented Jan 19, 2021

devinivy commented Feb 2, 2021

targos commented Feb 2, 2021

targos commented Feb 2, 2021 • edited Loading

devinivy commented Feb 2, 2021

kanongil commented Feb 2, 2021

targos commented Feb 2, 2021

ronag commented Feb 2, 2021

kanongil commented Feb 2, 2021

dnlup commented Feb 3, 2021

ronag commented Feb 3, 2021

ronag commented Feb 3, 2021 • edited Loading

devinivy commented Feb 28, 2021 • edited Loading

dnlup commented Feb 28, 2021 • edited Loading

dnlup commented Feb 28, 2021

devinivy commented Feb 28, 2021

devinivy commented Mar 16, 2021

targos commented Mar 16, 2021

targos commented Mar 16, 2021

dnlup commented Mar 16, 2021

gibfahn commented Jun 2, 2017 •

edited

Loading

gibfahn commented Jun 2, 2017 •

edited by BridgeAR

Loading

gibfahn commented Jun 2, 2017 •

edited

Loading

targos commented Aug 10, 2017 •

edited

Loading

al-k21 commented May 21, 2018 •

edited

Loading

AdriVanHoudt commented Jan 7, 2021 •

edited

Loading

targos commented Feb 2, 2021 •

edited

Loading

ronag commented Feb 3, 2021 •

edited

Loading

devinivy commented Feb 28, 2021 •

edited

Loading

dnlup commented Feb 28, 2021 •

edited

Loading