expressjs / express

Fast, unopinionated, minimalist web framework for node.
https://expressjs.com
MIT License
65.15k stars 15.6k forks source link

serv-static stops when loading many images from iphone mobile safari. #2406

Closed omochi closed 9 years ago

omochi commented 9 years ago

I use serv-static and put one html and some images which will load from the html. I connect this page from my iphone through LAN. Mobile safari loading progress bar is stop and some image file is not loaded.

This issue doesn't occur in chrome on iOS, safari on Mac.

First I think this is safari issue and is not express issue. But after my many investigation (eliminating application specific logic and search reproducible occurring condition), I think this is latter.

I found that logic flow which close file reading stream before sending file completion. because of on-finish module implementation, if response.socket is null, it occur. I print log about response.socket in SendStream.stream() and in case image file not loaded, it was null.

I tried to modify this code section. and issue is solved.

I report detail of this issue at this repository. This can reproduce issue and run my patch. https://github.com/omochi/express-serv-static-issue

I think that this stackoverflow is related to this issue. http://stackoverflow.com/questions/26352839/linkedin-dustjs-on-node-express-ios-safari

dougwilson commented 9 years ago

Is there a way to reproduce without mobile Safari (or mail me an iPhone? :) )? Also, what version of Node.js is the server running on?

dougwilson commented 9 years ago

@omochi I'll mainly communicate with you through https://github.com/omochi/express-serv-static-issue/issues since we can make multiple threads.

dougwilson commented 9 years ago

I believe I understand what the issue is. I will test and make a patch to on-finished, which is where the root issue lies.

omochi commented 9 years ago

@dougwilson I haven't test other device yet on my issue report repo. But I met trouble first on production project with iPad Air. I'll report about other iOS devices in office tomorrow. I can also test with Android devices but it may not raise issue because Chrome in iOS was no problem.

Sure about communication on my report repo.

dougwilson commented 9 years ago

I'll have a fix for you to try very soon. I'll keep you posted.

dougwilson commented 9 years ago

P.S. I have no Apple or Android devices to use for testing :)

calvintwr commented 9 years ago

@omochi thanks for referencing the issue here.

@dougwilson this happens with the latest version of node 0.10.32

omochi commented 9 years ago

@calvintwr I hope that your case is same issue.

calvintwr commented 9 years ago

@omochi it is. check this out http://vogue-verve.com:3000

for me iPad was okay, but on the iPhone, the symptoms shows.

dougwilson commented 9 years ago

Thanks for the node version, though I already have it. The fix applies to all versions of node.

calvintwr commented 9 years ago

btw you got to clear the history and cache for it to happen.

dougwilson commented 9 years ago

We don't need anymore confirmation, please :)

omochi commented 9 years ago

I was surprised at issue is not in on-finish but in node.js. I'm glad I did report.

@calvintwr I connected from my iPhone and confirmed it. It's same about clear history and cache behavior.

dougwilson commented 9 years ago

The fix is being tracked in https://github.com/jshttp/on-finished/issues/10 , FYI. I believe it only affects clients that have pipelined requests that actually reach Node.js (so Node.js servers behind reverse proxies are not affected, which I think is why this hasn't come up before).

dougwilson commented 9 years ago

FYI for those who are watching here, @omochi has confirmed that the patch I have fixed the issue. I'll be releasing the fix tomorrow (US time, so ~8 hours) so I'll be available when people start using it, as it, in the end, is more than a simple patch, as the underlying library just didn't handle pipelined requests correctly and then a very subtle bug in Node.js 0.8 further complicated the patch :)

dougwilson commented 9 years ago

Once again, thank you very much for the detailed bug report. 3.18.1 is the 3.x version that contains the fix; the fix itself can be mostly had within 4.9.x with a new install or npm update on an existing install, which will bump the on-finished version in all but one module. Since a lot of the modules have moved on since 4.9.x, the complete update for this will be in 4.10.0 instead.

omochi commented 9 years ago

@dougwilson Issue is still remains in 4.9.8. I tested it in this branch (https://github.com/omochi/express-serv-static-issue/tree/check-express-4.9.8). Because of on-finished@2.1.0 referenced from send@0.9.3 referenced from express@4.9.8. output of $ npm list is here (https://github.com/omochi/express-serv-static-issue/blob/check-express-4.9.8/docs/npm-list.txt ). It may be what you said "all but one module". I understand that complete update will come in 4.10.0. I report it just in case. @calvintwr say same here (https://github.com/omochi/express-serv-static-issue/issues/3#issuecomment-60200616)

dougwilson commented 9 years ago

@omochi correct, it will exist in 4.9.8. If you can, if you can, a test with 3.18.1 would work too, as that should be all updated.

omochi commented 9 years ago

I tested 3.17.8, 3.18.0, 3.18.1. My index.js (https://github.com/omochi/express-serv-static-issue/blob/master/index.js) is run on 3.x without change.

I opened page over 10 times for each versions. Result:

Each npm list is here ( https://github.com/omochi/express-serv-static-issue/tree/check-express-3.17.8/docs )

3.18.0 case and 4.9.8 case including both on-finished 2.1.0 and on-finished 2.1.1 are suggest that on-finished used by serv-static is important.

omochi commented 9 years ago

I switched express version by follows.

$ rm -rf node_modules
$ npm install express@3.18.1
dougwilson commented 9 years ago

Yes, in the case of your simple app which does nothing but use express.static, the node_modules/express/node_modules/send/node_modules/on-finished version doesn't come into play.

3.18.x work since it contains a newer version of serve-static that pulled in the on-finished update.

Thank you for confirming 3.18.x, by the way :)

dougwilson commented 9 years ago

If you guys can, please verify if it's fixed in 4.10.0, please :)

omochi commented 9 years ago

I tested 4.10.0 in here (https://github.com/omochi/express-serv-static-issue/tree/check-express-4.10.0) and updated my job project. There are all ok. Thank you very much!

dougwilson commented 9 years ago

yay! Thank you so much for being responsive :) I'm going to close this issue now, as it has been solved in the latest 3.x and 4.x series :)

calvintwr commented 9 years ago

@dougwilson The "loading" issue is solved. Thanks for that.

However, I have a question regarding a possible edge case.

I am still experiencing problems with iOS Safari whenever there are static serves after the site has completed loading. For example:

if(cond) {
    img.onload = function() {
        //do something
    }
    img.src = example.jpg;
}

I tend to run into problems, specifically only on iOS when I have places that will do several of such operations. It does seems that when the image file is small, its not a problem. And the likelihood increases with size and number of requests.

@omochi would you have any advice on this?

dougwilson commented 9 years ago

@calvintwr I'm not 100% sure, as I don't do too much front end stuff. Do you have the same issue if you load the entire site through something else like nginx or Apache (I'm asking to see if it can be isolated to something in iOS browser, rather than on the back end)?

omochi commented 9 years ago

@calvintwr Sorry, I don'n know. I recommend you that create small reproducible project. How about fork from my this commit (https://github.com/omochi/express-serv-static-issue/tree/check-express-4.10.0) and rewrite this html ( https://github.com/omochi/express-serv-static-issue/blob/check-express-4.10.0/static/test.html). In this case image is loading from div/style/background-image. Cause of our case may be loading from img.src = on JS.

calvintwr commented 9 years ago

@dougwilson The problem is difficult to nail I can't be certain. This is a common technique used to change out a lower quality image for a higher one when an action is detected.

Before the fix it occurs almost certainly without nginx as proxy. The occurrence is greatly reduced when I put up ngxinx to. Only certain cases, seemingly when I trigger several such src switching within a shorter period of time.

@omochi Thanks for that. I will fork your repo to run tests. iOS Safari really is one delinquent child...

pepkin88 commented 9 years ago

I experienced a similar problem on iOS Safari, my solution was to turn off "keep-alive" socket option and to send "Connection: close" header for every request coming from iPhone. Check if it works for you.

calvintwr commented 9 years ago

@pepkin88 thanks for that. I'll keep that in mind. Now, with the upgraded express the problem seemed to have gone away.

dougwilson commented 9 years ago

@calvintwr thanks for the confirmation that I did indeed fix the issue :)