fix: only lock for key generation

[aeneasr]

Closes #3865
Closes #3863

Related issue(s)

Checklist

• I have read the contributing guidelines.
• I have referenced an issue containing the design document if my change
  introduces a new feature.
• I am following the
  contributing code guidelines.
• I have read the security policy.
• I confirm that this pull request does not address a security
  vulnerability. If this pull request addresses a security vulnerability, I
  confirm that I got the approval (please contact
  [email protected]) from the maintainers to push
  the changes.
• I have added tests that prove my fix is effective or that my feature
  works.
• I have added or changed the documentation.

Further Comments

[awill1988]

I doubt this adds anything valuable but just in case, here's a couple ideas:

1. Use sync.Map to simplify the getLock(set string) *RWMutex
2. Wrap Persister.GetKeySet with a transaction

awill1988@b645b23#diff-cf3d2851cb06a9dfce1d62248d47c33494589220b8c2cca64c576a5d45b42594L167

[terev]

I doubt this adds anything valuable but just in case, here's a couple ideas:

1. Use `sync.Map` to simplify the `getLock(set string) *RWMutex`

2. Wrap `Persister.GetKeySet` with a transaction

awill1988@b645b23#diff-cf3d2851cb06a9dfce1d62248d47c33494589220b8c2cca64c576a5d45b42594L167

1. FWIW I agree this seems like a good idea. This use case fits one of their prescribed uses well.
2. Unless I'm missing something, I don't think the transaction would achieve anything there. Since it's a single read there's nothing to rollback.

[awill1988]

@terev for 2. the thought was by putting behind a db trx I imagined it could block if there's an insert happening on the table but that's making some broad assumptions about the level of trx isolation per db implementation and likely not the right solution.

I think @alnr had mentioned something along the lines of selecting for insert.. maybe putting the whole function in a db transaction would work?

[aeneasr]

FWIW I agree this seems like a good idea. This use case fits one of their prescribed uses well.

Atomic maps are also just maps with a sync mutex. Unfortunately sync.Map is not generic and I don't want to deal with type assertions. So this is fine IMO.

[terev]

@awill1988 I'm not 100% sure but I think trying to accomplish this in a single transaction will result in deadlocks. At least for the default isolation level.

The sync.Map is more sophisticated than just a map protected by a mutex. But since reading from the map isn't on the hot path it seems reasonable to just use a mutex. I noticed we're not using the read lock when accessing the map so maybe we just use a plain mutex?

[aeneasr]

I think @alnr had mentioned something along the lines of selecting for insert.. maybe putting the whole function in a db transaction would work?

He did, but doing INSERT ON CONFLICT on every JWK read is (I would assume) significantly more expensive than just reading from a read only transaction, as we have to acquire (potentially) a write lock. I really don't think this is a good solution as this functino is being quite a lot (which is why we're getting lock contention in the first place).

[alnr]

I'm not suggesting an insert on every read. Just when the select returns no rows.

[aeneasr]

Hm, not sure why conformity tests are failing. Maybe we generate too many keys now and it confuses the tests.