Replace Kemal::StaticFileHandler with direct subclass of stdlib HTTP::StaticFileHandler on Crystal < 1.17.0
#5338
+386
−6
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Kemal's subclass of the stdlib `HTTP::StaticFileHandler` is not as maintained as its parent, and so misses out on many enhancements and bug fixes from upstream, which unfortunately also includes the patches for security vulnerabilities... Though this isn't necessarily Kemal's fault since the bulk of the stdlib handler's logic was done in a single big method, making any changes hard to maintain. This was fixed in Crystal 1.17.0 where the handler was refactored into many private methods, making it easier for an inheriting type to implement custom behaviors while still leveraging much of the pre-existing code. Since we don't actually use any of the Kemal specific features added by `Kemal::StaticFileHandler`, there really isn't a reason to not just create a new handler based upon the stdlib implementation instead which will address the problems mentioned above. This PR implements a new handler which inherits from the stdlib variant and overrides the helper methods added in Crystal 1.17.0 to add the caching behavior with minimal code changes. Since this new handler depends on the code in Crystal 1.17.0, it will only be applied on versions greater than or equal to 1.17.0. On older versions we'll fallback to the current monkey patched `Kemal::StaticFileHandler`
Overriding `#call` or patching out `serve_file_compressed` provides only minimal benefits over the ease of maintenance granted by only overriding what we need to for the caching behavior.
Running `crystal spec` without a file argument essentially produces one big program that combines every single spec file, their imports, and the files that those imports themselves depend on. Most of the types within this combined program will get ignored by the compiler due to a lack of any calls to them from the spec files. But for some types, partially the HTTP module ones, using them within the spec files will suddenly make the compiler enable a bunch of previously ignored code. And those code will suddenly require the presence of additional types, constants, etc. This not only make it annoying for getting the specs working but also makes it difficult to isolate behaviors for testing. The `static_assets_handler_spec.cr` causes this issue and so will be marked as an isolated spec for now. In the future all of the tests should be organized into independent groupings similar to how the Crystal compiler splits their tests into std, compiler, primitives and interpreter.
Summing the sizes of each cached file every time is very inefficient. Instead we can simply store the cache size in an constant and increase it everytime a file is added into the cache.
syeopite
commented
Jun 4, 2025
| end | ||
| end | ||
|
|
||
| CACHE_LIMIT = 5_000_000 # 5MB |
There was a problem hiding this comment.
The CACHE_LIMIT is actually way too lenient imo. It actually allows you to fit everything from the assets folder since the entire non-compressed folder with the videojs dependencies is 4.9MB. And if you minified the videojs scripts, its only 2.7MB. Adding gzip compression to that can then get it down to around 840kB.
Adding some simple LRU cache and/or compressing the cached files might be something to consider.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes the CI for Crystal nightly
Kemal's subclass of the stdlib
HTTP::StaticFileHandleris not asmaintained as its parent, and so misses out on many enhancements and bug fixes from upstream, which unfortunately also includes the patches for security vulnerabilities...
Though this isn't necessarily Kemal's fault since the bulk of the stdlib handler's logic was done in a single big method, making any changes hard to maintain. This was fixed in Crystal 1.17.0 where the handler was refactored into many private methods, making it easier for an inheriting type to implement custom behaviors while still leveraging
much of the pre-existing code.
Since we don't actually use any of the Kemal specific features added by
Kemal::StaticFileHandler, there really isn't a reason to not just create a new handler based upon the stdlib implementation instead whichwill address the problems mentioned above.
This PR implements a new handler which inherits from the stdlib version and overrides the helper methods added in Crystal 1.17.0 to add the caching behavior with minimal code changes. Since this new handler depends on the code in Crystal 1.17.0, it will only be applied on versions greater than or equal to 1.17.0. On older versions we'll fallback to the current monkey patched
Kemal::StaticFileHandler