[
https://issues.apache.org/jira/browse/HDDS-10210?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Ivan Andika updated HDDS-10210:
-------------------------------
Description:
The cmdStatusMap in StateContext is a ConcurrentHashMap
Currently the update logic for command status is as follows:
{code:java}
public boolean updateCommandStatus(Long cmdId,
Consumer<CommandStatus> cmdStatusUpdater) {
if (cmdStatusMap.containsKey(cmdId)) {
cmdStatusUpdater.accept(cmdStatusMap.get(cmdId));
return true;
}
return false;
} {code}
It does not seem correct. Although the get containsKey and get is protected by
the ConcurrentHashMap, the CommandStatus retrieved is mutated directly using
the cmdStatusUpdater and not protected by the ConcurrentHashMap. This might
trigger race condition.
We should use atomic operation like computeIfPresent.
Note: The cmdStatusMap is also exposed to public (getCommandStatusMap) which
might not be a good idea since we cannot guarantee correct usage of the map. We
might need to encapsulate properly inside StateContext.
was:
The cmdStatusMap in StateContext is a ConcurrentHashMap
Currently the update logic for command status is as follows:
{code:java}
public boolean updateCommandStatus(Long cmdId,
Consumer<CommandStatus> cmdStatusUpdater) {
if (cmdStatusMap.containsKey(cmdId)) {
cmdStatusUpdater.accept(cmdStatusMap.get(cmdId));
return true;
}
return false;
} {code}
It does not seem correct. Although the get containsKey and get is protected by
the ConcurrentHashMap, the CommandStatus retrieved is mutated directly using
the cmdStatusUpdater and not protected by the ConcurrentHashMap. This might
trigger race condition.
We should use atomic operation like computeIfPresent.
> Ensure atomic update in StateContext#updateCommandStatus
> --------------------------------------------------------
>
> Key: HDDS-10210
> URL: https://issues.apache.org/jira/browse/HDDS-10210
> Project: Apache Ozone
> Issue Type: Improvement
> Components: DN, Ozone Datanode
> Affects Versions: 1.4.0
> Reporter: Ivan Andika
> Assignee: Ivan Andika
> Priority: Major
>
> The cmdStatusMap in StateContext is a ConcurrentHashMap
> Currently the update logic for command status is as follows:
> {code:java}
> public boolean updateCommandStatus(Long cmdId,
> Consumer<CommandStatus> cmdStatusUpdater) {
> if (cmdStatusMap.containsKey(cmdId)) {
> cmdStatusUpdater.accept(cmdStatusMap.get(cmdId));
> return true;
> }
> return false;
> } {code}
> It does not seem correct. Although the get containsKey and get is protected
> by the ConcurrentHashMap, the CommandStatus retrieved is mutated directly
> using the cmdStatusUpdater and not protected by the ConcurrentHashMap. This
> might trigger race condition.
> We should use atomic operation like computeIfPresent.
> Note: The cmdStatusMap is also exposed to public (getCommandStatusMap) which
> might not be a good idea since we cannot guarantee correct usage of the map.
> We might need to encapsulate properly inside StateContext.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
