search

spdy-http2 / node-spdy

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

Protocol error on push promise #237

Closed lewispham closed 8 years ago

lewispham commented 8 years ago

I got the protocol error while I was trying to push multiple files via PUSH_PROMISE. Here is the sample code.

spdy.createServer(options, function(req, res) {
    ['/app/main.js','/app/main.css','/app/favicon-32x32.png'].forEach(function(url){
        console.log('fetching',url);
      var stream = res.push(url, {
          'content-type': `${MIME[url.substr(url.lastIndexOf('.')+1)]};charset=utf-8`,
          'content-encoding' : 'gzip',
        });
      stream.on('error', function(er) {
          throw er;
      });
      var fstream = fs.createReadStream('/home'+url);
      fstream.pipe(stream);
    });

  res.end(`
    <!DOCTYPE html>
    <html>
        <head>
            <script src="/app/main.js" async></script>
            <link rel="stylesheet" href="/app/main.css" async>
            <link rel="icon" type="image/png" href="/app/favicon-32x32.png" sizes="32x32" async>
            <meta name="viewport" content="width=device-width, initial-scale=1">
        </head>
        <body></body>
    </html>`
  );

Result: untitled

Anything wrong with my code? Or is this a node-spdy bug?

indutny commented 8 years ago

@tresdin may I ask you to provide npm ls spdy output here?

lewispham commented 8 years ago

@indutny No problem. untitled2 That's all I have.

indutny commented 8 years ago

@tresdin thank you! May I ask you to also run npm ls spdy-transport?

lewispham commented 8 years ago

@indutny Here you are. untitled3

indutny commented 8 years ago

Thank you! Two more question to you:

  1. What browser are you using to test it? Does it fail in other browser too?
  2. Could you please run the same scenario with DEBUG="spdy*" environment variable and upload the output log to gist.github.com ?

Thank you again!

lewispham commented 8 years ago

@indutny

  1. I tested on Chrome Version 47.0.2526.106 m (64-bit). Here is the result on Firefox.

untitled-firefox

  1. Could you point me out the location of log file? I've tested with node /home/app/index.js DEBUG="spdy*" but nothing else happened.
indutny commented 8 years ago

@tresdin you will need to run it like this DEBUG="spdy*" node /home/app/index.js. Thanks for testing it on Firefox.

lewispham commented 8 years ago

@indutny My output log https://gist.github.com/tresdin/908a0ebf0ea67a9c9783

lewispham commented 8 years ago

@indutny Sorry for the wrong log. Here is the one with this error. https://gist.github.com/tresdin/e557984ceba774853e3e

indutny commented 8 years ago

Thank you very much for detailed logs. However, I'm afraid I still need a bit more information than this.

May I ask you to open chrome://net-internals/#export in Chrome before testing it (please make sure that you do not have any other tabs open so that they won't leak any private data), start the server, reproduce the issue, and click "Save to file" there in chrome. You may want to send the output log by email, just in case if it contains any private data (I will make sure nothing will be exposed).

Thank you again!

indutny commented 8 years ago

Nevermind, I know what is happening. Will create a fix today.

indutny commented 8 years ago

Ah, actually I was wrong. I still need that net-internals log. Sorry!

lewispham commented 8 years ago

@indutny I've sent the log to fedor@indutny.com.

indutny commented 8 years ago

@tresdin thank you! So the answer for our problem is:

Received duplicate pushed stream with url: https://hostname/app/favicon-32x32.png

I think if you will handle err instead of re-throwing it in error event listener - you should be able to handle it. It doesn't look like a problem on node-spdy side.

The idea behind this error message is that you actually push streams on every incoming request, and this error happens if you push the same url twice on the same TLS connection.

Thank you very much for providing the log.

lewispham commented 8 years ago

@indutny

The idea behind this error message is that you actually push streams on every incoming request

This error still stands when requests are filtered.

if(req.url === '/'){
    ['/app/main.js','/app/main.css','/app/favicon-32x32.png'].forEach(function(url){
        console.log('fetching',url);
      var stream = res.push(url, {
          'content-type': `${MIME[url.substr(url.lastIndexOf('.')+1)]};charset=utf-8`,
          'content-encoding' : 'gzip',
        });
      stream.on('error', function(er) {
          throw er;
      });
      var fstream = fs.createReadStream('/home'+url);
      fstream.pipe(stream);
    });

  res.end(`
    <!DOCTYPE html>
    <html>
        <head>
            <script src="/app/main.js" async></script>
            <link rel="stylesheet" href="/app/main.css" async>
            <link rel="icon" type="image/png" href="/app/favicon-32x32.png" sizes="32x32" async>
            <meta name="viewport" content="width=device-width, initial-scale=1">
        </head>
        <body></body>
    </html>`
  );
}

this error happens if you push the same url twice on the same TLS connection

How can url be pushed twice on the same TLS connection with my sample code? And also, what is your solution for this error?

indutny commented 8 years ago

@tresdin the browser may cancel any PUSH stream if it thinks that it needs to do so (be it duplicate streams, or whatever). The solution is to handle error event on PUSH stream, and act accordingly.

lewispham commented 8 years ago

@indutny I just want to know the cause of the duplication. You've confirmed that this error is not from node spdy side. So which part of my code causes this error? Or is this a Chrome bug?

indutny commented 8 years ago

@tresdin this is not a bug at all. Browser just decides that it does not want to receive any data from that PUSH stream and cancels it. This is completely legal in HTTP2

lewispham commented 8 years ago

@indutny I think it should be considered as a bug. Browsers assume that this behaviour is incorrect on server side when some duplicated streams are sent. So the problem can only be solved when browsers stop rejecting incoming pushed streams.

indutny commented 8 years ago

@tresdin I don't get why it is a bug. Please remove throw err from your code, and it will work just fine.