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

HTTP2 simple test fails and hangs with node 12.17.0 & 12.18.0 #244

Closed
1 task
mgabeler-lee-6rs opened this issue Jun 17, 2020 · 10 comments
Closed
1 task
Assignees

Comments

@mgabeler-lee-6rs
Copy link

Checklist

  • Resolver issue

Detailed Description

With node 12.17.0 or 12.18.0:

$ npm test
...
  101 passing (2s)
  1 failing

  1) HTTP2 tests
       simple
         should match expected output:
     Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/mgl/3psrc/oas-kit/test/http2.test.js)
      at listOnTimeout (internal/timers.js:549:17)
      at processTimers (internal/timers.js:492:7)

and it hangs at this point, at least for a while.

Passes on 12.16.3

Other stuff

This is basically #223 again, but for the versions of node listed as supported, and is actively broken. One can have node LTS security fixes, or a working version of this package, but not both right now. That prior issue suggested 14.1.0 is where it broke and 14.3.0 fixed it, but didn't manage to identify an actual root cause change in either of those versions. I'd happily champion requesting a bugfix be backported to 12.x, but my node internals fu is not strong enough to identify which one is the problem there. I'm hoping someone here can help there.

See also wework/speccy#422

@MikeRalphson
Copy link
Contributor

Yep, it looks like unfortunately the Node.js regression testing missed a serious issue (hence the reversion in 14.3.0) and backporting the initial change to an LTS line was a major problem. I also don't know exactly what change caused the problem, and can only suggest we wait for 12.19.0 to see if the fix is backported too.

If not, I can look at disabling http2 either via a switch or a node version blocklist.

Speccy is, as far as I can tell, unmaintained so will never take a new version of oas-kit, so if that's your use-case you'll need to migrate to oas-kit's boast or spectral anyway.

@mgabeler-lee-6rs
Copy link
Author

Speccy is, as far as I can tell, unmaintained so will never take a new version of oas-kit

Speccy uses caret versions, so if a new version of oas-kit publishes that is compatible with it, it likely will get picked up on a fresh npm install

so if that's your use-case you'll need to migrate to oas-kit's boast or spectral anyway.

Was looking around for alternatives to speccy because of this, at first glance boast looks like it might fit our needs, thanks for the pointers 👍

@MikeRalphson
Copy link
Contributor

Speccy uses caret versions, so if a new version of oas-kit publishes that is compatible with it, it likely will get picked up on a fresh npm install

But speccy comes with a package-lock.json so I think it will use the locked versions unless you use it as a library?

Was looking around for alternatives to speccy because of this, at first glance boast looks like it might fit our needs, thanks for the pointers +1

boast is (at the moment) really just a simple example CLI for bits of oas-kit so if it is missing any features, please let me know.

@mgabeler-lee-6rs
Copy link
Author

package-lock.json is not published, it's only relevant when developing speccy. In our use case, we install speccy within our package as a devDependency as if it were a library, and use it as part of our CI workflow's linting step. But even if you did npm i -g speccy, the package-lock.json would still not be used due to it not being part of published packages. There is a separate npm-shrinkwrap.json (https://docs.npmjs.com/cli/shrinkwrap) that can apply dependency locks in a published package, but Speccy doesn't seem to use that.

@MikeRalphson
Copy link
Contributor

Still broken in Node 12.18.1

@MikeRalphson
Copy link
Contributor

As a further workaround, you can now override the default fetch function in the options object, i.e. with an alternative http2 fetch implementation or a non-http2 one.

@mgabeler-lee-6rs
Copy link
Author

12.18.3 has this, which looks promising: nodejs/node#33911

... but tests still in the same way for me with that version :(

@MikeRalphson
Copy link
Contributor

This looks to be fixed in node 12.20.0 - can anyone confirm please?

@mgabeler-lee-6rs
Copy link
Author

Confirmed on e26cda0 and node 12.20.0 for me: 104 passing (300ms)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants