[GitHub] [hadoop] ghanko commented on a change in pull request #1894: HADOOP-16819 Possible inconsistent state of AbstractDelegationTokenSecretManager

[GitBox <gi...@apache.org>]

ghanko commented on a change in pull request #1894:
URL: https://github.com/apache/hadoop/pull/1894#discussion_r595392501



##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/AbstractDelegationTokenSecretManager.java
##########
@@ -356,16 +356,14 @@ private void updateCurrentKey() throws IOException {
     int newCurrentId;
     synchronized (this) {
       newCurrentId = incrementCurrentKeyId();
-    }
-    DelegationKey newKey = new DelegationKey(newCurrentId, System
-        .currentTimeMillis()
-        + keyUpdateInterval + tokenMaxLifetime, generateSecret());
-    //Log must be invoked outside the lock on 'this'
-    logUpdateMasterKey(newKey);
-    synchronized (this) {
-      currentKey = newKey;
+      currentKey = new DelegationKey(newCurrentId, System
+          .currentTimeMillis()
+          + keyUpdateInterval + tokenMaxLifetime, generateSecret());
+
       storeDelegationKey(currentKey);
     }
+    //Log must be invoked outside the lock on 'this'
+    logUpdateMasterKey(currentKey);

Review comment:
       Yes, but I'm not sure why it was placed between them. They have to be in the same synchronized block to be atomic, otherwise other threads may find that there's no key for the current id.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org