Author: atm
Date: Mon May 19 19:59:08 2014
New Revision: 1596027
URL: http://svn.apache.org/r1596027
Log:
HADOOP-10489. UserGroupInformation#getTokens and UserGroupInformation#addToken
can lead to ConcurrentModificationException. Contributed by Robert Kanter.
Modified:
hadoop/common/branches/branch-2/hadoop-common-project/hadoop-common/CHANGES.txt
hadoop/common/branches/branch-2/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java
hadoop/common/branches/branch-2/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestUserGroupInformation.java
Modified:
hadoop/common/branches/branch-2/hadoop-common-project/hadoop-common/CHANGES.txt
URL:
http://svn.apache.org/viewvc/hadoop/common/branches/branch-2/hadoop-common-project/hadoop-common/CHANGES.txt?rev=1596027&r1=1596026&r2=1596027&view=diff
==============================================================================
---
hadoop/common/branches/branch-2/hadoop-common-project/hadoop-common/CHANGES.txt
(original)
+++
hadoop/common/branches/branch-2/hadoop-common-project/hadoop-common/CHANGES.txt
Mon May 19 19:59:08 2014
@@ -151,6 +151,9 @@ Release 2.5.0 - UNRELEASED
HADOOP-10401. ShellBasedUnixGroupsMapping#getGroups does not always return
primary group first (Akira AJISAKA via Colin Patrick McCabe)
+ HADOOP-10489. UserGroupInformation#getTokens and UserGroupInformation
+ #addToken can lead to ConcurrentModificationException (Robert Kanter via
atm)
+
Release 2.4.1 - UNRELEASED
INCOMPATIBLE CHANGES
Modified:
hadoop/common/branches/branch-2/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java
URL:
http://svn.apache.org/viewvc/hadoop/common/branches/branch-2/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java?rev=1596027&r1=1596026&r2=1596027&view=diff
==============================================================================
---
hadoop/common/branches/branch-2/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java
(original)
+++
hadoop/common/branches/branch-2/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java
Mon May 19 19:59:08 2014
@@ -1343,7 +1343,7 @@ public class UserGroupInformation {
* @param token Token to be added
* @return true on successful add of new token
*/
- public synchronized boolean addToken(Token<? extends TokenIdentifier> token)
{
+ public boolean addToken(Token<? extends TokenIdentifier> token) {
return (token != null) ? addToken(token.getService(), token) : false;
}
@@ -1354,10 +1354,11 @@ public class UserGroupInformation {
* @param token Token to be added
* @return true on successful add of new token
*/
- public synchronized boolean addToken(Text alias,
- Token<? extends TokenIdentifier> token)
{
- getCredentialsInternal().addToken(alias, token);
- return true;
+ public boolean addToken(Text alias, Token<? extends TokenIdentifier> token) {
+ synchronized (subject) {
+ getCredentialsInternal().addToken(alias, token);
+ return true;
+ }
}
/**
@@ -1365,10 +1366,11 @@ public class UserGroupInformation {
*
* @return an unmodifiable collection of tokens associated with user
*/
- public synchronized
- Collection<Token<? extends TokenIdentifier>> getTokens() {
- return Collections.unmodifiableCollection(
- new ArrayList<Token<?>>(getCredentialsInternal().getAllTokens()));
+ public Collection<Token<? extends TokenIdentifier>> getTokens() {
+ synchronized (subject) {
+ return Collections.unmodifiableCollection(
+ new ArrayList<Token<?>>(getCredentialsInternal().getAllTokens()));
+ }
}
/**
@@ -1376,23 +1378,27 @@ public class UserGroupInformation {
*
* @return Credentials of tokens associated with this user
*/
- public synchronized Credentials getCredentials() {
- Credentials creds = new Credentials(getCredentialsInternal());
- Iterator<Token<?>> iter = creds.getAllTokens().iterator();
- while (iter.hasNext()) {
- if (iter.next() instanceof Token.PrivateToken) {
- iter.remove();
+ public Credentials getCredentials() {
+ synchronized (subject) {
+ Credentials creds = new Credentials(getCredentialsInternal());
+ Iterator<Token<?>> iter = creds.getAllTokens().iterator();
+ while (iter.hasNext()) {
+ if (iter.next() instanceof Token.PrivateToken) {
+ iter.remove();
+ }
}
+ return creds;
}
- return creds;
}
/**
* Add the given Credentials to this user.
* @param credentials of tokens and secrets
*/
- public synchronized void addCredentials(Credentials credentials) {
- getCredentialsInternal().addAll(credentials);
+ public void addCredentials(Credentials credentials) {
+ synchronized (subject) {
+ getCredentialsInternal().addAll(credentials);
+ }
}
private synchronized Credentials getCredentialsInternal() {
Modified:
hadoop/common/branches/branch-2/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestUserGroupInformation.java
URL:
http://svn.apache.org/viewvc/hadoop/common/branches/branch-2/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestUserGroupInformation.java?rev=1596027&r1=1596026&r2=1596027&view=diff
==============================================================================
---
hadoop/common/branches/branch-2/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestUserGroupInformation.java
(original)
+++
hadoop/common/branches/branch-2/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestUserGroupInformation.java
Mon May 19 19:59:08 2014
@@ -35,6 +35,7 @@ import java.io.IOException;
import java.io.InputStreamReader;
import java.security.PrivilegedExceptionAction;
import java.util.Collection;
+import java.util.ConcurrentModificationException;
import java.util.LinkedHashSet;
import java.util.Set;
@@ -798,4 +799,69 @@ public class TestUserGroupInformation {
Collection<Token<? extends TokenIdentifier>> tokens =
ugi.getCredentials().getAllTokens();
assertEquals(1, tokens.size());
}
+
+ /**
+ * This test checks a race condition between getting and adding tokens for
+ * the current user. Calling UserGroupInformation.getCurrentUser() returns
+ * a new object each time, so simply making these methods synchronized was
not
+ * enough to prevent race conditions and causing a
+ * ConcurrentModificationException. These methods are synchronized on the
+ * Subject, which is the same object between UserGroupInformation instances.
+ * This test tries to cause a CME, by exposing the race condition.
Previously
+ * this test would fail every time; now it does not.
+ */
+ @Test
+ public void testTokenRaceCondition() throws Exception {
+ UserGroupInformation userGroupInfo =
+ UserGroupInformation.createUserForTesting(USER_NAME, GROUP_NAMES);
+ userGroupInfo.doAs(new PrivilegedExceptionAction<Void>(){
+ @Override
+ public Void run() throws Exception {
+ // make sure it is not the same as the login user because we use the
+ // same UGI object for every instantiation of the login user and you
+ // won't run into the race condition otherwise
+ assertNotEquals(UserGroupInformation.getLoginUser(),
+ UserGroupInformation.getCurrentUser());
+
+ GetTokenThread thread = new GetTokenThread();
+ try {
+ thread.start();
+ for (int i = 0; i < 100; i++) {
+ @SuppressWarnings("unchecked")
+ Token<? extends TokenIdentifier> t = mock(Token.class);
+ when(t.getService()).thenReturn(new Text("t" + i));
+ UserGroupInformation.getCurrentUser().addToken(t);
+ assertNull("ConcurrentModificationException encountered",
+ thread.cme);
+ }
+ } catch (ConcurrentModificationException cme) {
+ cme.printStackTrace();
+ fail("ConcurrentModificationException encountered");
+ } finally {
+ thread.runThread = false;
+ thread.join(5 * 1000);
+ }
+ return null;
+ }});
+ }
+
+ static class GetTokenThread extends Thread {
+ boolean runThread = true;
+ volatile ConcurrentModificationException cme = null;
+
+ @Override
+ public void run() {
+ while(runThread) {
+ try {
+ UserGroupInformation.getCurrentUser().getCredentials();
+ } catch (ConcurrentModificationException cme) {
+ this.cme = cme;
+ cme.printStackTrace();
+ runThread = false;
+ } catch (IOException ex) {
+ ex.printStackTrace();
+ }
+ }
+ }
+ }
}
