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-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
