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

Unreleased semaphore blocks httprc indefinitely if a Transform panics #33

Open
arseniocosta1 opened this issue Sep 22, 2024 · 5 comments · May be fixed by #34
Open

Unreleased semaphore blocks httprc indefinitely if a Transform panics #33

arseniocosta1 opened this issue Sep 22, 2024 · 5 comments · May be fixed by #34

Comments

@arseniocosta1
Copy link

Description

The semaphore acquired here

httprc/cache.go

Line 155 in e71784b

e.acquireSem()
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 arseniocosta1 changed the title Unreleased semaphore can block httprc if a Transform panics Unreleased semaphore blocks httprc indefinitely if a Transform panics Sep 22, 2024
@arseniocosta1
Copy link
Author

I've open #33 which should fix the issue

@lestrrat
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Collaborator

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
Copy link
Author

arseniocosta1 commented Sep 24, 2024

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

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 a pull request may close this issue.

2 participants