Set cache control in the HTTP-RPC response header by JayT106 · Pull Request #6265 · tendermint/tendermint

JayT106 · 2021-03-23T03:41:25Z

This PR added the "cache-control" into the HTTP-RPC response header and set the cache expired time to one year

yihuang · 2021-03-23T03:52:32Z

rpc/jsonrpc/server/http_server.go

 		return fmt.Errorf("json marshal: %w", err)
 	}
 	w.Header().Set("Content-Type", "application/json")
+	w.Header().Set("Cache-Control", "max-age=31536000") // expired after one year


I think there's some endpoints are not supposed to be cached, for example /status, the response will change according to current status.

Yeah I think this is advisable. We should instead consider adding a cache policy on individual requests. Also, one year seems too large, no?

@alexanderbez Agree with the cache policy that might need to be added. @yihuang, I will check the RPCs and add constrain to them.

@alexanderbez Was my intuition, however, found a link to explain why set the age to one year if we see the data is immutable (but varnish cache doesn't support it)
https://ashton.codes/set-cache-control-max-age-1-year/

Absolutely. If the response data does not change, then this makes sense 👍

codecov · 2021-03-23T03:54:38Z

Codecov Report

Merging #6265 (dcee6f1) into master (b517dd5) will decrease coverage by 0.09%.
The diff coverage is 60.00%.

@@            Coverage Diff             @@
##           master    #6265      +/-   ##
==========================================
- Coverage   60.90%   60.80%   -0.10%     
==========================================
  Files         282      282              
  Lines       26866    26885      +19     
==========================================
- Hits        16362    16347      -15     
- Misses       8804     8835      +31     
- Partials     1700     1703       +3

Impacted Files	Coverage Δ
rpc/core/routes.go	`0.00% <0.00%> (ø)`
rpc/jsonrpc/server/http_uri_handler.go	`32.47% <0.00%> (ø)`
light/client.go	`73.24% <33.33%> (ø)`
rpc/jsonrpc/server/http_server.go	`64.03% <66.66%> (+0.64%)`	⬆️
rpc/jsonrpc/server/http_json_handler.go	`61.48% <70.58%> (+1.63%)`	⬆️
rpc/jsonrpc/server/rpc_func.go	`93.54% <100.00%> (+0.21%)`	⬆️
blockchain/v0/pool.go	`74.52% <0.00%> (-4.57%)`	⬇️
consensus/reactor.go	`68.57% <0.00%> (-2.95%)`	⬇️
p2p/transport_memory.go	`83.78% <0.00%> (-2.71%)`	⬇️
blockchain/v0/reactor.go	`63.20% <0.00%> (-1.60%)`	⬇️
... and 11 more

JayT106 · 2021-03-23T15:50:13Z

@alexanderbez are these routes files not allowed to modify?

alexanderbez · 2021-03-23T18:48:44Z

Not sure I follow @JayT106? What do you mean exactly? Can they override the HTTP header?

JayT106 · 2021-03-24T00:49:09Z

Not sure I follow @JayT106? What do you mean exactly? Can they override the HTTP header?
@alexanderbez
My bad, I didn't solve the file conflict (and rebase correctly) in the PR.

JayT106 · 2021-03-24T01:45:56Z

golangci-lint 2 errors related to the unused code. Can I just remove it?
::error file=light/proxy/routes.go,line=162,col=6::makeBlockSearchFunc is unused (deadcode)
::error file=light/proxy/routes.go,line=154,col=6::type rpcBlockSearchFunc is unused (unused)

light/proxy/routes.go

cmwaters · 2021-04-13T12:12:39Z

rpc/jsonrpc/server/http_json_handler.go

+		// Set the default response cache to true unless we see any error and any rpc request doesn't
+		// allow to be cached
+		var c = true


There is one other condition that we have to be wary about. For a lot of RPC requests i.e validators, consensus params, blocks and commits, if a client requests this data with a height of 0 the node responds by sending that data type at the latest height. This means that the same request of height 0 to these RPC endpoints should also over time send different responses and thus shouldn't cache the response

That is correct!

Thanks for the feedback, I will check the condition and revise the logic.

Awesome. Once this is done I think the PR should be ready to merge

Added default height checking for these types of RPC requests.

cmwaters

Awesome work @JayT106 🚀

JayT106 requested review from alexanderbez, ebuchman and tessr as code owners March 23, 2021 03:41

JayT106 force-pushed the http-cache branch from bccf0c0 to 7acd983 Compare March 23, 2021 03:48

yihuang reviewed Mar 23, 2021

View reviewed changes

JayT106 requested a review from cmwaters as a code owner March 23, 2021 15:37

JayT106 force-pushed the http-cache branch from 96e41d1 to 958a6ef Compare March 24, 2021 01:04

JayT106 force-pushed the http-cache branch from 958a6ef to 78ee21f Compare March 24, 2021 01:50

cmwaters reviewed Mar 24, 2021

View reviewed changes

light/proxy/routes.go Show resolved Hide resolved

JayT106 force-pushed the http-cache branch 5 times, most recently from 1edecb2 to 1feb651 Compare March 24, 2021 15:35

JayT106 force-pushed the http-cache branch from 1feb651 to d9c71ab Compare April 1, 2021 02:34

JayT106 requested a review from tychoish as a code owner April 1, 2021 02:34

JayT106 force-pushed the http-cache branch from d9c71ab to 2b3ddd6 Compare April 5, 2021 20:04

cmwaters reviewed Apr 13, 2021

View reviewed changes

JayT106 added 3 commits April 13, 2021 16:01

Set cache control in the HTTP-RPC response header

ad21a0f

Add a simply cache policy to the RPC routes

d451d85

add a condition to check the RPC request has default height settings

60831f2

JayT106 force-pushed the http-cache branch from 2b3ddd6 to 60831f2 Compare April 13, 2021 20:25

add changelog

e70d035

JayT106 requested a review from tac0turtle as a code owner April 14, 2021 08:05

Merge branch 'master' into http-cache

dcee6f1

cmwaters approved these changes Apr 14, 2021

View reviewed changes

cmwaters merged commit ca7dbea into tendermint:master Apr 14, 2021

tychoish pushed a commit that referenced this pull request Apr 14, 2021

Set cache control in the HTTP-RPC response header (#6265)

be48474

JayT106 mentioned this pull request Sep 16, 2022

rpc: Set cache control in the HTTP-RPC response header #9442

Closed

3 tasks

Bro-Chain added a commit to Bro-Chain/tendermint that referenced this pull request Oct 13, 2022

Added back CacheControl headers from PR tendermint#6265

9eb0dc0

Bro-Chain mentioned this pull request Oct 13, 2022

Added back CacheControl headers from PR #6265 #9549

Closed

3 tasks

thanethomson mentioned this pull request Oct 31, 2022

rpc: Add caching support #9650

Merged

3 tasks

Conversation

JayT106 commented Mar 23, 2021

Uh oh!

yihuang Mar 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Mar 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

JayT106 commented Mar 23, 2021

Uh oh!

alexanderbez commented Mar 23, 2021

Uh oh!

JayT106 commented Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JayT106 commented Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmwaters left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yihuang Mar 23, 2021 •

edited

Loading

codecov bot commented Mar 23, 2021 •

edited

Loading

JayT106 commented Mar 24, 2021 •

edited

Loading

JayT106 commented Mar 24, 2021 •

edited

Loading