Issue 52824: Don't mutate domains from the cache #6690
Conversation
…e types more similar
labkey-jeckels
left a comment
labkey-jeckels
left a comment
There was a problem hiding this comment.
Minor suggestions.
I wonder if we should do a followup PR and get rid of some or many of the getDomain() methods that don't take the forUpdate argument. Or perhaps keep it, but make it a final method that just passes through to the overloaded version. I've seen lots of cases where subclass overrides get inconsistent for overloaded methods with defaults.
| /** Get a schema that includes queries like Batch, Run, Results, and any additional tables. */ | ||
| AssayProtocolSchema createProtocolSchema(User user, Container container, @NotNull ExpProtocol protocol, @Nullable Container targetStudy); | ||
|
|
||
| Domain getBatchDomain(ExpProtocol protocol); |
There was a problem hiding this comment.
Worth a comment to say the default forUpdate is false?
| @Override | ||
| public Domain getDomain(boolean forUpdate) | ||
| { | ||
| return getDomain(); |
There was a problem hiding this comment.
Maybe this should throw UnsupportedOperationException when passed true?
| IPropertyType getType(Container container, String domainURI); | ||
|
|
||
| @Nullable | ||
| Domain getDomain(Container container, String domainURI); |
There was a problem hiding this comment.
Maybe comments here about the default value too?
| _domain = PropertyService.get().createDomain(getContainer(), getLSID(), getName()); | ||
| try | ||
| { | ||
| _domain.save(null); |
There was a problem hiding this comment.
Should this return an immutable copy if !forUpdate?
| { | ||
| String domainURI = getDomainURI(); | ||
| _domain = PropertyService.get().getDomain(getDomainContainer(), domainURI); | ||
| _domain = PropertyService.get().getDomain(getDomainContainer(), domainURI, forUpdate); |
There was a problem hiding this comment.
It seems wrong to stash a mutable copy and return it from getDomain(false)
There was a problem hiding this comment.
Yeah, it's a little unsavory, but currently also not really harmful. I was thinking to save the extra query, but I'm sure the unsavory part will be more bothersome in the future. Updated.
Rationale
Issue 52824 - We get occasional exceptions reported to Mothership with error messages like "java.lang.RuntimeException: Duplicate property descriptor name found for: c2d33_donor.SerologyInformation". The stack traces generally indicate they are coming from some background job (search indexer or ETL, for example). This appears to be happening because our methods for updating domains draw objects from the cache and then manipulate those cached objects before saving them. If some other thread extracts the domain from the cache while it is being manipulated (to rearrange the fields, for example) that other thread can get an invalid domain.
This PR adds an
isMutableproperty to domains, which is checked when a domain is saved and throws an exception if it is true. (Note that domains already have a different notion oflockedso that was not used here.) It would probably be best if we threw an exception from the setter methods for domains as well, but since domains are complex objects that contain other objects that could be mutated, that is not an entirely simple thing, and this PR doesn't go that far.There is the possibility that this change will be introducing some inefficiencies since we are deliberately bypassing the cache, but hope they won't be noticeable since we should be getting the immutable domain objects much more often than mutable ones.
Related Pull Requests
Changes
getDomain(boolean forUpdate)andgetDomainDescriptor(int id, boolean forUpdate)and many other similar overloading methods for getting domainsDomainclass withisMutableproperty