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 - compat not working as http #15491

Closed
ronag opened this issue Sep 20, 2017 · 15 comments
Closed

http2 - compat not working as http #15491

ronag opened this issue Sep 20, 2017 · 15 comments
Labels
confirmed-bug Issues with confirmed bugs. http2 Issues or PRs related to the http2 subsystem.

Comments

@ronag
Copy link
Member

ronag commented Sep 20, 2017

I've got a weird case where http & https works without problem. However, http2 compat does not.

In the case where I'm trying to do a PATCH, PUT or POST the request doesn't finish with http2. GET, HEAD and other "safe" methods work without problem.

Given that http2 compat should be mostly an in place replacement for http I believe this is a bug somewhere?

I'm basically using http2-proxy something like:

const http2 = require('http2')
const proxy = require('http2-proxy')
const http = require('http')
const fs = require('fs')

const proxyServer = http2
   .createSecureServer({
     cert: fs.readFileSync('./server.crt'),
     key: fs.readFileSync('./server.key'),
     allowHTTP1: true
   })
  .on('request', (req, res) => {
    proxy.web(req, res, {
      hostname: 'localhost',
      port: 7000
    }, err => {
      if (err) {
        console.error('proxy error', err)
      }
    })
  })
  .listen(6000)

const server = http
  .createServer()
  .on('request', (req, res) => {
    req
      .pipe(fs.createWriteStream(Math.random().toString()))
      .on('finish', () => {
        res.statusCode = 200
        res.end()
     })
  })
  .listen(7000)

Can't create a full repo since I don't know how to do http2 requests in node.

@ronag
Copy link
Member Author

ronag commented Sep 20, 2017

I've tried with master.

@apapirovski
Copy link
Member

apapirovski commented Sep 20, 2017

Just an FYI, but you can't do allowHTTP1 with createServer, only createSecureServer. Not sure if that's the culprit as I'm having trouble following exactly what's going wrong. Any more debugging info or a full-source that I can run to reproduce it would be helpful. Thanks!

Also browsers won't connect via http2 to a non-secure server.

@ronag
Copy link
Member Author

ronag commented Sep 20, 2017

@apapirovski: It's the same with createSecureServer.

@ronag
Copy link
Member Author

ronag commented Sep 20, 2017

A full source is going to be difficult since there are quite a lot of components involved to get it fully working.

Have to think about how/if I can create a repo case you can run.

What kind of debugging info would be useful?

@ronag
Copy link
Member Author

ronag commented Sep 20, 2017

Do you have an example of how to create a http2 file upload using a client in node?

@apapirovski
Copy link
Member

Yeah, for sure. This test has a fully working example: https://github.com/nodejs/node/blob/master/test/parallel/test-http2-client-upload.js

@ronag
Copy link
Member Author

ronag commented Sep 20, 2017

@apapirovski: The minimal case seems to work. So basically:

  • http2 - minimal works
  • http2 - full setup doesn't works
  • http - full setup works

Not sure where to go from here...

@apapirovski
Copy link
Member

apapirovski commented Sep 20, 2017

Can you tell me a bit more about what's happening in the full case? How exactly is the comparison for http vs http2 being done — are you just swapping http2.createSecureServer for the equivalent http/https version or something more? How far does it get in the http2 version — does the proxied to server receive the data at all?

Would love to help (since it sounds like there might be a legitimate issue) but it's hard without insight into what your code is doing, especially since the reduced test case works for you.

@ronag
Copy link
Member Author

ronag commented Sep 20, 2017

I'm making a simple proxy that forwards http2 requests from Chrome to our backend storage server:

Currently, I simply switch from http2 to http like this:

// Doesn't work
// const httpsServer = http2
//   .createSecureServer({
//     cert: readFileSync('./server.crt'),
//     key: readFileSync('./server.key'),
//     allowHTTP1: true
//   })
const httpsServer = https
  .createServer({
    cert: readFileSync('./server.crt'),
    key: readFileSync('./server.key')
  })

When I try to upload from Chrome the requests are stuck on "pending":

screen shot 2017-09-20 at 15 52 35
screen shot 2017-09-20 at 15 52 44
screen shot 2017-09-20 at 15 52 51

@ronag
Copy link
Member Author

ronag commented Sep 20, 2017

Using nginx instead as http2->http1 proxy works. So I don't think it's a problem with the storage server.

We're trying to replace nginx with a minimal node alternative.

@apapirovski
Copy link
Member

I think I'm able to replicate the issue. Looking into it.

@apapirovski
Copy link
Member

Issue is definitely in http2 and I can reproduce it with a minimal test case involving pipe. Trying to find the root cause and will submit a PR when done. Thanks for the report @ronag.

@mscdex mscdex added the http2 Issues or PRs related to the http2 subsystem. label Sep 20, 2017
@teerapat1739
Copy link

this same problem with me

@benjamingr benjamingr added the confirmed-bug Issues with confirmed bugs. label Sep 20, 2017
@apapirovski
Copy link
Member

@ronag I have a proposed patch in #15503. If you wouldn't mind testing it against your code, that would be absolutely amazing. I think my test case matches but it's possible there's also another issue at play.

@ronag
Copy link
Member Author

ronag commented Sep 21, 2017

@apapirovski: Yep, that fixes it!

apapirovski added a commit to apapirovski/node that referenced this issue Sep 21, 2017
Handle edge case where stream pause is called between resume being
called and actually evaluated. Other minor adjustments to avoid
various edge cases around stream events. Add new tests that cover
all changes.

Fixes: nodejs#15491
jasnell pushed a commit that referenced this issue Sep 25, 2017
Handle edge case where stream pause is called between resume being
called and actually evaluated. Other minor adjustments to avoid
various edge cases around stream events. Add new tests that cover
all changes.

Fixes: #15491
PR-URL: #15503
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Sep 29, 2017
Handle edge case where stream pause is called between resume being
called and actually evaluated. Other minor adjustments to avoid
various edge cases around stream events. Add new tests that cover
all changes.

Fixes: #15491
PR-URL: #15503
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit to addaleax/ayo that referenced this issue Sep 30, 2017
Handle edge case where stream pause is called between resume being
called and actually evaluated. Other minor adjustments to avoid
various edge cases around stream events. Add new tests that cover
all changes.

Fixes: nodejs/node#15491
PR-URL: nodejs/node#15503
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

No branches or pull requests

5 participants