Use try-with-resources in DefStore #4480
Conversation
|
I think I need to delete the caches for that to work though don't I 🤔 |
| protected Hashtable<String, Long> mReqCounts = new Hashtable<>(); | ||
| protected Map<String, Long> mReqCounts = new HashMap<>(); |
There was a problem hiding this comment.
If this was a local variable it would be safe to convert a Hashtable into a Map since that variable was only used by a single thread. However, since it's a field in a class, and the class seems to be used for OCSP validation, so theoretically there could be multiple threads accessing the field concurrently, so I'm not quite sure if it's safe. What do you think?
There was a problem hiding this comment.
This particular Hashtable seems to be used to count the requests received by each CRLIssuingPointRecord. Since the number of CRLIssuingPointRecord itself probably doesn't change a lot, it might be safe to convert it into a Map.
There was a problem hiding this comment.
Hmm, good point. If we're doing validation/reading then perhaps it is sufficient to make the map volatile rather than going for a full blown synchronization? That way we ensure any writes to the map are done before any attempts to read it, but don't go as far as blocking threads from accessing the map. What do you think?
There was a problem hiding this comment.
There are several things to consider here:
- the
mReqCountsfield which contains a reference to aHashMapobject - the keys in the
HashMap - the values in the
HashMap
I'm not too familiar with volatile but based on what I read so far will it only apply to the mReqCounts field, but not to the keys/values in the map, so if the map is updated at the same time by multiple threads there is still a risk that the keys/values in the map (but not the field) might get corrupted.
As for the mReqCounts field itself, since the code does not update the value after the initialization there is actually no risk of field corruption.
As for the keys in the HashMap, as I said earlier most people probably will only have one key (i.e. the MasterCRL), so once that key is added in incReqCount() there will be no more changes to the hash table inside the HashMap, but it's possible that several OCSP clients might call the initial incReqCount() at the same time, so there's still a small risk for map key corruption.
As for the values in the HashMap, the process to increment the counter is not atomic, so there's also a (pretty big) risk of map value corruption, but I think this is actually an existing problem in the current code.
So for this PR I'd suggest to revert it back to Hashtable and merge it, then we can address the concurrency issue in a separate PR. We probably need to use a HashMap of AtomicLong.
| protected boolean mNotFoundGood = true; | ||
| protected boolean mUseCache = true; | ||
| protected boolean mByName = true; | ||
| protected boolean mIncludeNextUpdate = false; | ||
| protected Hashtable<String, CRLIPContainer> mCacheCRLIssuingPoints = new Hashtable<>(); | ||
| protected Map<String, CRLIPContainer> mCacheCRLIssuingPoints = new HashMap<>(); |
There was a problem hiding this comment.
This one is using the digest of the public key of the CA cert of a CRLIssuingPointRecord as a key. Assuming CRLIssuingPointRecord doesn't change a lot it might be safe too.
| private Hashtable<String, CRLIPContainer> mCache = null; | ||
| private Map<String, CRLIPContainer> mCache = null; |
There was a problem hiding this comment.
This one is pointing to mCacheCRLIssuingPoints, so it should be the same type.
|
Ideally we should populate the map during startup since there's only one thread doing the initialization. Once the service starts, multiple threads can access the map and maybe change the values in the map as well, but as long as they don't change the keys in the map it should be fine. Just something to think about. |
|
OK, I have reverted the type changes - I will merge once the CI backlog has cleared out a bit - thanks for the discussion all! |
|
/packit copr-build |
|
Kudos, SonarCloud Quality Gate passed! |
Also:
I am hoping that the CI will pull in test updates for
jboss-loggingand we can see if that package update integrates well. I figure if I'm going to do that, may as well do some tidying up while I'm at it.