lestrrat-go / httprc

Quasi Up-to-date HTTP In-memory Cache
MIT License
17 stars 5 forks source link

Aquire semaphore when network will be accessed #25

Closed natenjoy closed 7 months ago

natenjoy commented 7 months ago

Potential fix for https://github.com/lestrrat-go/httprc/issues/24 . The full fix required not just acquiring the semaphore in cache.go more guardedly, but also not locking the event in queue.go until after the network request is made.

From a concurrency standpoint, the change to queue.go is safe because:

As a sanity check, I ran go test -race -count=1 ./... 100 times against this patch and the race detector did not find issues. The bug reproduction repo (https://github.com/natenjoy/httprc-cache-issue) contains instructions in how to run this patch against a minimal server setup.

I did see there is a v2 branch, but I still opted to open this PR against the main branch since latest tagged jwx package still uses v1.0.4. I was hoping if this change (eventually) looks mergeable that you could pull it over to the v2 branch.

codecov-commenter commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 63.42%. Comparing base (8a18d6d) to head (2d1b1b4). Report is 1 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #25 +/- ## ========================================== - Coverage 64.01% 63.42% -0.59% ========================================== Files 6 6 Lines 503 432 -71 ========================================== - Hits 322 274 -48 + Misses 168 145 -23 Partials 13 13 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

lestrrat commented 7 months ago

Ah I see. That makes sense. Thanks, but please wait a few more days for a release: I generally like to take a breathe and let the dust settle before going ahead with releases.

lestrrat commented 6 months ago

mental note: this does not apply to v2, as we do not have a worker pool