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 GOAWAY not working (error in popsicle as used by client-oauth2 package) #134

Closed
d1manson opened this issue Jun 9, 2020 · 14 comments
Closed
Labels

Comments

@d1manson
Copy link

d1manson commented Jun 9, 2020

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
Copy link
Member

blakeembrey commented Jun 9, 2020

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.

@blakeembrey blakeembrey added the bug label Jun 9, 2020
@d1manson
Copy link
Author

d1manson commented Jun 9, 2020

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
Copy link

tmatth commented Jun 10, 2020

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
Copy link

Metroninja commented Jun 10, 2020

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
Copy link
Member

blakeembrey commented Jun 11, 2020

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

Found that it's likely related to nodejs/node#32958 (comment). I have a patch locally to workaround this, but it doesn't sound like what this thread run into specifically.

@blakeembrey
Copy link
Member

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
Copy link
Author

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
Copy link

@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
Copy link

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
Copy link

tmatth commented Jun 11, 2020

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
Copy link
Member

@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
Copy link

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
Copy link
Member

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

@jasonterando
Copy link

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.

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

No branches or pull requests

5 participants