agingade commented on a change in pull request #6835:
URL: https://github.com/apache/geode/pull/6835#discussion_r711248759
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/MessageDispatcher.java
##########
@@ -362,31 +387,67 @@ protected void runDispatcher() {
}
waitForResumption();
}
- try {
- clientMessage = (ClientMessage) _messageQueue.peek();
- } catch (RegionDestroyedException skipped) {
- break;
+
+ // if message is not delivered due to authentication expiation, this
clientMessage
+ // would not be null.
+ if (clientMessage == null) {
+ try {
+ clientMessage = (ClientMessage) _messageQueue.peek();
+ } catch (RegionDestroyedException skipped) {
+ break;
+ }
}
+
getStatistics().setQueueSize(_messageQueue.size());
if (isStopped()) {
break;
}
- if (clientMessage != null) {
- // Process the message
- long start = getStatistics().startTime();
- //// BUGFIX for BUG#38206 and BUG#37791
- boolean isDispatched = dispatchMessage(clientMessage);
- getStatistics().endMessage(start);
- if (isDispatched) {
+
+ if (clientMessage == null) {
+ _messageQueue.remove();
+ continue;
+ }
+
+ // Process the message
+ long start = getStatistics().startTime();
+ try {
+ if (dispatchMessage(clientMessage)) {
+ getStatistics().endMessage(start);
_messageQueue.remove();
if (clientMessage instanceof ClientMarkerMessageImpl) {
getProxy().setMarkerEnqueued(false);
}
}
- } else {
+ clientMessage = null;
+ wait_for_re_auth_start_time = -1;
+ } catch (NotAuthorizedException notAuthorized) {
+ // behave as if the message is dispatched, remove from the queue
+ logger.info("skip delivering message: " + clientMessage,
notAuthorized);
_messageQueue.remove();
+ clientMessage = null;
+ } catch (AuthenticationExpiredException expired) {
+ if (wait_for_re_auth_start_time == -1) {
+ wait_for_re_auth_start_time = 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));
+ }
+ // for older client, we still wait, just in case client will
perform some operations to
+ // trigger credential refresh on its own.
+ Thread.sleep(200);
+ } else {
+ long elapsedTime = System.currentTimeMillis() -
wait_for_re_auth_start_time;
+ if (elapsedTime > reAuthenticateWaitTime) {
+ logger.warn("Client did not re-authenticate back successfully in
" + elapsedTime
+ + "ms. Unregister this client proxy.");
+ pauseOrUnregisterProxy(expired);
+ } else {
+ Thread.sleep(200);
Review comment:
As we know sleep is not the best way to wait for certain state to be
changed; its a last resort if there is no option. Based on the explanation,
what I see is the code is expecting a state change from two different client:
1. Old client:
In this case, if it is an old client disconnect the server to client
connection immediately. At this level we know the clients versions (from the
client proxy); if it is an old version, the connection could be closed
immediately without any wait.
2. New Client:
In this case, the expectation is the new client will re-auth; for java
clients the re-auth is processed by the command or security service; using the
client info the wait in the dispatch could be notified.
These are just thoughts, not sure if there are any hurdle in doing this way.
It will be good investigate to see if this is possible.
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/CacheClientUpdater.java
##########
@@ -1763,6 +1767,19 @@ private void processMessages() {
}
}
+ private void handleAuthenticate(Message clientMessage) {
+ // if client is in multi-user mode, the CacheClientUpdator (at this point)
+ // can't differentiate which user this message is intended to. so throw
exception for now
+ // one possible solution is re-authenticate all users in this client
+ if (qManager.getPool().getMultiuserAuthentication()) {
+ throw new UnsupportedOperationException(
Review comment:
This is getting processed by CacheClientUpdater thread...As you can see
from the thread name, this thread is used only for local cache operation; its
not used to do client-to-server operation. I am worried about the exception it
could see by doing server side operation....And its impact on overall closing
CCU thread and re-creating it. In the past we had several issues with older and
new CCU threads running simultaneously. Without sufficient tests its hard to
see its impact. It will be nice to process this in separate thread than the CCU
thread.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
