[
https://issues.apache.org/jira/browse/HDDS-10210?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Ivan Andika updated HDDS-10210:
-------------------------------
Description:
While tracing the block deletion logic, I found a possible problem regarding
command status update logic.
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 it 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.
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 it properly inside StateContext.
> 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
>
> While tracing the block deletion logic, I found a possible problem regarding
> command status update logic.
> 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 it 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]
