Skip to content

Unreleased semaphore blocks httprc indefinitely if a Transform panics #33

Description

@arseniocosta1

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

Activity

  1. changed the title [-]Unreleased semaphore can block httprc if a Transform panics[/-] [+]Unreleased semaphore blocks httprc indefinitely if a Transform panics[/+] on Sep 22, 2024
  2. arseniocosta1 commented on Sep 22, 2024

    @arseniocosta1
    Author

    I've open #33 which should fix the issue

  3. lestrrat commented on Sep 23, 2024

    @lestrrat
    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.

  4. arseniocosta1 commented on Sep 23, 2024

    @arseniocosta1
    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?

  5. lestrrat commented on Sep 23, 2024

    @lestrrat
    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)

  6. arseniocosta1 commented on Sep 24, 2024

    @arseniocosta1
    Author

    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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions