[GitHub] [geode] pivotal-jbarrett commented on a diff in pull request #7556: GEODE-10097: Avoid Thread.sleep for re-auth in MessageDispatcher

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

pivotal-jbarrett commented on code in PR #7556:
URL: https://github.com/apache/geode/pull/7556#discussion_r843128255


##########
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/MessageDispatcher.java:
##########
@@ -432,34 +442,42 @@ protected void runDispatcher() {
           _messageQueue.remove();
           clientMessage = null;
         } catch (AuthenticationExpiredException expired) {
-          if (waitForReAuthenticationStartTime == -1) {
+          synchronized (reAuthenticationLock) {
+            // turn on the "isWaitingForReAuthentication" flag before we send the re-auth message
+            // if we do it the other way around, the re-auth might be finished before we turn on the
+            // flag for the notify to happen.
             waitForReAuthenticationStartTime = System.currentTimeMillis();
             // only send the message to clients who can handle the message
             if (getProxy().getVersion().isNewerThanOrEqualTo(RE_AUTHENTICATION_START_VERSION)) {
               EventID eventId = createEventId();
               sendMessageDirectly(new ClientReAuthenticateMessage(eventId));
             }
+
             // We wait for all versions of clients to re-authenticate. For older clients we still
             // wait, just in case client will perform some operations to
             // trigger credential refresh on its own.
-            Thread.sleep(200);
-          } else {
+            long waitFinishTime = waitForReAuthenticationStartTime + reAuthenticateWaitTime;
+            subjectUpdated = false;

Review Comment:
   While the synchronization prevents a true race condition here, you are setting `subjectUpdated` to `false` after you have dispatched the message to the client. The client could respond between the dispatch and this setting. This should be set prior to dispatching the message.



##########
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnection.java:
##########
@@ -1231,6 +1231,7 @@ long putSubject(Subject subject, long existingUniqueId) {
       secureLogger.debug("update subject on client proxy {} with uniqueId {}", clientProxy,
           uniqueId);
       clientProxy.setSubject(subject);

Review Comment:
   Feels like `setSubject` should have the sufficient notification framework to inform our waiting thread.



##########
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/MessageDispatcher.java:
##########
@@ -432,34 +442,42 @@ protected void runDispatcher() {
           _messageQueue.remove();
           clientMessage = null;
         } catch (AuthenticationExpiredException expired) {
-          if (waitForReAuthenticationStartTime == -1) {
+          synchronized (reAuthenticationLock) {

Review Comment:
   This should all be moved to a method invoked by this catch block. The new method should be unit testable.



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

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