search

kevburnsjr / microcache

A non-standard HTTP cache implemented as Go middleware
MIT License
155 stars 5 forks source link

Don't spawn unnecessary goroutines for StaleWhileRevalidate #1

Closed erikdubbelboer closed 5 years ago

erikdubbelboer commented 5 years ago

Also moved racy tests into a separate file that doesn't get build when the race detector is enabled. These tests are racy because they don't use the mutexes in monitorFunc to read the values.

erikdubbelboer commented 5 years ago

I just rebased it on master so there is no more merge conflict.

kevburnsjr commented 5 years ago

@erikdubbelboer First of all, thanks for all the PRs. It's great to see participation on this repo :)
Sorry it took so long to respond, I just realized I wasn't following this repository XD

We had some problems with stale-while-revalidate in production at Crunchyroll. Background revalidation requests were panicking for unknown reasons (status 0 iirc), causing objects to become stuck in cache long past their ttl. We had to disable the feature and I never got around to tracking down the source of the issue. Your status 0 fix may be the solution.

This PR does seem more efficient, but I'm concerned the inline mutex might delay requests for hot stale objects accessed with high concurrency. It's just map RW so it's probably not a significant bottleneck and obviously spawning fewer goroutines for hot stale objects is the point of the optimization, but I'm curious:
What is the reason for this change?
Does it fix a race condition or is it purely a performance optimization?

kevburnsjr commented 5 years ago

I see the race condition in the failing test so I'm going to merge this.

erikdubbelboer commented 5 years ago

This was purely a performance optimization. Yes you are now locking revalidateMutex inside your main function instead of in a goroutine. But seeing as the only things happening during a revalidateMutex lock are getting and setting a map this shouldn't cause any slowdown at all. Starting the goroutine was a much bigger overhead.

The race condition in the tests is just because those tests aren't thread safe. They read and write to all the counters without any mutex/atomic. That's why I moved them to a file with // +build !race so they aren't run when the race detector is enabled.