cachers,cmd/go-cacher-server: fix unexpected http 404s #18
Conversation
| if !validHex(actionID) { | ||
| return "" | ||
| } | ||
| return filepath.Join(dc.Dir, fmt.Sprintf("o-%s", outputID)) |
There was a problem hiding this comment.
The diff is rendering in a confusing way because it looks like this is for action ID now, but the bug was that this returned path was missing the component outputID[:2] in the middle.
| statusCode int | ||
| } | ||
|
|
||
| func (w *loggingResponseWriter) WriteHeader(statusCode int) { | ||
| w.statusCode = statusCode |
There was a problem hiding this comment.
What the real net/http responseWriter implementations do it as call WriteHeader(200) on the first Write call, and then track the first WriteHeader value (implicit or explicit) and never change from that if WriteHeader is called multiple times.
You could just use https://pkg.go.dev/net/http/httptest#ResponseRecorder too, really, as long as you avoid the NewRecord constructor and make the ResponseRecorder yourself with the Body field set to nil, which is documented as not capturing writes.
There was a problem hiding this comment.
I tried httptest.ResponseRecorder first, but I couldn't see an easy way to hold it like middleware that also writes through to an underlying http.ResponseWriter, so in either case I was going to need a new type definition. I guess I could keep the loggingResponseWriter and have it use httptest.ResponseRecorder to make the status code detection more robust if that's what you're suggesting?
bradfitz
left a comment
bradfitz
left a comment
There was a problem hiding this comment.
Could you add a test that failed before but passes now?
Running with a cold disk cache for go-cacher and a warm disk cache for go-cacher-server, I was getting no cache hits. This is due to a missing component in the output filename when serving up what go-cacher-server thinks is a cache hit; it was trying to serve from `<dir>/o-<hex>` instead of the real file location of `<dir>/<o2>/o-<hex>` where o2 is the first 2 characters of the hex string. To help with consistency, I consolidated all places that need to calculate an action or output file path into 2 shared functions, but there is only one functional change. Tested via the benchmarks I included in #17, but it's probably worth investing in some unit tests as well as we start to use this more seriously. Signed-off-by: Tom Proctor <tomhjp@users.noreply.github.com>
tomhjp
force-pushed
the
tomhjp/fix-http-misses
branch
from
c7b5106 to
db27c2c
Compare
|
Added a test that fails before and passes now. LMK if I was just holding |
Running with a cold disk cache for go-cacher and a warm disk cache for go-cacher-server, I was getting no cache hits. This is due to a missing component in the output filename when serving up what go-cacher-server thinks is a cache hit; it was trying to serve from
<dir>/o-<hex>instead of the real file location of<dir>/<o2>/o-<hex>where o2 is the first 2 characters of the hex string.To help with consistency, I consolidated all places that need to calculate an action or output file path into 2 shared functions, but there is only one functional change. Tested via the benchmarks I included in #17, but it's probably worth investing in some unit tests as well as we start to use this more seriously.