Fix 3xx responses from upstream

sillygod / cdp-cache

a caddy 2 proxy cache plugin

MIT License

119 stars 17 forks source link

Fix 3xx responses from upstream #34

Closed danlsgiga closed 1 year ago

danlsgiga commented 3 years ago

Close #33

Reply to the client with the correct response status code
Use a better, more specific defaultCacheKeyTemplate to exact matches on resources that are redirected.
The other changes are my VSCode editor removing white spaces
Changes how the file backend store the files by using sha256 hashes based on the entry keyWithRespectVary() content. This way, we don't waste storage space by creating duplicated temporary files every time the caddy process is restarted.
Implemented the Length() function for the file backend, since some browsers have grief when the Content-Length is set to 0 and abort the operation causing the http request to have an empty body
Fix some tests

fgilio commented 3 years ago

Hi @danlsgiga! After reading your issue I wonder, does this PR make caching work for 30x responses or does it completely disable catching on those?

danlsgiga commented 3 years ago

hi @fgilio, yeah... basically, 3xx responses are redirection only and what was happening is that I was getting 302's from the upstream but the cache was responding with a 200 and the http redirection never happened since the browser only reads the new Location: header if the response is 301 or 302.

danlsgiga commented 3 years ago

Also, I'm sorry for the failing tests... I started using the memory backend but soon I started to get huge memory usages, probably memory leaking too, and moved to the file backend and made a few adjustments there too.

sillygod commented 3 years ago

hi @fgilio, yeah... basically, 3xx responses are redirection only and what was happening is that I was getting 302's from the upstream but the cache was responding with a 200 and the http redirection never happened since the browser only reads the new Location: header if the response is 301 or 302.

Hi @danlsgiga ,

Thanks for your work. I've fixed this bug in another pull request so this pr need to be updated.

danlsgiga commented 3 years ago

@sillygod any chances we can get the other features I added merged?

- Changes how the file backend store the files by using sha256 hashes based on the entry keyWithRespectVary() content. This way, we don't waste storage space by creating duplicated temporary files every time the caddy process is restarted.
- Implemented the Length() function for the file backend, since some browsers have grief when the Content-Length is set to 0 and abort the operation causing the http request to have an empty body

sillygod commented 3 years ago

@sillygod any chances we can get the other features I added merged?

- Changes how the file backend store the files by using sha256 hashes based on the entry keyWithRespectVary() content. This way, we don't waste storage space by creating duplicated temporary files every time the caddy process is restarted.
- Implemented the Length() function for the file backend, since some browsers have grief when the Content-Length is set to 0 and abort the operation causing the http request to have an empty body

Sorry for replying so late, I'll check it today.

Furthermore, would you please fix the failed test? I've seen that the file backend's schema is changed so the relevant tests should be revised.