search

lestrrat-go / httprc

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

Unreleased semaphore blocks httprc indefinitely if a Transform panics #33

Open arseniocosta1 opened 1 week ago

arseniocosta1 commented 1 week ago

Description

The semaphore acquired here https://github.com/lestrrat-go/httprc/blob/e71784b6c22397cb2a0cf2579a4b90083c797fc0/cache.go#L155 isn't released with a defer

If a Transform panics the semaphore is never released, and the next call to a Cache.Get will block indefinitely

arseniocosta1 commented 1 week ago

I've open #33 which should fix the issue

lestrrat commented 1 week ago

IIRC, that releaseSem is intentional: I wanted it to be released BEFORE the next lock happened in the same function happened, not when the function returned. I think if a Transoform could panic it would be better to protect the call to Transform itself, not the locks.

arseniocosta1 commented 1 week ago

I think it's troublesome that a Transform could block the cache

Since a Transform is intended to do things like parsing XML, RSS, and it can happen for one of those to panic

We run this lib in a REST/GQL service and I as a client did not expect that panic would block my GQL resolvers/REST handlers :D

I've already protected my Transform function, but I still think this behaviour should not be expected At the very least could we add a note in the README?

lestrrat commented 1 week ago

No, I'm not arguing against putting a fix, but I'm suggesting that you could protect the call to e.transform.Transform in fetchAndStore instead of mucking with the locks.

(As an side: I don't disagree with needing to handle error cases like this, but TBH if you have the leeway to do so, I'd reconsider using RSS/XML libraries that panic instead of returning errors)

arseniocosta1 commented 1 week ago

Ah, I misunderstood, I'll try to give it a go this weekend on protecting the Transform call itself (.transform.Transform) and leaving the semaphore alone