spdy-http2 / node-spdy

SPDY server on Node.js
2.81k stars 196 forks source link

Fails on Node.js 4.6.0 #306

Closed Shuunen closed 5 years ago

Shuunen commented 7 years ago

Hi,

I just went throught "res.push is not a function" error while using node v4, now I switched to v6, it works,

It could be nice to put this as requirement in the Readme :)

daviddias commented 7 years ago

We currently test against Node.js versions: 0.10, 0.12, 4, 6 and 7, are you 100% positive it was the Node.js version causing the issue? If yes, can you provide more info or even better, a repro case? Thank you

Shuunen commented 7 years ago

To reproduce I use node 4.6.0 :

nvm use 4.6.0
# Now using node v4.6.0 (npm v2.15.9)

This is my server.js :

var fs = require('fs');
var port = 1403;
var express = require('express');
var http2 = require('spdy');
var app = express();
var options = {
    key: fs.readFileSync('./certs/server.key'),
    cert: fs.readFileSync('./certs/server.crt')
};
http2.createServer(options, app).listen(port, (error) => {
    if (error) {
        console.log(error);
    } else {
        console.log('server started on https://localhost:' + port);
    }
});

app.get('/pushy', (req, res) => {
    var stream = res.push('/main.js', {
        request: {
            accept: '*/*'
        },
        response: {
            'content-type': 'application/javascript'
        }
    });
    stream.end('alert("hello from push stream!");');
    res.end('<script src="/main.js"></script>');
});

Then I access : https://localhost:1403/pushy

With node 6 it works & shows me the alert("hello from push stream!")

With node 4.6.0 I got this error :

TypeError: res.push is not a function
   at /test/server2.js:19:22
   at Layer.handle [as handle_request] (/test/node_modules/express/lib/router/layer.js:95:5)
   at next (/test/node_modules/express/lib/router/route.js:131:13)
   at Route.dispatch (/test/node_modules/express/lib/router/route.js:112:3)
   at Layer.handle [as handle_request] (/test/node_modules/express/lib/router/layer.js:95:5)
   at /test/node_modules/express/lib/router/index.js:277:22
   at Function.process_params (/test/node_modules/express/lib/router/index.js:330:12)
   at next (/test/node_modules/express/lib/router/index.js:271:10)
   at expressInit (/test/node_modules/express/lib/middleware/init.js:33:5)
   at Layer.handle [as handle_request] (/test/node_modules/express/lib/router/layer.js:95:5)
sericaia commented 7 years ago

I just tested and the res is not even using SPDY. It fallbacks to H1.

image

I know it does not solve your problem, but a good practise is to add a validation because you may not be talking with a client that supports H2:

if (!req.isSpdy) {
    return res.end('SPDY is off. We cannot use Server Push :(')
}
daviddias commented 7 years ago

Thanks @sericaia, good check. Any thoughts on why it works with Node.js 6 in this case?

MiguelCastillo commented 7 years ago

I was running into this issue and eventually found that ALPN was added in node 4.8.0. Without ALPN, chrome will use http/1.1. What this means is that in node 4.8.0 and above, chrome will use h2. With version earlier than that, chrome will use http/1.1.

for reference https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V4.md#2017-02-21-version-480-argon-lts-mylesborins

Curl can ignore the ALPN negotiation step and continue to use h2.

Below is curl with nodejs 4.8.0

➜  3dub git:(master) ✗ curl https://localhost:3000/ -kiv
*   Trying ::1...
* TCP_NODELAY set
* Connected to localhost (::1) port 3000 (#0)
* ALPN, offering h2
* ALPN, offering http/1.1
* Cipher selection: ALL:!EXPORT:!EXPORT40:!EXPORT56:!aNULL:!LOW:!RC4:@STRENGTH
* successfully set certificate verify locations:
*   CAfile: /usr/local/etc/openssl/cert.pem
  CApath: /usr/local/etc/openssl/certs
* TLSv1.2 (OUT), TLS header, Certificate Status (22):
* TLSv1.2 (OUT), TLS handshake, Client hello (1):
* TLSv1.2 (IN), TLS handshake, Server hello (2):
* TLSv1.2 (IN), TLS handshake, Certificate (11):
* TLSv1.2 (IN), TLS handshake, Server key exchange (12):
* TLSv1.2 (IN), TLS handshake, Server finished (14):
* TLSv1.2 (OUT), TLS handshake, Client key exchange (16):
* TLSv1.2 (OUT), TLS change cipher, Client hello (1):
* TLSv1.2 (OUT), TLS handshake, Finished (20):
* TLSv1.2 (IN), TLS change cipher, Client hello (1):
* TLSv1.2 (IN), TLS handshake, Finished (20):
* SSL connection using TLSv1.2 / ECDHE-RSA-AES128-GCM-SHA256
* ALPN, server accepted to use h2
* Server certificate:
*  subject: C=US; ST=MI; L=Det; O=Ltd; OU=Ltd; CN=localhost; emailAddress=test@test.com
*  start date: Sep 14 13:49:48 2017 GMT
*  expire date: Sep 14 13:49:48 2018 GMT
*  issuer: C=US; ST=MI; L=Det; O=Ltd; OU=Ltd; CN=localhost; emailAddress=test@test.com
*  SSL certificate verify result: self signed certificate (18), continuing anyway.
* Using HTTP2, server supports multi-use
* Connection state changed (HTTP/2 confirmed)
* Copying HTTP/2 data in stream buffer to connection buffer after upgrade: len=0
* Using Stream ID: 1 (easy handle 0x7fff43800c00)
> GET / HTTP/2
> Host: localhost:3000
> User-Agent: curl/7.55.1
> Accept: */*
> 
* Connection state changed (MAX_CONCURRENT_STREAMS updated)!
< HTTP/2 200 
HTTP/2 200 
< x-powered-by: Express
x-powered-by: Express
< accept-ranges: bytes
accept-ranges: bytes
< cache-control: public, max-age=0
cache-control: public, max-age=0
< last-modified: Thu, 14 Sep 2017 03:50:46 GMT
last-modified: Thu, 14 Sep 2017 03:50:46 GMT
< etag: W/"b3-15e7e82a9f0"
etag: W/"b3-15e7e82a9f0"
< content-type: text/html; charset=UTF-8
content-type: text/html; charset=UTF-8
< content-length: 179
content-length: 179

< 
<html>
  <head>
    <title>Hello world!!</title>
    <script type="text/javascript" src="bundle.js" defer></script>
  </head>
  <body>
    <div id="root"></div>
  </body>
</html>

And here is node 4.7.3 - notice the ALPN failure to negotiate the protocol...

➜  3dub git:(master) ✗ curl https://localhost:3000/ -kiv
*   Trying ::1...
* TCP_NODELAY set
* Connected to localhost (::1) port 3000 (#0)
* ALPN, offering h2
* ALPN, offering http/1.1
* Cipher selection: ALL:!EXPORT:!EXPORT40:!EXPORT56:!aNULL:!LOW:!RC4:@STRENGTH
* successfully set certificate verify locations:
*   CAfile: /usr/local/etc/openssl/cert.pem
  CApath: /usr/local/etc/openssl/certs
* TLSv1.2 (OUT), TLS header, Certificate Status (22):
* TLSv1.2 (OUT), TLS handshake, Client hello (1):
* TLSv1.2 (IN), TLS handshake, Server hello (2):
* NPN, negotiated HTTP2 (h2)
* TLSv1.2 (IN), TLS handshake, Certificate (11):
* TLSv1.2 (IN), TLS handshake, Server key exchange (12):
* TLSv1.2 (IN), TLS handshake, Server finished (14):
* TLSv1.2 (OUT), TLS handshake, Client key exchange (16):
* TLSv1.2 (OUT), TLS change cipher, Client hello (1):
* TLSv1.2 (OUT), TLS handshake, Unknown (67):
* TLSv1.2 (OUT), TLS handshake, Finished (20):
* TLSv1.2 (IN), TLS change cipher, Client hello (1):
* TLSv1.2 (IN), TLS handshake, Finished (20):
* SSL connection using TLSv1.2 / ECDHE-RSA-AES128-GCM-SHA256
* ALPN, server did not agree to a protocol
* Server certificate:
*  subject: C=US; ST=MI; L=Det; O=Ltd; OU=Ltd; CN=localhost; emailAddress=test@test.com
*  start date: Sep 14 13:49:48 2017 GMT
*  expire date: Sep 14 13:49:48 2018 GMT
*  issuer: C=US; ST=MI; L=Det; O=Ltd; OU=Ltd; CN=localhost; emailAddress=test@test.com
*  SSL certificate verify result: self signed certificate (18), continuing anyway.
* Using HTTP2, server supports multi-use
* Connection state changed (HTTP/2 confirmed)
* Copying HTTP/2 data in stream buffer to connection buffer after upgrade: len=0
* Using Stream ID: 1 (easy handle 0x7ff15d004c00)
> GET / HTTP/2
> Host: localhost:3000
> User-Agent: curl/7.55.1
> Accept: */*
> 
* Connection state changed (MAX_CONCURRENT_STREAMS updated)!
< HTTP/2 200 
HTTP/2 200 
< x-powered-by: Express
x-powered-by: Express
< accept-ranges: bytes
accept-ranges: bytes
< cache-control: public, max-age=0
cache-control: public, max-age=0
< last-modified: Thu, 14 Sep 2017 03:50:46 GMT
last-modified: Thu, 14 Sep 2017 03:50:46 GMT
< etag: W/"b3-15e7e82a9f0"
etag: W/"b3-15e7e82a9f0"
< content-type: text/html; charset=UTF-8
content-type: text/html; charset=UTF-8
< content-length: 179
content-length: 179

< 
<html>
  <head>
    <title>Hello world!!</title>
    <script type="text/javascript" src="bundle.js" defer></script>
  </head>
  <body>
    <div id="root"></div>
  </body>
</html>

I hope this helps, and it would really help to have this noted in the readme. I am happy to send a PR for that.

jacobheun commented 5 years ago

Closing as node 4 is no longer supported. We recently updated to support node 6,8, and 10 though.