Skip to content

option for static serve to check if-modified-since#994

Closed
avimar wants to merge 7 commits into
restify:4.xfrom
avimar:patch-1
Closed

option for static serve to check if-modified-since#994
avimar wants to merge 7 commits into
restify:4.xfrom
avimar:patch-1

Conversation

@avimar

@avimar avimar commented Jan 18, 2016

Copy link
Copy Markdown
Contributor

Mentioned in #993

@avimar

avimar commented Jan 29, 2016

Copy link
Copy Markdown
Contributor Author

Is there a reason why this build failed? It looks like a travis config issue.

@micahr

micahr commented Jan 29, 2016

Copy link
Copy Markdown
Contributor

Looks like an eslint config issue. I've updated master with the appropriate change.

@avimar avimar closed this Mar 13, 2016
@avimar avimar reopened this Mar 13, 2016
@avimar

avimar commented Mar 13, 2016

Copy link
Copy Markdown
Contributor Author

I fixed some lint issues... the tests for static don't work on windows, so I didn't try writing any tests.

@DonutEspresso

Copy link
Copy Markdown
Member

It might need to - saw something similar filed against ecstatic a little bit ago:
jfhbrook/node-ecstatic#179

@DonutEspresso

Copy link
Copy Markdown
Member

Thanks for the PR! Can you elaborate with the issue with windows, I'm not familiar.

@avimar

avimar commented Mar 17, 2016

Copy link
Copy Markdown
Contributor Author

Date is still an issue in node v4.3.0...

but if you do (windows) curl "http://localhost:8000/bower.json" -H "Connection: keep-alive" -H "If-Modified-Since: 275760-10-13 (my static test file)
You get: {"code":"InternalError","message":"illegal access"}" but it doesn't crash the node process since restify handles exceptions.

So - if someone sends a super malformed request, it fails. (It doesn't show up in my logs..) Does that matter?

@DonutEspresso

Copy link
Copy Markdown
Member

In 5.x the uncaughtException handlers are going to be off by default, so yes, this would crash the server.

@avimar

avimar commented Mar 23, 2016

Copy link
Copy Markdown
Contributor Author

I had borrowed some of the code from here which will then also also crash:
https://github.com/restify/node-restify/blob/4.x/lib/plugins/conditional_request.js#L122
if (Date.parse(mtime) <= Date.parse(ctime)) {

but later, https://github.com/restify/node-restify/blob/4.x/lib/plugins/conditional_request.js#L162 is wrapped in a try/catch, I guess that's what we have to do in both these places?

This was pushed upstream in v8 to not throw an exception, but current versions of node still throw.

@DonutEspresso

Copy link
Copy Markdown
Member

Yeah, I think it's reasonable to wrap in try/catch like in the conditional request plugin. Thanks for digging into it!

@koszny

koszny commented Mar 30, 2016

Copy link
Copy Markdown

is it known when it can be expected ot be merged?

@DonutEspresso

Copy link
Copy Markdown
Member

I think once @avimar gets a chance to add the try/catch in there I'd be happy to merge this in.

That said, I think we'd like if you could file this PR against the restify/plugins repo instead. For our next 5.x release, all major plugins will be removed from core.

@DonutEspresso

DonutEspresso commented May 13, 2016

Copy link
Copy Markdown
Member

Closing in favor of restify/plugins#23

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

Successfully merging this pull request may close these issues.

4 participants