ReportResult should be given the result if it is a success

[bseto]

according to the Limiter interface, successes will be given to ReportResult as err == nil.

In releaseConn it's given the err regardless of nil or non nil - expected
But with getConn, it is only given err if err != nil - not expected, this means we don't get success

I don't think this should be the behaviour because right before getConn is called, there's an Allow() call. I can't speak for everyone's use-case, but I'm setting Allow() to return true, and looking for a success call next - and this usually works - but sometimes the code gets stuck, and I think it's because in the case where Allow() returns true for a _getConn, if this function succeeds, ReportResult is not given the success

[jit-ci]

Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset.

In case there are security findings, they will be communicated to you as a comment inside the PR.

Hope you’ll enjoy using Jit.

Questions? Comments? Want to learn more? Get in touch with us.

[ndyakov]

hello @bseto , thank you for opening this PR.
@ofekshenawa would you be able to take a look at it?

[ofekshenawa]

@bseto Could you please add a small test case (perhaps using a mock limiter) to verify that ReportResult is now correctly called on success? Also It would also be good to briefly confirm that this change won't introduce significant overhead or locking contention for standard limiters, given it will now run on every operation

[bseto]

Sorry nevermind. I see that Allow() is only called once. So if it gets the connection, then the success there is ignored because the function still has to run, and then the result of that function will determine the final success/failure to ReportResult.