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

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

Open
crvarner opened this issue Jan 19, 2016 · 30 comments

Comments

@crvarner
Copy link

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

indutny commented Jan 19, 2016

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

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

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

indutny commented Jan 19, 2016

What does npm ls say?

@crvarner
Copy link
Author

[email protected]

@indutny
Copy link
Collaborator

indutny commented Jan 19, 2016

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

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

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

@indutny
Copy link
Collaborator

indutny commented Jan 19, 2016

Yikes! Should be fixed now, please update!

@crvarner
Copy link
Author

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

@crvarner
Copy link
Author

now running [email protected] 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
Copy link
Author

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

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

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

indutny commented Jan 20, 2016

Looking, thank you for the information!

@indutny
Copy link
Collaborator

indutny commented Jan 20, 2016

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

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

indutny commented Jan 20, 2016

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

v4.1.0

@crvarner
Copy link
Author

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

@crvarner
Copy link
Author

After updating to v5.4.1 still no dice

@crvarner
Copy link
Author

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

@indutny
Copy link
Collaborator

indutny commented Jan 20, 2016

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

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

indutny commented Jan 20, 2016

Gosh, I know what's happening!

@indutny
Copy link
Collaborator

indutny commented Jan 20, 2016

Please try updating, just pushed out [email protected] . Hopefully it will help.

@indutny
Copy link
Collaborator

indutny commented Jan 20, 2016

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

@crvarner
Copy link
Author

[email protected] and same as before on FF. Requires a cleared cache to load.

@indutny
Copy link
Collaborator

indutny commented Jan 20, 2016

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

@indutny
Copy link
Collaborator

indutny commented Jan 22, 2016

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

bcherny commented Feb 21, 2016

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:

  • safari: 9.0.3
  • node: 4.2.2
  • spdy: 3.2.0
  • spdy-transport: 2.0.10 (same errors with 2.0.8 and 2.0.9)

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

No branches or pull requests

3 participants