spdy-http2 / node-spdy

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

Any reason .push() would work on chrome and safari mobile but not FF or regular safari? #242

Open crvarner opened 8 years ago

crvarner commented 8 years ago

Here is an express middleware function used to test:

var jquery = fs.readFileSync('public/js/jquery.min.js');

router.use('/test', function(req,res,next) {
    if (req.isSpdy) {
        var headers = { 'content-type': 'application/javascript' };

        res.push('/js/jquery.min.js', headers, function(err,stream) {
            stream.on('error', function(err) {
                stream.end();
                console.log(err);
            });
            stream.end(jquery);
        });
    }
    next();
});

The code above successfully pushes the file contents to the browser in Chrome. In Safari mobile, there is an error which is ignored and the file is loaded normally in another request (I can see the GET in the concole).

In both FF and Safari (desktop), the page fails to load completely. There are no errors in the console. The only peculiar thing is console shows the following:

GET /test - - ms - -

vs. Chrome:

GET /test 200 55.056 ms - 8147

Commenting out the code above, FF/Safari load the page normally (but with a 304):

GET /test 304 35.207 ms - -

Also, the code within if (req.isSpdy) is being run in all cases.

indutny commented 8 years ago

Could it be that you are using not that latest version of spdy-transport dependency for node-spdy? Does npm update in the project's directory help?

I believe that I have fixed very similar issue recently.

crvarner commented 8 years ago

just running npm update in the root directory of my express app? didn't change anything. I only npm install spdyed maybe 4 days ago. How does the browser make a difference?

crvarner commented 8 years ago

If it helps I am creating the spdy server as follows:

var app         = require('../app');
var fs          = require('fs');
var path        = require('path');
var spdy        = require('spdy');

var options = {
    key: fs.readFileSync(path.join('keys','server.key')),
    cert: fs.readFileSync(path.join('keys','server.crt')),
    ca: fs.readFileSync(path.join('keys','server.csr')),

    spdy: {
        protocols: ['h2', 'spdy/3.1', 'spdy/3', 'http/1.1'],
        plain: false
    }
}

spdy.createServer(options,app).listen(8081);
indutny commented 8 years ago

What does npm ls say?

crvarner commented 8 years ago

spdy-transport@2.0.6

indutny commented 8 years ago

Oh, yeah... it is the latest. Thank you!

May I ask you to run your app with DEBUG="spdy*" environment variable, reproduce the issue with firefox, and send me the logs (probably using gist)?

crvarner commented 8 years ago

https://gist.github.com/crvarner/79f8cd957407f354ed82

note: the path is actually called /support not /test

indutny commented 8 years ago

Yikes! Should be fixed now, please update!

crvarner commented 8 years ago

No longer at work but I will let you know if this fixes it tomorrow! Thank you for the quick response!

crvarner commented 8 years ago

now running spdy-transport@2.0.7 and still no luck. Diffing the spdy debugs show no difference other than some +ms values.

I put together a repo to recreate the problem.

https://github.com/crvarner/spdy-firefox-bug

crvarner commented 8 years ago

Using charles request proxy, the only difference in the two requests is the presence on a Connection: keep-alive header on the Firefox request.

Here is the logs from a Safari request: https://gist.github.com/crvarner/908e4da3b13449487b47

And the respective Safari error shown in the browser:

Safari can't open the page "https://localhost:3000/". The error is: "The operation couldn't be completed. Protocol error" (NSPOSIXErrorDomain:100)

crvarner commented 8 years ago

Also, not exactly sure how you intended error handling to be done. Currently using this form:

res.push( path, headers, function(err, stream) {
    stream.on('error', function(err) {
        stream.end();
        console.log(err);
    });
    stream.end(content);
});

While the page fails to load in FF, the errors ( [Error: socket hang up] code: 'ECONNRESET' ) are printed and the app continues to serve. In Safari, the app breaks completely with an unhandled error thrown from the same spot.

crvarner commented 8 years ago

Please check out the README of the repo I linked. It has the most comprehensive documentation of the issue. This way you can clone it and log whatever you wish while running the scenarios I specified.

indutny commented 8 years ago

Looking, thank you for the information!

indutny commented 8 years ago

@crvarner I can tell for sure that STREAM_REFUSED is ok if the rest of requests a served normally. Browser can cancel any PUSH stream for any reason.

I will look into Safari issue carefully now, as it does seem to be some kind of spdy-transport problem

crvarner commented 8 years ago

I knew a browser can cancel a push stream, but still that should not hang the entire page and require the cache to be cleared before the page will load again.

After the STREAM_REFUSED error, Firefox is no longer able to load the initial request of a page refresh. Let alone, display the mentioned fallback behavior and serve up assets normally (1 request per asset)

indutny commented 8 years ago

This is pretty strange, I have just downloaded your code and tried it in Chrome/FF/Safari, and everywhere I got a JS alertbox without any errors on the console...

Also I don't see any problems in the gist that you sent to me.

What node.js version are you using?

crvarner commented 8 years ago

v4.1.0

crvarner commented 8 years ago

That is really strange... Did you also try https://localhost:3000/weird? Does it get pushes on Chrome, FF, and Safari?

crvarner commented 8 years ago

After updating to v5.4.1 still no dice

crvarner commented 8 years ago

I will clone repo to a different machine and try when I get home. Thanks again for the help with this.

indutny commented 8 years ago

Weird appears to be working too, but I see the spinning circle in Firefox.

It would really help to get that DEBUG output during one of the problems...

crvarner commented 8 years ago

Okay so I have cloned and tried the code on a new machine with a fresh install of npm/node and got the same results (on firefox only. the new machine is windows so no Safari)

The following gist was achieved by starting the server, navigating to https://localhost:3000/ in firefox (which shows the alert), then clicking refresh (which fails to load anything). I marked in the gist where the separate requests take place in the logs. Any subsequent attempts at accessing the page result in failures matching what is displayed after the #### page refresh #### delimiter

https://gist.github.com/crvarner/1c5d7da27523d5b9fc7b

Also a note: the fallback behavior I mentioned earlier took place in the initial load. The logs show GET /asset.js 200 ... meaning no push() was performed.

indutny commented 8 years ago

Gosh, I know what's happening!

indutny commented 8 years ago

Please try updating, just pushed out spdy-transport@2.0.8 . Hopefully it will help.

indutny commented 8 years ago

Not sure about stuck loads, though... Will have to take a deeper look, just to be sure.

crvarner commented 8 years ago

spdy-transport@2.0.8 and same as before on FF. Requires a cleared cache to load.

indutny commented 8 years ago

Gosh, ok. Thank you for giving it a try.

indutny commented 8 years ago

@crvarner I'm sorry, the code was really broken for some paths. I have partially reverted recent changes until we will be able to resolve them. Please give a try to 2.0.9, hopefully it will help.

bcherny commented 8 years ago

i'm having the same issue when sending push streams on safari desktop. safari complains

Failed to load resource: The operation couldn’t be completed. Protocol error

server throws a bunch of

error: push stream error Error: socket hang up
    at TLSSocket.onclose (/Users/boris/Sites/foo/node_modules/spdy-transport/lib/spdy-transport/connection.js:186:15)
    at TLSSocket.g (events.js:260:16)
    at emitOne (events.js:82:20)
    at TLSSocket.emit (events.js:169:7)
    at Socket.<anonymous> (net.js:469:12)
    at Socket.g (events.js:260:16)
    at emitOne (events.js:77:13)
    at Socket.emit (events.js:169:7)
    at TCP._onclose (net.js:469:12)
error: push stream error Error: Stream write aborted
    at /Users/boris/Sites/foo/node_modules/spdy-transport/lib/spdy-transport/stream.js:144:16
    at doNTCallback0 (node.js:419:9)
    at process._tickDomainCallback (node.js:389:13)
error: push stream error Error: Stream write aborted

i added a temporary hack to get around this:

if (req.isSpdy && !req.headers['user-agent'].includes('Safari')) {
  // send push headers
}

versions: