-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement a basic HTTP memory cache #4117
Conversation
… (pre-redirect) URL and request headers and only stores GET responses.
…g against doomed cache enties.
…dated. Correct inverted expiry check in revalidation code.
…nt failures due to concurrent cache tests.
Critic review: https://critic.hoppipolla.co.uk/r/3296 This is an external review system which you may optionally use for the code review of your pull request. In order to help critic track your changes, please do not make in-place history rewrites (e.g. via |
//TODO: Last-Modified | ||
//TODO: Range requests | ||
//TODO: Revalidation rules for query strings | ||
//TODO: Vary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vary?
@jdm First pass of review finished—let me know what spec I should be looking at and I can review the logic. |
I've been going off of the old RFC 2616, but it looks like http://tools.ietf.org/html/rfc7234 and http://tools.ietf.org/html/rfc7232 are the new specs that should apply here. |
(also based on code inspection of nsHttpChannel.cpp in Gecko) |
@jdm OK, the logic looks good per those specs. |
I'll rebase this at some point. Until then, no need to keep this open. |
[WIP] Continue http cache work <!-- Please describe your changes on the following line: --> Work in progress, and not quite worth a review yet. (Continuation of #4117) TODO - [ ] cache metadata (find some subset of`net_traits::Metadata` that can be shared across threads, it seems the problem is mainly stuff inside `hyper::header` in the `headers` field) - [ ] determine which other fields of a `Response` need to be cached, so a full and valid one can be returned upon a cache hit. - [ ] determine how to best share the cache across fetch threads (inside HttpState like I tried now?) - [ ] Spend more time reading the spec and make sure the cache follows it where it matters. - [ ] Make the current wpt tests pass. - [ ] More... --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #12972 (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18676) <!-- Reviewable:end -->
[WIP] Continue http cache work <!-- Please describe your changes on the following line: --> Work in progress, and not quite worth a review yet. (Continuation of #4117) TODO - [ ] cache metadata (find some subset of`net_traits::Metadata` that can be shared across threads, it seems the problem is mainly stuff inside `hyper::header` in the `headers` field) - [ ] determine which other fields of a `Response` need to be cached, so a full and valid one can be returned upon a cache hit. - [ ] determine how to best share the cache across fetch threads (inside HttpState like I tried now?) - [ ] Spend more time reading the spec and make sure the cache follows it where it matters. - [ ] Make the current wpt tests pass. - [ ] More... --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #12972 (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18676) <!-- Reviewable:end -->
[WIP] Continue http cache work <!-- Please describe your changes on the following line: --> Work in progress, and not quite worth a review yet. (Continuation of #4117) TODO - [ ] cache metadata (find some subset of`net_traits::Metadata` that can be shared across threads, it seems the problem is mainly stuff inside `hyper::header` in the `headers` field) - [ ] determine which other fields of a `Response` need to be cached, so a full and valid one can be returned upon a cache hit. - [ ] determine how to best share the cache across fetch threads (inside HttpState like I tried now?) - [ ] Spend more time reading the spec and make sure the cache follows it where it matters. - [ ] Make the current wpt tests pass. - [ ] More... --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #12972 (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18676) <!-- Reviewable:end -->
[WIP] Continue http cache work <!-- Please describe your changes on the following line: --> Work in progress, and not quite worth a review yet. (Continuation of #4117) TODO - [ ] cache metadata (find some subset of`net_traits::Metadata` that can be shared across threads, it seems the problem is mainly stuff inside `hyper::header` in the `headers` field) - [ ] determine which other fields of a `Response` need to be cached, so a full and valid one can be returned upon a cache hit. - [ ] determine how to best share the cache across fetch threads (inside HttpState like I tried now?) - [ ] Spend more time reading the spec and make sure the cache follows it where it matters. - [ ] Make the current wpt tests pass. - [ ] More... --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #12972 (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18676) <!-- Reviewable:end -->
[WIP] Continue http cache work <!-- Please describe your changes on the following line: --> Work in progress, and not quite worth a review yet. (Continuation of #4117) TODO - [ ] cache metadata (find some subset of`net_traits::Metadata` that can be shared across threads, it seems the problem is mainly stuff inside `hyper::header` in the `headers` field) - [ ] determine which other fields of a `Response` need to be cached, so a full and valid one can be returned upon a cache hit. - [ ] determine how to best share the cache across fetch threads (inside HttpState like I tried now?) - [ ] Spend more time reading the spec and make sure the cache follows it where it matters. - [ ] Make the current wpt tests pass. - [ ] More... --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #12972 (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18676) <!-- Reviewable:end -->
Continue http cache work <!-- Please describe your changes on the following line: --> Work in progress, and not quite worth a review yet. (Continuation of #4117) TODO - [ ] cache metadata (find some subset of`net_traits::Metadata` that can be shared across threads, it seems the problem is mainly stuff inside `hyper::header` in the `headers` field) - [ ] determine which other fields of a `Response` need to be cached, so a full and valid one can be returned upon a cache hit. - [ ] determine how to best share the cache across fetch threads (inside HttpState like I tried now?) - [ ] Spend more time reading the spec and make sure the cache follows it where it matters. - [ ] Make the current wpt tests pass. - [ ] More... --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #12972 (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18676) <!-- Reviewable:end -->
Continue http cache work <!-- Please describe your changes on the following line: --> Work in progress, and not quite worth a review yet. (Continuation of #4117) TODO - [ ] cache metadata (find some subset of`net_traits::Metadata` that can be shared across threads, it seems the problem is mainly stuff inside `hyper::header` in the `headers` field) - [ ] determine which other fields of a `Response` need to be cached, so a full and valid one can be returned upon a cache hit. - [ ] determine how to best share the cache across fetch threads (inside HttpState like I tried now?) - [ ] Spend more time reading the spec and make sure the cache follows it where it matters. - [ ] Make the current wpt tests pass. - [ ] More... --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #12972 (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18676) <!-- Reviewable:end -->
…e_http_cache_work); r=jdm <!-- Please describe your changes on the following line: --> Work in progress, and not quite worth a review yet. (Continuation of servo/servo#4117) TODO - [ ] cache metadata (find some subset of`net_traits::Metadata` that can be shared across threads, it seems the problem is mainly stuff inside `hyper::header` in the `headers` field) - [ ] determine which other fields of a `Response` need to be cached, so a full and valid one can be returned upon a cache hit. - [ ] determine how to best share the cache across fetch threads (inside HttpState like I tried now?) - [ ] Spend more time reading the spec and make sure the cache follows it where it matters. - [ ] Make the current wpt tests pass. - [ ] More... --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #12972 (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> Source-Repo: https://github.com/servo/servo Source-Revision: e2bc0f017cfd24734777f64d8607b23dbe9552d7 --HG-- extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear extra : subtree_revision : 059fa41ef6c333ab9cc0adece4f431fec59a84c2
…e_http_cache_work); r=jdm <!-- Please describe your changes on the following line: --> Work in progress, and not quite worth a review yet. (Continuation of servo/servo#4117) TODO - [ ] cache metadata (find some subset of`net_traits::Metadata` that can be shared across threads, it seems the problem is mainly stuff inside `hyper::header` in the `headers` field) - [ ] determine which other fields of a `Response` need to be cached, so a full and valid one can be returned upon a cache hit. - [ ] determine how to best share the cache across fetch threads (inside HttpState like I tried now?) - [ ] Spend more time reading the spec and make sure the cache follows it where it matters. - [ ] Make the current wpt tests pass. - [ ] More... --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #12972 (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> Source-Repo: https://github.com/servo/servo Source-Revision: e2bc0f017cfd24734777f64d8607b23dbe9552d7
…e_http_cache_work); r=jdm <!-- Please describe your changes on the following line: --> Work in progress, and not quite worth a review yet. (Continuation of servo/servo#4117) TODO - [ ] cache metadata (find some subset of`net_traits::Metadata` that can be shared across threads, it seems the problem is mainly stuff inside `hyper::header` in the `headers` field) - [ ] determine which other fields of a `Response` need to be cached, so a full and valid one can be returned upon a cache hit. - [ ] determine how to best share the cache across fetch threads (inside HttpState like I tried now?) - [ ] Spend more time reading the spec and make sure the cache follows it where it matters. - [ ] Make the current wpt tests pass. - [ ] More... --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #12972 (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> Source-Repo: https://github.com/servo/servo Source-Revision: e2bc0f017cfd24734777f64d8607b23dbe9552d7
…e_http_cache_work); r=jdm <!-- Please describe your changes on the following line: --> Work in progress, and not quite worth a review yet. (Continuation of servo/servo#4117) TODO - [ ] cache metadata (find some subset of`net_traits::Metadata` that can be shared across threads, it seems the problem is mainly stuff inside `hyper::header` in the `headers` field) - [ ] determine which other fields of a `Response` need to be cached, so a full and valid one can be returned upon a cache hit. - [ ] determine how to best share the cache across fetch threads (inside HttpState like I tried now?) - [ ] Spend more time reading the spec and make sure the cache follows it where it matters. - [ ] Make the current wpt tests pass. - [ ] More... --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #12972 (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> Source-Repo: https://github.com/servo/servo Source-Revision: e2bc0f017cfd24734777f64d8607b23dbe9552d7
…e_http_cache_work); r=jdm <!-- Please describe your changes on the following line: --> Work in progress, and not quite worth a review yet. (Continuation of servo/servo#4117) TODO - [ ] cache metadata (find some subset of`net_traits::Metadata` that can be shared across threads, it seems the problem is mainly stuff inside `hyper::header` in the `headers` field) - [ ] determine which other fields of a `Response` need to be cached, so a full and valid one can be returned upon a cache hit. - [ ] determine how to best share the cache across fetch threads (inside HttpState like I tried now?) - [ ] Spend more time reading the spec and make sure the cache follows it where it matters. - [ ] Make the current wpt tests pass. - [ ] More... --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #12972 (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> Source-Repo: https://github.com/servo/servo Source-Revision: e2bc0f017cfd24734777f64d8607b23dbe9552d7 UltraBlame original commit: 8032f2843df37d05efa56b3e83a38b326789b8b2
…e_http_cache_work); r=jdm <!-- Please describe your changes on the following line: --> Work in progress, and not quite worth a review yet. (Continuation of servo/servo#4117) TODO - [ ] cache metadata (find some subset of`net_traits::Metadata` that can be shared across threads, it seems the problem is mainly stuff inside `hyper::header` in the `headers` field) - [ ] determine which other fields of a `Response` need to be cached, so a full and valid one can be returned upon a cache hit. - [ ] determine how to best share the cache across fetch threads (inside HttpState like I tried now?) - [ ] Spend more time reading the spec and make sure the cache follows it where it matters. - [ ] Make the current wpt tests pass. - [ ] More... --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #12972 (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> Source-Repo: https://github.com/servo/servo Source-Revision: e2bc0f017cfd24734777f64d8607b23dbe9552d7 UltraBlame original commit: 8032f2843df37d05efa56b3e83a38b326789b8b2
…e_http_cache_work); r=jdm <!-- Please describe your changes on the following line: --> Work in progress, and not quite worth a review yet. (Continuation of servo/servo#4117) TODO - [ ] cache metadata (find some subset of`net_traits::Metadata` that can be shared across threads, it seems the problem is mainly stuff inside `hyper::header` in the `headers` field) - [ ] determine which other fields of a `Response` need to be cached, so a full and valid one can be returned upon a cache hit. - [ ] determine how to best share the cache across fetch threads (inside HttpState like I tried now?) - [ ] Spend more time reading the spec and make sure the cache follows it where it matters. - [ ] Make the current wpt tests pass. - [ ] More... --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #12972 (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> Source-Repo: https://github.com/servo/servo Source-Revision: e2bc0f017cfd24734777f64d8607b23dbe9552d7 UltraBlame original commit: 8032f2843df37d05efa56b3e83a38b326789b8b2
High-level design:
This design handles multiple simultaneous requests for an in-progress cached response; each time a new portion of the body is stored, the list of awaiting consumers will be notified. It is also quite amenable to future changes to use asynchronous, push-based network operations.
Additionally, this changeset enables some really simple tests of caching behaviour by making all content tests run under a custom webserver that can track the number of full responses for a particular resource.