Default Project CR-8089

OPENAM-5542 - improve the caching of ServiceConfigImpl instances

Closed on 28 Sep 15

  • Author & Moderator
  • Reviewers
    • Reviewer completed
    • Reviewer completed

CR-8089 5

Keyboard shortcuts  
Summarize the review outcomes (optional)


Warning: no files are visible, they have all been filtered.
Participant Role Time Spent Comments Latest Comment
Author & Moderator 47m 2 Will change to: if (answer != null) { ...
Reviewer - 100% reviewed 2m    
Reviewer - Complete 13m 2 see also SMSEntry#save. if isNewEntry is true, then it us...
Reviewer - Complete 25m 1 I can't really see the logic of this bit of code. The 2 c...
Total   1h 27m 5  


When running OpenAM for any length of time, the number of ServiceConfigImpl instances grows beyond the number of cached entries.

The logic in the ServiceConfigImpl.getFromCache() call included a check of

   if (answer.smsEntry.isNewEntry()) {
        answer = null;

does not appear to provide any value (and from looking back in the history of the OpenSSO code, has no real reason for being). By just setting the answer to null and not cleaning up the cache or calling clear() leaves the old instance in memory due to listener references and you end up with a whole new instance of ServiceConfigImpl created.

It is not completely clear why smsEntry.isNewEntry() being true is a reason to not return a cached instance, an alternative to this change would be to include the same cleanup steps for the !answer.isValid() case and leave the if (answer.smsEntry.isNewEntry()) check in place.

If nothing else, it would be good to get opinions as to what this code was actually trying to do


Issues Raised From Comments

Key Summary State Assignee

General Comments

There are no general comments on this review.
/openam-core/src/.../sm/ Changed   5
Open in IDE #permalink

Review updated: Reload | Ignore | Collapse

You cannot reload the review while writing a comment.

Create Issue

Assign To Me

Log time against