Skip to content

Commit

Permalink
Revert "Fix for blank page on Safari reload"
Browse files Browse the repository at this point in the history 
This reverts commit 3b27386.
  • Loading branch information
@tj
tj committed Feb 19, 2014
1 parent 6d20242 commit f2c79c2
Showing 1 changed file with 2 additions and 2 deletions.
4 changes: 2 additions & 2 deletions index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@ function fresh(req, res) {
// unconditional request
if (!modifiedSince && !noneMatch) return false;

// check for no-cache or max-age=0 cache request directive
if (cc && (cc.indexOf('no-cache') !== -1 || cc.indexOf('max-age=0') !== -1)) return false;
// check for no-cache cache request directive
if (cc && cc.indexOf('no-cache') !== -1) return false;

// parse if-none-match
if (noneMatch) noneMatch = noneMatch.split(/ *, */);
Expand Down

11 comments on commit f2c79c2

@ericf
Copy link

@ericf ericf commented on f2c79c2 Mar 3, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this reverted? There's no entry in the Changelog, and I'm seeing this as an issue on iOS.

@tj
Copy link
Contributor Author

@tj tj commented on f2c79c2 Mar 3, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's an issue open for it with some discussion, it was breaking end-to-end validation. I've never seen this issue personally but it seems like a bug in the browser, might have to find a less hacky way around it for now (or at least away that is not baked into express etc)

@ericf
Copy link

@ericf ericf commented on f2c79c2 Mar 3, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, do you have a link handy to the issue so I can follow it?
For now, I'll add something dynamic and always changing to the body so it gets a different etag each time.

@DrMoriarty
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello, guys! I have troubles with safari and 304 on reloading for all my sites.
Can I help to fix this problem in right way?
It's very annoying!

@eeid26
Copy link

@eeid26 eeid26 commented on f2c79c2 Mar 5, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This issue has been discussed here #8
Returning 200 always when the browser tries to validate the cache breaks the http caching for all browsers.
Returning 200 or 304 is following http standard.

This safari issue has not been explained by any one, except stating it does not work.

To be able to solve this issue more information is needed to understand what is wrong.
What are the requests and response when safari

  1. Has no cache. Clear the cache enter the resource url in the browser address bar press enter. (do not hit refresh). What are the requests and responses for that resource and the resources it reference. This is testing with no browser cache?
  2. Once the resource is rendered by the browser, refresh the page. This will force the browser to send requests to the server to validate the cache. What are requests and responses for the resources.

@DrMoriarty
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I've dumped req to console for the main page:
The first run was made with anonymous safari mode, the site looks ok, status 200. Log: https://gist.github.com/DrMoriarty/9383803#file-first_request-log

The second run: I pressed Cmd-R. The site looks ok again, status 200: https://gist.github.com/DrMoriarty/9383803#file-second_request-log

The third run: I pressed to the site logo (go to /) then again pressed Cmd-R. The site again looks ok, status 200: https://gist.github.com/DrMoriarty/9383803#file-third_request-log

Then I went to another page and then returned again to /. The site looks ok, status 304! Log: https://gist.github.com/DrMoriarty/9383803#file-fourth_request-log

Than I clicked several times on links and several times pressed Cmd-R. The site was ok.
After many tries I've got the blank screen: https://gist.github.com/DrMoriarty/9383803#file-tenth_request-log

In the log I saw max-age=0 several times. But all responses apart the last returned status 200 for it. Also there were several 304 responses, but for all of them (apart the last one) no max-age=0 was added to the request's headers.
So the trouble exists only if status 304 returns when max-age=0 is in the headers.

Let me know if you need more logs
Thank you

@eeid26
Copy link

@eeid26 eeid26 commented on f2c79c2 Mar 7, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at the logs and I can't see anything wrong with the responses. again, max-age=0 in the request is asking the browser to verify the cache.
I am not sure what is wrong with that safari, but I do not think the fix for it should be in node-fresh module.

@imyller
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Safari is complying to RFC-2616 when it sends Cache-Control: max-age=0. Safari requests end-to-end revalidation when reloading the page manually.

http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.9.3

Especially:
http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.9.4

Specific end-to-end revalidation
The request includes a "max-age=0" cache-control directive, which forces each cache along the path to the origin server to revalidate its own entry, if any, with the next cache or server. The initial request includes a cache-validating conditional with the client's current validator.

node-fresh module should react to max-age=0 by preferring fresh content instead of allowing 304 response.

@Dakuan
Copy link

@Dakuan Dakuan commented on f2c79c2 Aug 9, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of our users were being affected by this so we wrote a little bit of middleware to hack round it:

https://github.com/Dakuan/jumanji

@imyller
Copy link

@imyller imyller commented on f2c79c2 Aug 9, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, Dakuan. jumanji middleware offers a nice workaround for Safari caching issue.

However, I personally still think that Safari works according to RFC-2616 specification when sending max-age=0 cache-control header.

What Safari actually requests is an "unspecified end-to-end revalidation" defined in the spec. Node-fresh lacks support for this and only supports "end-to-end reload" (with no-cache):

Quote from the RFC-2616:

End-to-end revalidation may be requested either when the client does not have its own local cached copy, in which case we call it "unspecified end-to-end revalidation", or when the client does have a local cached copy, in which case we call it "specific end-to-end revalidation.

Unspecified end-to-end revalidation:
The request includes "max-age=0" cache-control directive, which forces each cache along the path to the origin server to revalidate its own entry, if any, with the next cache or server.

Proper solution would be to implement support for all RFC-2616 cache control directives to node-fresh.

@jonathanong
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR welcome! I'm not sure how to fix this but I'll look into and suggestions

Please sign in to comment.