feat: add HTTP caching for PAC fetch using httpcache (RFC 9111 compli… - #171
feat: add HTTP caching for PAC fetch using httpcache (RFC 9111 compli…#171Sneha41sb wants to merge 3 commits into
Conversation
samuong
left a comment
samuong
left a comment
There was a problem hiding this comment.
Ensured cached PAC is not used as fallback when server is unreachable
It's not really clear to me how the httpcache package behaves when the server is unreachable - does it error out or does it return a response from the cache? To make sure this is behaving properly, I think we need to have a test that:
- sets up a test server (use a httptest.Server) to serve a pac file
- run the pac fetcher to download a file from it (and cache it)
- kill the test server
- rerun the pac fetcher, and check that it does the right thing
| "time" | ||
|
|
||
| "github.com/sandrolain/httpcache" | ||
| "github.com/sandrolain/httpcache/diskcache" |
There was a problem hiding this comment.
Using a disk cache opens up a whole can of worms that we could avoid if we just stick to an in-memory cache, let's stick with in-memory?
Also, I see a lot of files in the cache directory, do we need to check them in to git, or are they just a result of this disk cache getting accidentally added?
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. |
There was a problem hiding this comment.
I see a lot of comments, including this one, removed - unless you have a reason to remove them, can we keep them?
There was a problem hiding this comment.
I still see a lot of comments that have been removed by this PR, that include important information about why things were done a certain way. Are you able to add them back?
| cache/ | ||
| cache/ |
There was a problem hiding this comment.
i think we just need this once?
| cache/ | |
| cache/ | |
| cache/ |
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. |
There was a problem hiding this comment.
I still see a lot of comments that have been removed by this PR, that include important information about why things were done a certain way. Are you able to add them back?
| //modified time.Time | ||
| //fetched time.Time | ||
| //expiry time.Time | ||
| //etag string |
There was a problem hiding this comment.
This is good - this commented out code can stay removed, but we should maintain all the other comments that have been deleted.
| client = &http.Client{ | ||
| Timeout: 30 * time.Second, | ||
| Transport: cacheTransport, | ||
| } |
There was a problem hiding this comment.
I think it's better to leave the original client initialisation above the if-else statement, so we don't repeat the timeout twice:
client := &http.Client{Timeout: 30 * time.Second}
And then for the else branch, maybe something like this?
| } | |
| client.Transport := httpcache.NewTransport(httpcache.NewMemoryCache()) | |
| // The DefaultClient in net/http uses the proxy specified in the http(s)_proxy | |
| // environment variable, which could be pointing at this instance of alpaca. When | |
| // fetching the PAC file, we always use a client that goes directly to the server, | |
| // rather than via a proxy. | |
| client.Transport.Transport = &http.Transport{Proxy: nil} |
2 participants
Problem:
PAC file requests currently do not use HTTP caching.
Solution:
Impact: