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

Status code not set to 304 #25

Closed
colinf opened this issue Jan 27, 2012 · 8 comments
Closed

Status code not set to 304 #25

colinf opened this issue Jan 27, 2012 · 8 comments

Comments

@colinf
Copy link
Contributor

colinf commented Jan 27, 2012

I have been having a problem in that the http response status code is 200 rather than 304 for files which have not changed. It works for me if I make the following change in status-handler.js

exports['304'] = function (res, next) {
  res.writeHead(304, res.headers);
  res.end();
};

I don't really understand why and it could potentially need to be similarly changed in several other places in that file, so would rather see if anyone understands it before I submit a pull request.

Cheers,
Colin

@jfhbrook
Copy link
Owner

Interesting! I don't 100% understand either, but I'm glad you found this clue. If you feel like digging into it, the node.js api docs for the http module might mention the more subtle differences between res.writeHead and res.statusCode.

@jfhbrook
Copy link
Owner

That said, I would accept a pull request if everything seemed to be in (more or less) working order.

@colinf
Copy link
Contributor Author

colinf commented Jan 27, 2012

Ok, I'll try to take a look next week.

On 27 Jan 2012, at 17:41, Joshua Holbrookreply@reply.github.com wrote:

Interesting! I don't 100% understand either, but I'm glad you found this clue. If you feel like digging into it, the node.js api docs for the http module might mention the more subtle differences between res.writeHead and res.statusCode.


Reply to this email directly or view it on GitHub:
#25 (comment)

@colinf
Copy link
Contributor Author

colinf commented Feb 1, 2012

It seems to be related to the way that the Union module handles responses. If you take a look at this file in union

https://github.com/flatiron/union/blob/master/lib/response-stream.js

You will see that to set a response it actually uses this.response.statusCode. And if I change the code in status-handlers.js to be

res.response.statusCode = 304;

that seems to work.

But I think I prefer the res.writeHead() approach as it reads better. I'll make some changes along those lines and submit a pull request.

Cheers,
Colin

@jfhbrook
Copy link
Owner

jfhbrook commented Feb 4, 2012

Interesting. I wonder if you can define the setter for res.statusCode to proxy properly... I haven't tried to do anything quite so tricksy with javascript.

Either way, I approve of the writeHead approach.

@colinf
Copy link
Contributor Author

colinf commented Feb 4, 2012

I'm sure you're right and union could be amended to allow the use of re.statusCode directly. But anyway I have submitted a pull request on issue 27 to use writeHead. Thanks.

@jfhbrook
Copy link
Owner

I merged your pull request afaik, and I made corresponding changes in the v0.2.x branch.

Still, this is a bug with union imo. I'll file it. Thanks!

@nuarhu
Copy link

nuarhu commented Jul 27, 2012

Hi,

I'm using 0.1.6 via npm. I've first checked the code and found a different fix, then I found this issue. I'm not sure this is the same thing but in effect I would have created an issue with a similar subject line, so I rather comment on this one.

My test is: request a file, and request it again. Check the headers.

Expected:

  • The Request Header contains the "if-modified-since" tag
  • on renewed request the status code is 304

Actual Result

  • no "if-modified-since"
  • status code 200

My setup looks like this:

app.use(flatiron.plugins.http, {
    before: [
        function (req, res) {
            var found = app.router.dispatch(req, res);
            if (!found) {
                res.emit('next');
            }
        },
        ecstatic(__dirname + '/public', {
            autoIndex : false,
            cache: 60000 // or "60000, must-revalidate"
        })
    ],
    after: []
});

Wenn I change the ecstatic.js code (lines 86pp) to check the 'if-modified-since' header than the status code is returned as I would expect it.

    // Return a 304 if necessary
    if ( req.headers
      && ( (req.headers['if-none-match'] === etag(stat))
        || (Date.parse(req.headers['if-none-match']) >= stat.mtime )
        || (Date.parse(req.headers['if-modified-since']) <= stat.mtime)
      )
    ) {
    console.log("sending 304");
      status[304](res, next);
    }

When I comment the date comparison of 'if-modified-since' I get back 200 every time with Firefox, if I uncomment it, I get 304.

With Safari however, I always get 200: Safari does never send 'if-modified-since'. I controlled different pages and there this header is sent and 304 is returned, so in general it can work. I've tried adding the 'Expires' header but that did not help.

EDIT:
I've checked the 0.2 branch. The respective code is there in ecstatic/serve.js.
What is the reason for 'if-modified-since' not being checked when determining whether to return 304?

EDIT:
BTW: "if-none-match" can be a list of strings, as well. See http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html

Thanks,
Chantal

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

No branches or pull requests

3 participants