search

fastify / send

Fork of the send module to deal with CVE-2017-20165
MIT License
13 stars 12 forks source link

Improve Etag performance by caching fs.stats #36

Open Uzlopak opened 1 year ago

Uzlopak commented 1 year ago

Prerequisites

🚀 Feature Proposal

Cache fs.stats result of files to improve performance. For this we could use an lru. If we know that the exposed files and folders are not changed we can store them without issues. But if we know that the files are getting modified it would make sense to use fs.watch and invalidate the lru. But it would be preferable to assume that e.g. fastify-static is running on a system were the files are not changed.

Motivation

I think the performance bottleneck of this library is the fs.stars call. By caching we could improve the performance.

Example

No response

climba03003 commented 1 year ago

I would wait for https://github.com/nodejs/node/pull/46345

I am thinking, are we open into race-condition of read file. Since, fs.stat and fs.createReadStream is called in different time. The stat data maybe out-dated during the two operation.

If true, would FileHandle helps in this case? Because a fileHandle always point to the same file descriptor for it's operation.

Uzlopak commented 1 year ago

Is a filehandle not created on every request then?

But you are right. It is kind of racy if the file is changing. Thats why I would suggest this for non changing filesystems. Like when you do wildcard: false in fastify-static. Maybe this has to be done in fastify-static?

climba03003 commented 1 year ago

Is a filehandle not created on every request then?

I expect it to be created on every request. Caching FileHandle or improper close would open in memory-leak. And I would expect FileHandle actually hold the File it open to prevent race-condition. FYI, the original send package support node@>= 0.8.0 and that's why it is not possible to use FileHandle. But, we can.

Maybe this has to be done in fastify-static?

Yes, for me. Caching is better to be done on higher level.

mcollina commented 1 year ago

I'm not sure what's the right level to put caching. I think we might want to do in this module, but that would require a significant amount of API changes.

serzero2007 commented 1 year ago

We can add stats argument to options to override fs.stats function in the new API here https://github.com/fastify/send/issues/15 By overriding stats function @Uzlopak will have ability to do whatever he wants with its calls. The same can be done for fs.createReadStream