Skip to content
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

Fix network issue and use cache when network is down #109

Merged
merged 1 commit into from
May 29, 2024

Conversation

ykelvis
Copy link
Contributor

@ykelvis ykelvis commented May 16, 2024

Relate to #108

I think it's not only an issue with http_proxy. I can reproduce this error without http_proxy settings by plug/unplug my network cable (forcing a connection issue during pacoloco's network activity), same problem will show up.

After a little tinkering I think the problem is d is not ended by default. And I added f.cachedFileExists so that cache can be served. (Otherwise pacoloco would just return err without sending anything. https://github.com/anatol/pacoloco/blob/master/pacoloco.go#L270)

This has a little bit downside, it would not register a cacheServingFailedCounter if cache is sent. But I guess that would not be a fail after all?

@anatol
Copy link
Owner

anatol commented May 16, 2024

Thank you for tracking down this issue and providing the fix. The change looks good to me. But we also need a test for this error case. Something similar to tests here https://github.com/anatol/pacoloco/blob/master/integration_test.go

@anatol
Copy link
Owner

anatol commented May 29, 2024

It looks like implementing such test might not be a trivial task as I thought initially.

Let's move forward with the fix. Having an integration test for faulty network would still be beneficial.

@anatol anatol merged commit bb774b2 into anatol:master May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants