You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@roller.apache.org by "Kohei Nozaki (JIRA)" <ji...@apache.org> on 2016/01/11 03:44:39 UTC
[jira] [Comment Edited] (ROL-2101) Thread unsafe use of HashMap for
cached mappings exist
[ https://issues.apache.org/jira/browse/ROL-2101?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15091349#comment-15091349 ]
Kohei Nozaki edited comment on ROL-2101 at 1/11/16 2:43 AM:
------------------------------------------------------------
I'm planning to replace the unsafe {{HashMap}} s by {{Hashtable}} as {{JPAWeblogManagerImpl}} or more recent thread safe ones ( {{ConcurrentHashMap}} or synchronized wrappers ) .
Also, I think there are some room for improvement here, And I have a question:
{code:title=JPAWeblogEntryManagerImpl#getWeblogEntryByAnchor()|borderStyle=solid}
// check cache first
// NOTE: if we ever allow changing anchors then this needs updating
if(this.entryAnchorToIdMap.containsKey(mappingKey)) {
WeblogEntry entry = this.getWeblogEntry(this.entryAnchorToIdMap.get(mappingKey));
if(entry != null) {
LOG.debug("entryAnchorToIdMap CACHE HIT - " + mappingKey);
return entry;
} else {
// mapping hit with lookup miss? mapping must be old, remove it
this.entryAnchorToIdMap.remove(mappingKey);
}
}
{code}
Why this cannot be written as follows?
{code}
WeblogEntry e = this.getWeblogEntry(this.entryAnchorToIdMap.get(mappingKey));
if(e != null) {
LOG.debug("entryAnchorToIdMap CACHE HIT - " + mappingKey);
return e;
}
{code}
Is there posibillity of any case that {{mappingKey}} is available but value is set to null?
was (Author: xkylex):
I'm planning to replace the unsafe {{HashMap}} s by {{Hashtable}} as the above or more recent thread safe ones ( {{ConcurrentHashMap}} or synchronized wrappers ) .
Also, I think there are some room for improvement here, And I have a question:
{code:title=JPAWeblogEntryManagerImpl#getWeblogEntryByAnchor()|borderStyle=solid}
// check cache first
// NOTE: if we ever allow changing anchors then this needs updating
if(this.entryAnchorToIdMap.containsKey(mappingKey)) {
WeblogEntry entry = this.getWeblogEntry(this.entryAnchorToIdMap.get(mappingKey));
if(entry != null) {
LOG.debug("entryAnchorToIdMap CACHE HIT - " + mappingKey);
return entry;
} else {
// mapping hit with lookup miss? mapping must be old, remove it
this.entryAnchorToIdMap.remove(mappingKey);
}
}
{code}
Why this cannot be written as follows?
{code}
WeblogEntry e = this.getWeblogEntry(this.entryAnchorToIdMap.get(mappingKey));
if(e != null) {
LOG.debug("entryAnchorToIdMap CACHE HIT - " + mappingKey);
return e;
}
{code}
Is there posibillity of any case that {{mappingKey}} is available but value is set to null?
> Thread unsafe use of HashMap for cached mappings exist
> ------------------------------------------------------
>
> Key: ROL-2101
> URL: https://issues.apache.org/jira/browse/ROL-2101
> Project: Apache Roller
> Issue Type: Bug
> Components: Data Model & JPA Backend
> Affects Versions: 5.1.2
> Reporter: Kohei Nozaki
> Assignee: Roller Unassigned
> Priority: Minor
>
> There are two singleton classes that have a instance field type of {{HashMap}} for cache.
> In {{JPAUserManagerImpl}}:
> {noformat}
> // cached mapping of userNames -> userIds
> private Map<String, String> userNameToIdMap = new HashMap<String, String>();
> {noformat}
> In {{JPAWeblogEntryManagerImpl}}:
> {noformat}
> // cached mapping of entryAnchors -> entryIds
> private Map<String, String> entryAnchorToIdMap = new HashMap<String, String>();
> {noformat}
> {{HashMap}} s should not be used as the above because it's not thread safe.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)