expressjs / serve-static

Serve static files
MIT License
1.38k stars 227 forks source link

proposal: add static cache #94

Closed eljefedelrodeodeljefe closed 5 years ago

eljefedelrodeodeljefe commented 6 years ago

This is WIP since it's a code proposal.

This intends to get rid of the arguments "do not use Node for static file serving", as software like Nginx does cached reading itself, in order to not call the FS. I suggest having this because it's a very simple yet powerful addition.

This needs some more checking in the code and obviously tests.

da4b549 adds benchmarks 0d34b27 is the implementation

cc @dougwilson

this is also a prelude to my comment here https://github.com/expressjs/serve-static/issues/62#issuecomment-325443953

See a simple, yet not averaged out benchmark here.

hyper
eljefedelrodeodeljefe commented 6 years ago

Failing tests because of missing lang features on old Node versions. Easily fixable.

dougwilson commented 6 years ago

What are the arguments against using Node.js for file serving? Thr only thing I am aware of in that regard is that Node.js doesn't have async file I/O; instead all file operations are sync on a background thread pool that only has 4 threads and is shared by crypto and dns operations, so they can starve each other potentially, but I'm not sure if that actually happens in practice.

dougwilson commented 6 years ago

Anyway, there are issues with the actual implementation, though it is a WIP, so that's not an issue. Let me know what you're looking for out of this PR :)

eljefedelrodeodeljefe commented 6 years ago

The argument against Node comes from many guides that are perceived as best practice. E.g. the "use nginx for static files" topic. I would discourage this in teams, because it's another thing that might just break or can be misconfigured.

The biggest reason for using Node like this here are:

Some of the guides are even near to the repo, as in express or send

@dougwilson I am happy with any implementation and/or guidance in order to change the implementation above. It would be just very important to me to complement the ecosystem with such a thing, to ease the "use another technology" argument. If you think that is worth it I will work on it the coming week.

dougwilson commented 6 years ago

I think it is worth it, though as a separate module.

eljefedelrodeodeljefe commented 6 years ago

It probably needs some kind plugin architecture here to call the two functions for storing and reading out the cache.

Woud you take it into docs?

eljefedelrodeodeljefe commented 6 years ago

The plugin architecture here would imo be charming, because it would also cater for https://github.com/expressjs/serve-static/issues/62#issuecomment-325443953

dougwilson commented 6 years ago

If you would like to pursue landing into this module, I can take a better look at your PR here in the coming weeks. As for the plugin architecture, I'm not sure I understand; if it was separate, it would simply act as a replacement middleware to drop in instead of serve-serve static.

dougwilson commented 6 years ago

And yes, I'm happy to add anything to the readme, including alternatives and why someone would want to use it, etc.

eljefedelrodeodeljefe commented 6 years ago

A drop in replacement doesn't seem to valuable too me, because I don't want to promote modules, but rather enhance and help maintain an existing ecosystem. Imo, here it would okay having it, because it's opt-in, but you call the shots.

Re: plugin architecture: I was thinking of something like express.

serveStatic.use(require('serve-static-cache'))

Where serve-static-cache could expose two functions that will be called on hooks like beforeRead and beforeSend. Those would correspond to the two code insertions above.

Let's do it like that: I work on the implementation plus tests, because it's a good stab anyhow. I keep in mind that factoring into a module if scopes don't align and we will see afterwards.

If you have some time: Do you have any guidance on API and implementation that would make it more fitting to the surrounding code?

dougwilson commented 6 years ago

Yea, I'm still trying to visualize your idea, so need some time to think on it. If it helps, the main issue with the implementation here is that after the first response, all future responses are missing the headers like Content-Type and no longer honor If-Modified-Since etc. Also if the first request for a filr is a ranged request, the stream you'll save the data from will only be the requested slice of the file so all future requests will never get more than that first slice. Hopefully some of that will help in thinking through how the hooks will work.

eljefedelrodeodeljefe commented 5 years ago

Sorry for inactivity