[ https://issues.apache.org/jira/browse/HDFS-16939?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17696193#comment-17696193 ]
ASF GitHub Bot commented on HDFS-16939: --------------------------------------- saxenapranav commented on code in PR #5450: URL: https://github.com/apache/hadoop/pull/5450#discussion_r1124593024 ########## hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/LowRedundancyBlocks.java: ########## @@ -369,7 +369,7 @@ synchronized boolean remove(BlockInfo block, * @return true if the block was found and removed from one of the priority * queues */ - boolean remove(BlockInfo block, int priLevel) { + synchronized boolean remove(BlockInfo block, int priLevel) { Review Comment: > > This will just ensure only one thread getting into the remove method. But what if there is one thread trying to remove, other trying to add or size(). > > add() and size() are already synchronized in the current code. > > 1. https://github.com/apache/hadoop/blob/01027e52a9789eb5b386729f52b7bb9e52fa5352/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/LowRedundancyBlocks.java#L290 > 2. https://github.com/apache/hadoop/blob/01027e52a9789eb5b386729f52b7bb9e52fa5352/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/LowRedundancyBlocks.java#L125 these method being synchronized means that only one thread can enter into the synchronized method, but doesn't make the object its working on synchronized. There could be one thread calling size which leads to `priorityQueues.get(i).size()`, and other calling add which leads to `priorityQueues.get(priLevel).add(blockInfo)` simultaneously. Example of probable issue is: for adding element https://github.com/apache/hadoop/blob/ccdb978603696a91c4828a66aad27f585543b376/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/util/LightWeightLinkedSet.java#L87-L125 is the code. let say thread goes till https://github.com/apache/hadoop/blob/ccdb978603696a91c4828a66aad27f585543b376/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/util/LightWeightLinkedSet.java#L101 and then context switch happens and the thread doesn't get CPU in future for some time. Now size has been incremented but addition has not happend. the thread calling size() gets the CPU, it will get size which is more than what exactly is in the set. Thats why feel that priorityQueues is still not thread-safe and hence suggesting the change. Please feel free to disagree. Thanks. > Fix the thread safety bug in LowRedundancyBlocks > ------------------------------------------------ > > Key: HDFS-16939 > URL: https://issues.apache.org/jira/browse/HDFS-16939 > Project: Hadoop HDFS > Issue Type: Bug > Components: namanode > Reporter: Shuyan Zhang > Assignee: Shuyan Zhang > Priority: Major > Labels: pull-request-available > > The remove method in LowRedundancyBlocks is not protected by synchronized. > This method is private and is called by BlockManager. As a result, > priorityQueues has the risk of being accessed concurrently by multiple > threads. -- This message was sent by Atlassian Jira (v8.20.10#820010) --------------------------------------------------------------------- To unsubscribe, e-mail: hdfs-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org