serviejs / popsicle

Simple HTTP requests for node and the browser
MIT License
246 stars 19 forks source link

http2 GOAWAY not working (error in popsicle as used by client-oauth2 package) #134

Closed d1manson closed 4 years ago

d1manson commented 4 years ago

I'm using client-oauth2 which in turn uses popsicle. In particular I tend to make requests to a server once or twice every few hours. For a week or so this seemed to work ok, but now I am getting the below error. Googling was unusually unhelpful on this, but I would have thought this would have been handled within node rather than by popsicle (I am using Node.js 12 running on 64bit Amazon Linux 2, elastic beanstalk). Any thoughts?

[ERR_HTTP2_GOAWAY_SESSION]: New streams cannot be created after receiving a GOAWAY 
at ClientHttp2Session.request (internal/http2/core.js:1610:13) 
at /var/app/current/node_modules/popsicle-transport-http/dist/index.js:299:36 
at new Promise (<anonymous>) 
at execHttp2 (/var/app/current/node_modules/popsicle-transport-http/dist/index.js:291:12) 
at /var/app/current/node_modules/popsicle-transport-http/dist/index.js:451:28 
at /var/app/current/node_modules/throwback/dist/index.js:26:32 
at new Promise (<anonymous>) 
at dispatch (/var/app/current/node_modules/throwback/dist/index.js:25:20) 
at next (/var/app/current/node_modules/throwback/dist/index.js:33:28) 
at /var/app/current/node_modules/popsicle-cookie-jar/dist/index.js:18:32 
at next (/var/app/current/node_modules/popsicle-cookie-jar/node_modules/tough-cookie/lib/cookie.js:1220:7) 
at /var/app/current/node_modules/popsicle-cookie-jar/node_modules/tough-cookie/lib/cookie.js:1208:5 
at MemoryCookieStore.findCookies (/var/app/current/node_modules/popsicle-cookie-jar/node_modules/tough-cookie/lib/memstore.js:113:3) 
at CookieJar.getCookies (/var/app/current/node_modules/popsicle-cookie-jar/node_modules/tough-cookie/lib/cookie.js:1189:9) 
at CookieJar.getCookieString (/var/app/current/node_modules/popsicle-cookie-jar/node_modules/tough-cookie/lib/cookie.js:1229:19) 
at res (/var/app/current/node_modules/popsicle-cookie-jar/dist/index.js:12:17) { code: 'ERR_HTTP2_GOAWAY_SESSION' } 

popsicle is a dependency of client-oauth2, as shown here - https://github.com/mulesoft/js-client-oauth2/blob/2752e8b3161ed06a6fddb5a6d042ec6a190929b0/package.json#L66.

Not sure how useful it will be, but for refernce I was talking to Xero's api, see - https://developer.xero.com/.

In an attempt to work around this, I am simply going to add one retry when the relevant request fails (not sure how popsicle works under the hood but maybe after the error has been caught the http2 connection will be reset or something...?!).

blakeembrey commented 4 years ago

Thanks for the report! Just for a sanity check, can you tell me the versions you’re using? It should be possible with npm ls | grep popsicle - mostly interested in the version of the HTTP transport.

d1manson commented 4 years ago

Here you go...

│ ├─┬ popsicle@12.0.5
│ │ ├── popsicle-content-encoding@1.0.0
│ │ ├─┬ popsicle-cookie-jar@1.0.0
│ │ ├── popsicle-redirects@1.1.0
│ │ ├─┬ popsicle-transport-http@1.0.7
│ │ ├── popsicle-transport-xhr@1.0.2
│ │ ├── popsicle-user-agent@1.0.0
tmatth commented 4 years ago

I also started seeing this systematically after an upgrade from Node 12.16.1 to 12.18.0, with these versions:

popsicle@12.0.5:
    popsicle-content-encoding "1.0.0"
    popsicle-cookie-jar "1.0.0"
    popsicle-redirects "1.1.0"
    popsicle-transport-http "1.0.7"
    popsicle-transport-xhr "1.0.2"
    popsicle-user-agent "1.0.0"

I was going to try forcing http1.1 (which can be done when initializing the http transport via the NegotiateHttpVersion option) as a workaround, but I don't think this is exposed to users of the client-oauth2 library.

Metroninja commented 4 years ago

not to +1 on this but I ran into this as well, spent a few hours on it today. I am using client-oauth2 as well. We have our docker images locked to from node:12, and our staging images started pulling down the new 12.18 version. switching to from node:12.16 solved our issue for now, but I think that leaves in the nasty high sev issues that were just fixed in place https://groups.google.com/forum/#!msg/nodejs-sec/UMBIO87oLbM/G3EzWRN4BQAJ

blakeembrey commented 4 years ago

I haven't been able to replicate GOAWAY yet, but I think I have replicated something along the lines of https://github.com/nodejs/node/issues/33343 on 12.17+. I'll keep working on resolving, sorry for the delay.

Found that it's likely related to https://github.com/nodejs/node/pull/32958#discussion_r418695485. I have a patch locally to workaround this, but it doesn't sound like what this thread run into specifically.

blakeembrey commented 4 years ago

I've published https://github.com/serviejs/popsicle-transport-http/releases/tag/v1.0.8 which you should get if you reinstall. I'm not sure that it fixes this issue, but may fix an issue with node.js ^12.17 since it looks like a flag was missed in the back port from the latest version.

d1manson commented 4 years ago

Thanks @blakeembrey. I wasn't expecting such a quick response, so I implemented a workaround (using a different request library.- client-oath2 supports using a custom request function.

It's kind of a pain to test this (as it was only happening in production), so I might not get to it for a few days now.

Metroninja commented 4 years ago

@d1manson @blakeembrey I have a local repro (you can force by using from node:12.18 in your dockerfile. I'm going to test this fix out shortly and I'll report back.

Metroninja commented 4 years ago

I can confirm it's fixed now, @blakeembrey you are my hero today. Since I'm using the client-oauth2 library I forced the version using the following resolution in my package.json

  "resolutions": {
    "client-oauth2/**/popsicle-transport-http":"^1.0.8"
  },

thanks for the incredibly quick resolution on this one.

tmatth commented 4 years ago

I can confirm it's fixed now, @blakeembrey you are my hero today. Since I'm using the client-oauth2 library I forced the version using the following resolution in my package.json

  "resolutions": {
    "client-oauth2/**/popsicle-transport-http":"^1.0.8"
  },

thanks for the incredibly quick resolution on this one.

Same here, thanks!

blakeembrey commented 4 years ago

@Metroninja would you mind sharing the repro? Would love to add a test covering it, although catching it on the specific version when it regressed in node.js is a different story.

Metroninja commented 4 years ago

To repro you need to be using the latest client-oauth2 package, on node 12.18 and on an oauth return call (aka after a successful auth) when extracting the token. specifically the following methodawait google.code.getToken(ctx.request.url);, and the google object in this case is new ClientOAuth2(googleConfig);.

AKA it's not a simple repro case to add test coverage for, at least in my use case. That said you can probably extract what they are doing here https://github.com/mulesoft/js-client-oauth2/blob/master/src/client-oauth2.js#L414 for your purposes

blakeembrey commented 4 years ago

Closing the issue since it's been resolved, but I'll try and revisit better tests around the HTTP2 connections.

jasonterando commented 3 years ago

Hi, for anybody using this library, make sure you create a new instance of ClientOAuth2 every time you are connecting to the OAuth server, even with the aforementioned update to popsicle-transport-http. In my case, I was passing an instance of it in via my constructor to make unit testing easier. The issue is that HTTP/2 sessions are being maintained during the life of the ClientOAuth2 object, so you can get the GOAWAY stream problem in a long running process.