You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2022/04/05 17:45:31 UTC
[GitHub] [geode] jinmeiliao opened a new pull request, #7556: GEODE-10097: Avoid Thread.sleep for re-auth in MessageDispatcher
jinmeiliao opened a new pull request, #7556:
URL: https://github.com/apache/geode/pull/7556
This is to revert the revert. The failure in the tests are due to the implementation of the specific security manager and a small race condition. This PR fixes that race condition.
--
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
[GitHub] [geode] pivotal-jbarrett commented on a diff in pull request #7556: GEODE-10097: Avoid Thread.sleep for re-auth in MessageDispatcher
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on code in PR #7556:
URL: https://github.com/apache/geode/pull/7556#discussion_r845338192
##########
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/MessageDispatcher.java:
##########
@@ -548,6 +529,48 @@ protected void runDispatcher() {
}
}
+ private boolean handleAuthenticationExpiredException(AuthenticationExpiredException expired)
+ throws InterruptedException {
+ long reAuthenticateWaitTime =
+ getSystemProperty(RE_AUTHENTICATE_WAIT_TIME, DEFAULT_RE_AUTHENTICATE_WAIT_TIME);
+ 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 notification to happen.
+ waitForReAuthenticationStartTime = System.currentTimeMillis();
+ subjectUpdated = false;
+ // 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.
+ long waitFinishTime = waitForReAuthenticationStartTime + reAuthenticateWaitTime;
+ long remainingWaitTime = waitFinishTime - System.currentTimeMillis();
+ while (!subjectUpdated && remainingWaitTime > 0) {
+ reAuthenticationLock.wait(remainingWaitTime);
+ remainingWaitTime = waitFinishTime - System.currentTimeMillis();
+ }
+ }
+ // the above wait timed out
+ if (!subjectUpdated) {
+ long elapsedTime = System.currentTimeMillis() - waitForReAuthenticationStartTime;
+ // reset the timer here since we are no longer waiting for re-auth to happen anymore
+ waitForReAuthenticationStartTime = -1;
Review Comment:
Sorry, confused it with other time variables.
--
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
[GitHub] [geode] pivotal-jbarrett commented on a diff in pull request #7556: GEODE-10097: Avoid Thread.sleep for re-auth in MessageDispatcher
Posted by 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
[GitHub] [geode] demery-pivotal commented on a diff in pull request #7556: GEODE-10097: Avoid Thread.sleep for re-auth in MessageDispatcher
Posted by GitBox <gi...@apache.org>.
demery-pivotal commented on code in PR #7556:
URL: https://github.com/apache/geode/pull/7556#discussion_r845408507
##########
geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/MessageDispatcherTest.java:
##########
@@ -173,6 +174,32 @@ public void oldClientWillNotGetClientReAuthenticateMessage() throws Exception {
verify(dispatcher, never()).dispatchResidualMessages();
}
+
+ @Test
+ public void oldClientWillContinueToDeliverMessageIfNotified() throws Exception {
+ doReturn(false, false, true).when(dispatcher).isStopped();
+ // make sure wait time is short
+ doReturn(10000L).when(dispatcher).getSystemProperty(eq(RE_AUTHENTICATE_WAIT_TIME), anyLong());
+ doThrow(AuthenticationExpiredException.class).when(dispatcher).dispatchMessage(any());
+ when(messageQueue.peek()).thenReturn(message);
+ when(proxy.getVersion()).thenReturn(KnownVersion.GEODE_1_14_0);
+
+ Thread dispatcherThread = new Thread(() -> dispatcher.runDispatcher());
+ Thread notifyThread = new Thread(() -> dispatcher.notifyReAuthentication());
+
+ dispatcherThread.start();
+ await().until(() -> dispatcher.isWaitingForReAuthentication());
+ notifyThread.start();
+
+ dispatcherThread.join();
+ notifyThread.join();
Review Comment:
A well-behaved rule can work in JUnit 5. Here's how:
Add `testImplementation('org.junit.jupiter:junit-jupiter-migrationsupport')` to the `build.gradle` file.
Add `@EnableRuleMigrationSupport` to the test class.
This works as long as the rule doesn't need to be on the call stack. `ExecutorServiceRule` probably doesn't need to be on the call stack.
--
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
[GitHub] [geode] mhansonp commented on a diff in pull request #7556: GEODE-10097: Avoid Thread.sleep for re-auth in MessageDispatcher
Posted by GitBox <gi...@apache.org>.
mhansonp commented on code in PR #7556:
URL: https://github.com/apache/geode/pull/7556#discussion_r843370226
##########
geode-core/src/distributedTest/java/org/apache/geode/security/AuthExpirationDUnitTest.java:
##########
@@ -66,6 +67,12 @@
private ClientVM clientVM;
+ @Before
Review Comment:
It looks like you made some junit5 changes elsewhere. Might be good to do it here.
--
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
[GitHub] [geode] jinmeiliao commented on a diff in pull request #7556: GEODE-10097: Avoid Thread.sleep for re-auth in MessageDispatcher
Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on code in PR #7556:
URL: https://github.com/apache/geode/pull/7556#discussion_r845456095
##########
geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/MessageDispatcherTest.java:
##########
@@ -173,6 +174,32 @@ public void oldClientWillNotGetClientReAuthenticateMessage() throws Exception {
verify(dispatcher, never()).dispatchResidualMessages();
}
+
+ @Test
+ public void oldClientWillContinueToDeliverMessageIfNotified() throws Exception {
+ doReturn(false, false, true).when(dispatcher).isStopped();
+ // make sure wait time is short
+ doReturn(10000L).when(dispatcher).getSystemProperty(eq(RE_AUTHENTICATE_WAIT_TIME), anyLong());
+ doThrow(AuthenticationExpiredException.class).when(dispatcher).dispatchMessage(any());
+ when(messageQueue.peek()).thenReturn(message);
+ when(proxy.getVersion()).thenReturn(KnownVersion.GEODE_1_14_0);
+
+ Thread dispatcherThread = new Thread(() -> dispatcher.runDispatcher());
+ Thread notifyThread = new Thread(() -> dispatcher.notifyReAuthentication());
+
+ dispatcherThread.start();
+ await().until(() -> dispatcher.isWaitingForReAuthentication());
+ notifyThread.start();
+
+ dispatcherThread.join();
+ notifyThread.join();
Review Comment:
Yeah, if it's junit4, I would use that. Not sure how to use that in Junit5.
--
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
[GitHub] [geode] jinmeiliao commented on a diff in pull request #7556: GEODE-10097: Avoid Thread.sleep for re-auth in MessageDispatcher
Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on code in PR #7556:
URL: https://github.com/apache/geode/pull/7556#discussion_r844173260
##########
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:
I already have unit tests covering this method from public accessors for both timeout and notified scenarios.
--
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
[GitHub] [geode] pivotal-jbarrett commented on a diff in pull request #7556: GEODE-10097: Avoid Thread.sleep for re-auth in MessageDispatcher
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on code in PR #7556:
URL: https://github.com/apache/geode/pull/7556#discussion_r843309640
##########
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:
When would we ever mutate the subject and not want to notify any waiters?
--
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
[GitHub] [geode] pivotal-jbarrett commented on a diff in pull request #7556: GEODE-10097: Avoid Thread.sleep for re-auth in MessageDispatcher
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on code in PR #7556:
URL: https://github.com/apache/geode/pull/7556#discussion_r843311029
##########
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/MessageDispatcher.java:
##########
@@ -193,6 +194,15 @@ public boolean isWaitingForReAuthentication() {
return waitForReAuthenticationStartTime > 0;
}
+ private volatile boolean subjectUpdated = false;
Review Comment:
`volatile` is redundant since all access is performed under `synchronized` block.
--
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
[GitHub] [geode] jinmeiliao commented on a diff in pull request #7556: GEODE-10097: Avoid Thread.sleep for re-auth in MessageDispatcher
Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on code in PR #7556:
URL: https://github.com/apache/geode/pull/7556#discussion_r844170579
##########
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:
the `setSubject` is also called when registering a new client
--
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
[GitHub] [geode] pivotal-jbarrett commented on a diff in pull request #7556: GEODE-10097: Avoid Thread.sleep for re-auth in MessageDispatcher
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on code in PR #7556:
URL: https://github.com/apache/geode/pull/7556#discussion_r843132689
##########
geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/MessageDispatcherTest.java:
##########
@@ -137,15 +137,15 @@ public void normalRunWillDispatchMessage() throws Exception {
@Test
Review Comment:
Update test to JUnit 5.
--
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
[GitHub] [geode] jinmeiliao commented on a diff in pull request #7556: GEODE-10097: Avoid Thread.sleep for re-auth in MessageDispatcher
Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on code in PR #7556:
URL: https://github.com/apache/geode/pull/7556#discussion_r843285783
##########
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:
since they use different lock, I will keep it in different methods.
##########
geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/MessageDispatcherTest.java:
##########
@@ -137,15 +137,15 @@ public void normalRunWillDispatchMessage() throws Exception {
@Test
Review Comment:
done
--
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
[GitHub] [geode] kirklund commented on a diff in pull request #7556: GEODE-10097: Avoid Thread.sleep for re-auth in MessageDispatcher
Posted by GitBox <gi...@apache.org>.
kirklund commented on code in PR #7556:
URL: https://github.com/apache/geode/pull/7556#discussion_r845393205
##########
geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/MessageDispatcherTest.java:
##########
@@ -173,6 +174,32 @@ public void oldClientWillNotGetClientReAuthenticateMessage() throws Exception {
verify(dispatcher, never()).dispatchResidualMessages();
}
+
+ @Test
+ public void oldClientWillContinueToDeliverMessageIfNotified() throws Exception {
+ doReturn(false, false, true).when(dispatcher).isStopped();
+ // make sure wait time is short
+ doReturn(10000L).when(dispatcher).getSystemProperty(eq(RE_AUTHENTICATE_WAIT_TIME), anyLong());
+ doThrow(AuthenticationExpiredException.class).when(dispatcher).dispatchMessage(any());
+ when(messageQueue.peek()).thenReturn(message);
+ when(proxy.getVersion()).thenReturn(KnownVersion.GEODE_1_14_0);
+
+ Thread dispatcherThread = new Thread(() -> dispatcher.runDispatcher());
+ Thread notifyThread = new Thread(() -> dispatcher.notifyReAuthentication());
+
+ dispatcherThread.start();
+ await().until(() -> dispatcher.isWaitingForReAuthentication());
+ notifyThread.start();
+
+ dispatcherThread.join();
+ notifyThread.join();
Review Comment:
Take a look at the `ExecutorServiceRule`. It can simplify a lot of this sort of testing and will kill and dump hung threads at the end of the test to make debugging easier if there is a failure of some sort.
--
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
[GitHub] [geode] jinmeiliao merged pull request #7556: GEODE-10097: Avoid Thread.sleep for re-auth in MessageDispatcher
Posted by GitBox <gi...@apache.org>.
jinmeiliao merged PR #7556:
URL: https://github.com/apache/geode/pull/7556
--
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
[GitHub] [geode] pivotal-jbarrett commented on a diff in pull request #7556: GEODE-10097: Avoid Thread.sleep for re-auth in MessageDispatcher
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on code in PR #7556:
URL: https://github.com/apache/geode/pull/7556#discussion_r843309312
##########
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:
Package private methods are acceptable for testing methods in isolation of the larger system.
--
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
[GitHub] [geode] jinmeiliao commented on a diff in pull request #7556: GEODE-10097: Avoid Thread.sleep for re-auth in MessageDispatcher
Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on code in PR #7556:
URL: https://github.com/apache/geode/pull/7556#discussion_r843285517
##########
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:
Done, it does feel right to initialize the boolean up front, even though the scenario you mentioned won't happen since it's guarded by a lock.
--
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
[GitHub] [geode] jinmeiliao commented on a diff in pull request #7556: GEODE-10097: Avoid Thread.sleep for re-auth in MessageDispatcher
Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on code in PR #7556:
URL: https://github.com/apache/geode/pull/7556#discussion_r843284906
##########
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:
I did move them into a separate method for readability, but I didn't make it accessible for testing, because I would like it to remain as a private method. I did add a test for it though.
--
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
[GitHub] [geode] pivotal-jbarrett commented on a diff in pull request #7556: GEODE-10097: Avoid Thread.sleep for re-auth in MessageDispatcher
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on code in PR #7556:
URL: https://github.com/apache/geode/pull/7556#discussion_r844456474
##########
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/MessageDispatcher.java:
##########
@@ -548,6 +529,48 @@ protected void runDispatcher() {
}
}
+ private boolean handleAuthenticationExpiredException(AuthenticationExpiredException expired)
+ throws InterruptedException {
+ long reAuthenticateWaitTime =
+ getSystemProperty(RE_AUTHENTICATE_WAIT_TIME, DEFAULT_RE_AUTHENTICATE_WAIT_TIME);
+ 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 notification to happen.
+ waitForReAuthenticationStartTime = System.currentTimeMillis();
+ subjectUpdated = false;
+ // 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.
+ long waitFinishTime = waitForReAuthenticationStartTime + reAuthenticateWaitTime;
+ long remainingWaitTime = waitFinishTime - System.currentTimeMillis();
+ while (!subjectUpdated && remainingWaitTime > 0) {
+ reAuthenticationLock.wait(remainingWaitTime);
+ remainingWaitTime = waitFinishTime - System.currentTimeMillis();
+ }
+ }
+ // the above wait timed out
+ if (!subjectUpdated) {
+ long elapsedTime = System.currentTimeMillis() - waitForReAuthenticationStartTime;
+ // reset the timer here since we are no longer waiting for re-auth to happen anymore
+ waitForReAuthenticationStartTime = -1;
Review Comment:
There is no use of this variable after this assignment.
--
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
[GitHub] [geode] demery-pivotal commented on a diff in pull request #7556: GEODE-10097: Avoid Thread.sleep for re-auth in MessageDispatcher
Posted by GitBox <gi...@apache.org>.
demery-pivotal commented on code in PR #7556:
URL: https://github.com/apache/geode/pull/7556#discussion_r845408507
##########
geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/MessageDispatcherTest.java:
##########
@@ -173,6 +174,32 @@ public void oldClientWillNotGetClientReAuthenticateMessage() throws Exception {
verify(dispatcher, never()).dispatchResidualMessages();
}
+
+ @Test
+ public void oldClientWillContinueToDeliverMessageIfNotified() throws Exception {
+ doReturn(false, false, true).when(dispatcher).isStopped();
+ // make sure wait time is short
+ doReturn(10000L).when(dispatcher).getSystemProperty(eq(RE_AUTHENTICATE_WAIT_TIME), anyLong());
+ doThrow(AuthenticationExpiredException.class).when(dispatcher).dispatchMessage(any());
+ when(messageQueue.peek()).thenReturn(message);
+ when(proxy.getVersion()).thenReturn(KnownVersion.GEODE_1_14_0);
+
+ Thread dispatcherThread = new Thread(() -> dispatcher.runDispatcher());
+ Thread notifyThread = new Thread(() -> dispatcher.notifyReAuthentication());
+
+ dispatcherThread.start();
+ await().until(() -> dispatcher.isWaitingForReAuthentication());
+ notifyThread.start();
+
+ dispatcherThread.join();
+ notifyThread.join();
Review Comment:
A well-behaved rule can work in JUnit 5. Here's how:
- Add `testImplementation('org.junit.jupiter:junit-jupiter-migrationsupport')` to the `build.gradle` file. (Whoever does this first will also have to add a version to `DependencyConstraings.groovy`.)
- Add `@EnableRuleMigrationSupport` to the test class.
This works as long as the rule doesn't need to be on the call stack. `ExecutorServiceRule` probably doesn't need to be on the call stack.
--
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
[GitHub] [geode] jinmeiliao commented on a diff in pull request #7556: GEODE-10097: Avoid Thread.sleep for re-auth in MessageDispatcher
Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on code in PR #7556:
URL: https://github.com/apache/geode/pull/7556#discussion_r845292857
##########
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/MessageDispatcher.java:
##########
@@ -548,6 +529,48 @@ protected void runDispatcher() {
}
}
+ private boolean handleAuthenticationExpiredException(AuthenticationExpiredException expired)
+ throws InterruptedException {
+ long reAuthenticateWaitTime =
+ getSystemProperty(RE_AUTHENTICATE_WAIT_TIME, DEFAULT_RE_AUTHENTICATE_WAIT_TIME);
+ 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 notification to happen.
+ waitForReAuthenticationStartTime = System.currentTimeMillis();
+ subjectUpdated = false;
+ // 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.
+ long waitFinishTime = waitForReAuthenticationStartTime + reAuthenticateWaitTime;
+ long remainingWaitTime = waitFinishTime - System.currentTimeMillis();
+ while (!subjectUpdated && remainingWaitTime > 0) {
+ reAuthenticationLock.wait(remainingWaitTime);
+ remainingWaitTime = waitFinishTime - System.currentTimeMillis();
+ }
+ }
+ // the above wait timed out
+ if (!subjectUpdated) {
+ long elapsedTime = System.currentTimeMillis() - waitForReAuthenticationStartTime;
+ // reset the timer here since we are no longer waiting for re-auth to happen anymore
+ waitForReAuthenticationStartTime = -1;
Review Comment:
Yes, there is, `isWaitingForReAuthentication()` is using it to determine if the dispatcher is waiting.
--
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