Add serve_files_by_extension

[yanganto]

No description provided.

[stephank]

(Oh, CI is failing because Hyper 1.0 was released hours ago. It's fixed if you merge with main.)

I'm not sure I understand what you're trying to do here. Do you have a directory with files all of the same type? In the doc comments, you show examples of serving HTML files, but the way it's implemented here, it becomes impossible to serve any other type of file, like CSS or images. Is that intended?

[yanganto]

Yes, I want to make a snapshot server.

All the files provided are .bin and should be nothing more, which is great because of security. Also, I want /snapshot/1 to get the 1.bin.

[stephank]

Ah, I see. I think as implemented here it's maybe a bit too specific for what I think the goal is for this library. I was thinking maybe a more generic rewrite functionality instead? Does something like #46 work for you?

[yanganto]

Hi @stephank,
Thank you for considering my case, and I am sorry to say it did not fulfill the requirement because /snapshot/0 will not get the file content of 0.bin.

This is not a website static file using case, but it is still pretty general in other file-specific file-sharing services as you can see in other services. For example. RGS, https://rapidsync.lightningdevkit.org

I can use rewrite and try_files of eginx to complete my work, but I believe hyper-staticfile can have a better performance by using set_extension in the bottom, that is pretty common so the standard lib of Rust provide it.

In the other view, if the hyper-staticfile tries to mimic eginx to do rewrite and try_files loses the performance, why does a user not just use egnix, which is much more mature?

I hope you can consider it and accept the PR, the function name in the PR may not always be proper, and it is appreciated to have a good review on it.

[stephank]

Hi @stephank, Thank you for considering my case, and I am sorry to say it did not fulfill the requirement because /snapshot/0 will not get the file content of 0.bin.

I'm not sure I follow. If you do something like this:

resolver.set_rewrite(|mut params| async move {
    params.path.set_extension("bin");
    Ok(params)
});

It should do exactly the same as what you were trying to accomplish, though I would recommend doing something slightly different:

resolver.set_rewrite(|mut params| async move {
    if !params.is_dir_request && params.path.extension() != Some("bin".as_ref()) {
        let mut path = params.path.into_os_string();
        path.push(".bin");
        params.path = path.into();
    }
    Ok(params)
});

That should also work for filenames with dots, e.g. my.cat.bin can be served as /my.cat. It also prevents serving your .bin files with other extensions, e.g. a request for /my.cat.html would only serve /my.cat.html.bin if it exists, and not /my.cat.bin.

This is not a website static file using case, but it is still pretty general in other file-specific file-sharing services as you can see in other services. For example. RGS, https://rapidsync.lightningdevkit.org

My reference for hyper_staticfile was npm send, and it does have an extensions option, though that is a bit more complex: https://www.npmjs.com/package/send#extensions

From what I understand, that does actual filesystem checks, so e.g. with options { root: "/docroot", extensions: ["bin"] } and a request /bla, it'd check the filesystem for /docroot/bla and /docroot/bla.bin. (I understand the first check is useless for you.)

But without those checks, and by simply doing set_extension(), I believe we create an API that is too surprising. From the example I gave above, a request /index.js would also unexpectedly serve /docroot/index.bin, and that might be dangerous. It would also break on filenames with dots, e.g. I can't request /docroot/my.cat.bin as /my.cat, because it'd try to find /docroot/my.bin.

(I would accept an implementation that looks like the extensions option in npm send, but from what I understand, that'd be worse for your performance concerns, because of the extra filesystem check.)

I can use rewrite and try_files of eginx to complete my work, but I believe hyper-staticfile can have a better performance by using set_extension in the bottom, that is pretty common so the standard lib of Rust provide it.

In the other view, if the hyper-staticfile tries to mimic eginx to do rewrite and try_files loses the performance, why does a user not just use egnix, which is much more mature?

#46 is not adding as much functionality or overhead as nginx rewrite or try_files. It's merely adding a function parameter that's called at a specific spot.

Performance before and after #46 looks the same to me. On an AWS EC2 m7i.large instance, benchmarking a release build against localhost with ab -c 50 -n 10000, I get 16000 requests per second either way.

I don't think you can compare PathBuf::set_extension to what we're doing here, because it is intended for trusted or validated input. I don't believe it is wise to offer API where we blindly apply it to user input.

[yanganto]

Thank you for doing a benchmark on this. 🙏

It is good to try the file index.bin when a user wants to get index.js in the case, this is an already known issue and also a good one when the user has any typo. The static file serving is in a file-specific manner. The reason I wanted to use hyper-staticfile at first is I wanted to avoid any kind of useless fancy routing things and let the server have a clean logic in an easily maintained manner, also good in with performance.

We already know the PathBuf::set_extension will have a great difference from any other rewrite solution, so you excluded it when doing the benchmark. It is appreciate your time and work on this if this PR is not acceptable here, please close it I will reconsider forking on this or just using Nginx. Thanks for your time again.

[stephank]

We already know the PathBuf::set_extension will have a great difference from any other rewrite solution, so you excluded it when doing the benchmark. It is appreciate your time and work on this if this PR is not acceptable here, please close it I will reconsider forking on this or just using Nginx. Thanks for your time again.

Sorry, I wasn't clear on what I actually benchmarked, but the benchmark with #46 did include a call to set_extension. It didn't seem to make a noticeable difference in the benchmark result for me.

I'll still merge #46, because I'm personally happy with it. But forking and using a git dependency is definitely something you can choose to do.

Thanks for discussing and contributing!