Lease monitor does not own the LeaseManager lock in changing leases.

• Key: HADOOP-4951
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Blocker
• Assignee: Tsz Wo (Nicholas), SZE
• Reporter: Tsz Wo (Nicholas), SZE
• Affects Version/s: 0.18.2
• Fix Version/s: 0.18.3

Description

In Monitor.checkLeases(), the monitor thread does not own the LeaseManager lock but it may modify the leases.

Comments

[Tsz Wo (Nicholas), SZE]

4951_20081229.patch: acquire the LeaseManager lock but not the Monitor lock

[Tsz Wo (Nicholas), SZE]

4951_20081229b.patch: acquire the lock in checkLeases()

[Raghu Angadi]

Fix looks fine. I think it is better to properly indent the code even if it means a bigger patch.

[Raghu Angadi]

or you could move checkLeases() to LeaseMonitor class (where it actually belongs).

[Tsz Wo (Nicholas), SZE]

> or you could move checkLeases() to LeaseMonitor class (where it actually belongs).

You are right. I should move it out.

4951_20081229c.patch: moved checkLeases() and synchronized toString().

[Raghu Angadi]

+1. Looks good.

[Tsz Wo (Nicholas), SZE]

     [exec] -1 overall.  
     [exec] 
     [exec]     +1 @author.  The patch does not contain any @author tags.
     [exec] 
     [exec]     -1 tests included.  The patch doesn't appear to include any new or modified tests.
     [exec]                         Please justify why no tests are needed for this patch.
     [exec] 
     [exec]     +1 javadoc.  The javadoc tool did not generate any warning messages.
     [exec] 
     [exec]     +1 javac.  The applied patch does not increase the total number of javac compiler warnings.
     [exec] 
     [exec]     +1 findbugs.  The patch does not introduce any new Findbugs warnings.
     [exec] 
     [exec]     +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

No new tests added since I don't know how to create a meaningful test for this.

[Tsz Wo (Nicholas), SZE]

4951_20081229c_0.18.patch: for 0.18

[Tsz Wo (Nicholas), SZE]

4951_20081229c_0.19.patch: for 0.19

[Tsz Wo (Nicholas), SZE]

Tested locally. Only TestMapReduceLocal failed but it is not related to this. See HADOOP-4907.

[Tsz Wo (Nicholas), SZE]

I just committed this.

[Koji Noguchi]

Nicholas, could you attach the stack trace you observed?
And what is the effect of this bug?

[Konstantin Shvachko]

This is the exception and the stack trace.

Exception in thread "org.apache.hadoop.dfs.LeaseManager$Monitor@3dbe8711"
java.util.ConcurrentModificationException
        at java.util.TreeMap$PrivateEntryIterator.nextEntry(TreeMap.java:1100)
        at java.util.TreeMap$KeyIterator.next(TreeMap.java:1154)
        at org.apache.hadoop.dfs.LeaseManager$Monitor.checkLeases(LeaseManager.java:370)
        at org.apache.hadoop.dfs.LeaseManager$Monitor.run(LeaseManager.java:346)
        at java.lang.Thread.run(Thread.java:619)

Another problem with this issue is that the name-node was running without lease monitor as if nothing happened. As a result, a lot of abandoned files have not been garbage collected.
This patch fixes the ConcurrentModificationException but does not fix the problem of NN running without the lease monitor. I'll file another jira for that.

[Tsz Wo (Nicholas), SZE]

> And what is the effect of this bug?

The LeaseManager lock is used to protect the lease related data structures. So the bug leads to a race condition on the leases.

[Konstantin Shvachko]

This did not fix the ConcurrentModificationException. Although it corrected the synchronization problem.
The exception now looks as

Exception in thread "org.apache.hadoop.dfs.LeaseManager$Monitor@2244d990" java.util.ConcurrentModificationException
        at java.util.TreeMap$PrivateEntryIterator.nextEntry(TreeMap.java:1100)
        at java.util.TreeMap$KeyIterator.next(TreeMap.java:1154)
        at org.apache.hadoop.dfs.LeaseManager.checkLeases(LeaseManager.java:371)
        at org.apache.hadoop.dfs.LeaseManager.access$800(LeaseManager.java:51)
        at org.apache.hadoop.dfs.LeaseManager$Monitor.run(LeaseManager.java:346)
        at java.lang.Thread.run(Thread.java:619)

Looks like the problem is that internalReleaseLease() finalizes empty files, which removes these file path names from the lease. So this modifies the Collection of file names which we are iterating on.
An additional patch need to be supplied.

[Konstantin Shvachko]

It will be easier to track changes if I open a new issue. Closing this one.