[accumulo] branch 2.1 updated: Rework #3047 so that the client address is captured correctly (#3103)

[cs...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

cshannon pushed a commit to branch 2.1
in repository https://gitbox.apache.org/repos/asf/accumulo.git


The following commit(s) were added to refs/heads/2.1 by this push:
     new d775dac96b Rework #3047 so that the client address is captured correctly (#3103)
d775dac96b is described below

commit d775dac96b96c5e232c1f517c5d5d9a3833a6f09
Author: Christopher L. Shannon <ch...@gmail.com>
AuthorDate: Sun Dec 4 11:00:35 2022 -0500

    Rework #3047 so that the client address is captured correctly (#3103)
    
    Rework #3047 so that the client address is captured correctly
    
    The original implementation didn't work correctly because more than one thread
    will process the same FrameBuffer. This new approach captures the client
    address from the socket for each new FrameBuffer which is used for each
    request and sets on the ThreadLocal on invoke()
---
 .../server/rpc/CustomNonBlockingServer.java        | 42 +++++++++++++++++-----
 1 file changed, 34 insertions(+), 8 deletions(-)

diff --git a/server/base/src/main/java/org/apache/accumulo/server/rpc/CustomNonBlockingServer.java b/server/base/src/main/java/org/apache/accumulo/server/rpc/CustomNonBlockingServer.java
index 8fcc2496c6..929d8900a5 100644
--- a/server/base/src/main/java/org/apache/accumulo/server/rpc/CustomNonBlockingServer.java
+++ b/server/base/src/main/java/org/apache/accumulo/server/rpc/CustomNonBlockingServer.java
@@ -112,20 +112,20 @@ public class CustomNonBlockingServer extends THsHaServer {
    * extract the client's network location before accepting the request.
    */
   private class CustomFrameBuffer extends FrameBuffer {
+    private final String clientAddress;
 
     public CustomFrameBuffer(TNonblockingTransport trans, SelectionKey selectionKey,
         AbstractSelectThread selectThread) throws TTransportException {
       super(trans, selectionKey, selectThread);
+      // Store the clientAddress in the buffer so it can be referenced for logging during read/write
+      this.clientAddress = getClientAddress();
     }
 
     @Override
     public void invoke() {
-      if (trans_ instanceof TNonblockingSocket) {
-        TNonblockingSocket tsock = (TNonblockingSocket) trans_;
-        Socket sock = tsock.getSocketChannel().socket();
-        TServerUtils.clientAddress
-            .set(sock.getInetAddress().getHostAddress() + ":" + sock.getPort());
-      }
+      // On invoke() set the clientAddress on the ThreadLocal so that it can be accessed elsewhere
+      // in the same thread that called invoke() on the buffer
+      TServerUtils.clientAddress.set(clientAddress);
       super.invoke();
     }
 
@@ -134,7 +134,7 @@ public class CustomNonBlockingServer extends THsHaServer {
       boolean result = super.read();
       if (!result) {
         log.trace("CustomFrameBuffer.read returned false when reading data from client: {}",
-            TServerUtils.clientAddress.get());
+            clientAddress);
       }
       return result;
     }
@@ -144,11 +144,37 @@ public class CustomNonBlockingServer extends THsHaServer {
       boolean result = super.write();
       if (!result) {
         log.trace("CustomFrameBuffer.write returned false when writing data to client: {}",
-            TServerUtils.clientAddress.get());
+            clientAddress);
       }
       return result;
     }
 
+    /*
+     * Helper method used to capture the client address inside the CustomFrameBuffer constructor so
+     * that it can be referenced inside the read/write methods for logging purposes. It previously
+     * was only set on the ThreadLocal in the invoke() method but that does not work because A) the
+     * method isn't called until after reading is finished so the value will be null inside of
+     * read() and B) The other problem is that invoke() is called on a different thread than
+     * read()/write() so even if the order was correct it would not be available.
+     *
+     * Since a new FrameBuffer is created for each request we can use it to capture the client
+     * address earlier in the constructor and not wait for invoke(). A FrameBuffer is used to read
+     * data and write a response back to the client and as part of creation of the buffer the
+     * TNonblockingSocket is stored as a final variable and won't change so we can safely capture
+     * the clientAddress in the constructor and use it for logging during read/write and then use
+     * the value inside of invoke() to set the ThreadLocal so the client address will still be
+     * available on the thread that called invoke().
+     */
+    private String getClientAddress() {
+      String clientAddress = null;
+      if (trans_ instanceof TNonblockingSocket) {
+        TNonblockingSocket tsock = (TNonblockingSocket) trans_;
+        Socket sock = tsock.getSocketChannel().socket();
+        clientAddress = sock.getInetAddress().getHostAddress() + ":" + sock.getPort();
+        log.trace("CustomFrameBuffer captured client address: {}", clientAddress);
+      }
+      return clientAddress;
+    }
   }
 
 }