GUACAMOLE-2036: Merge changes to reuse buffers received by parser when converting instructions back to character arrays.

aleitner · web-flow · commit 8aec82fd855e · 2025-02-26T07:12:40.000+09:00
diff --git a/guacamole-common/src/main/java/org/apache/guacamole/io/ReaderGuacamoleReader.java b/guacamole-common/src/main/java/org/apache/guacamole/io/ReaderGuacamoleReader.java
@@ -24,7 +24,6 @@
 import java.io.Reader;
 import java.net.SocketException;
 import java.net.SocketTimeoutException;
-import java.util.Arrays;
 import org.apache.guacamole.GuacamoleConnectionClosedException;
 import org.apache.guacamole.GuacamoleException;
 import org.apache.guacamole.GuacamoleServerException;
@@ -92,7 +91,7 @@ public char[] read() throws GuacamoleException {
         if (instruction == null)
             return null;
 
-        return instruction.toString().toCharArray();
+        return instruction.toCharArray();
     }
 
     @Override
diff --git a/guacamole-common/src/main/java/org/apache/guacamole/protocol/GuacamoleInstruction.java b/guacamole-common/src/main/java/org/apache/guacamole/protocol/GuacamoleInstruction.java
@@ -41,41 +41,91 @@ public class GuacamoleInstruction {
     private final List<String> args;
 
     /**
-     * The cached result of converting this GuacamoleInstruction to the format
-     * used by the Guacamole protocol.
+     * The cached String result of converting this GuacamoleInstruction to the
+     * format used by the Guacamole protocol.
+     *
+     * @see #toString()
      */
-    private String protocolForm = null;
+    private String rawString = null;
 
     /**
-     * Creates a new GuacamoleInstruction having the given Operation and
-     * list of arguments values.
+     * The cached char[] result of converting this GuacamoleInstruction to the
+     * format used by the Guacamole protocol.
      *
-     * @param opcode The opcode of the instruction to create.
-     * @param args The list of argument values to provide in the new
-     *             instruction if any.
+     * @see #toCharArray()
+     */
+    private char[] rawChars = null;
+
+    /**
+     * Creates a new GuacamoleInstruction having the given opcode and list of
+     * argument values.
+     *
+     * @param opcode
+     *     The opcode of the instruction to create.
+     *
+     * @param args
+     *     The list of argument values to provide in the new instruction, if
+     *     any.
      */
     public GuacamoleInstruction(String opcode, String... args) {
         this.opcode = opcode;
         this.args = Collections.unmodifiableList(Arrays.asList(args));
     }
 
     /**
-     * Creates a new GuacamoleInstruction having the given Operation and
-     * list of arguments values. The list given will be used to back the
-     * internal list of arguments and the list returned by getArgs().
+     * Creates a new GuacamoleInstruction having the given opcode and list of
+     * argument values. The list given will be used to back the internal list of
+     * arguments and the list returned by {@link #getArgs()}.
+     * <p>
+     * The provided argument list may not be modified in any way after being
+     * provided to this constructor. Doing otherwise will result in undefined
+     * behavior.
+     *
+     * @param opcode
+     *     The opcode of the instruction to create.
      *
-     * @param opcode The opcode of the instruction to create.
-     * @param args The list of argument values to provide in the new
-     *             instruction if any.
+     * @param args
+     *     The list of argument values to provide in the new instruction, if
+     *     any.
      */
     public GuacamoleInstruction(String opcode, List<String> args) {
         this.opcode = opcode;
         this.args = Collections.unmodifiableList(args);
     }
 
+    /**
+     * Creates a new GuacamoleInstruction having the given opcode, list of
+     * argument values, and underlying protocol representation. The list given
+     * will be used to back the internal list of arguments and the list
+     * returned by {@link #getArgs()}. The provided protocol representation
+     * will be used to back the internal protocol representation and values
+     * returned by {@link #toCharArray()} and {@link #toString()}.
+     * <p>
+     * Neither the provided argument list nor the provided protocol
+     * representation may be modified in any way after being provided to this
+     * constructor. Doing otherwise will result in undefined behavior.
+     *
+     * @param opcode
+     *     The opcode of the instruction to create.
+     *
+     * @param args
+     *     The list of argument values to provide in the new instruction, if
+     *     any.
+     *
+     * @param raw
+     *     The underlying representation of this instruction as would be sent
+     *     over the network via the Guacamole protocol.
+     */
+    public GuacamoleInstruction(String opcode, List<String> args, char[] raw) {
+        this(opcode, args);
+        this.rawChars = raw;
+    }
+
     /**
      * Returns the opcode associated with this GuacamoleInstruction.
-     * @return The opcode associated with this GuacamoleInstruction.
+     *
+     * @return
+     *     The opcode associated with this GuacamoleInstruction.
      */
     public String getOpcode() {
         return opcode;
@@ -86,8 +136,9 @@ public String getOpcode() {
      * GuacamoleInstruction. Note that the List returned is immutable.
      * Attempts to modify the list will result in exceptions.
      *
-     * @return A List of all argument values specified for this
-     *         GuacamoleInstruction.
+     * @return
+     *     An unmodifiable List of all argument values specified for this
+     *     GuacamoleInstruction.
      */
     public List<String> getArgs() {
         return args;
@@ -122,28 +173,58 @@ public String toString() {
 
         // Avoid rebuilding Guacamole protocol form of instruction if already
         // known
-        if (protocolForm == null) {
+        if (rawString == null) {
 
-            StringBuilder buff = new StringBuilder();
+            // Prefer to construct String from existing char array, rather than
+            // reconstruct protocol from scratch
+            if (rawChars != null)
+                rawString = new String(rawChars);
 
-            // Write opcode
-            appendElement(buff, opcode);
+            // Reconstruct protocol details only if truly necessary
+            else {
 
-            // Write argument values
-            for (String value : args) {
-                buff.append(',');
-                appendElement(buff, value);
-            }
+                StringBuilder buff = new StringBuilder();
 
-            // Write terminator
-            buff.append(';');
+                // Write opcode
+                appendElement(buff, opcode);
 
-            // Cache result for future calls
-            protocolForm = buff.toString();
+                // Write argument values
+                for (String value : args) {
+                    buff.append(',');
+                    appendElement(buff, value);
+                }
+
+                // Write terminator
+                buff.append(';');
+
+                // Cache result for future calls
+                rawString = buff.toString();
+
+            }
 
         }
 
-        return protocolForm;
+        return rawString;
+
+    }
+
+    /**
+     * Returns this GuacamoleInstruction in the form it would be sent over the
+     * Guacamole protocol. The returned char[] MUST NOT be modified. If the
+     * returned char[] is modified, the results of doing so are undefined.
+     *
+     * @return
+     *     This GuacamoleInstruction in the form it would be sent over the
+     *     Guacamole protocol. The returned char[] MUST NOT be modified.
+     */
+    public char[] toCharArray() {
+
+        // Avoid rebuilding Guacamole protocol form of instruction if already
+        // known
+        if (rawChars == null)
+            rawChars = toString().toCharArray();
+
+        return rawChars;
 
     }
 
diff --git a/guacamole-common/src/main/java/org/apache/guacamole/protocol/GuacamoleParser.java b/guacamole-common/src/main/java/org/apache/guacamole/protocol/GuacamoleParser.java
@@ -108,6 +108,18 @@ private enum State {
      */
     private final String elements[] = new String[INSTRUCTION_MAX_ELEMENTS];
 
+    /**
+     * A copy of the raw protocol data that has been parsed for the current
+     * instruction. This value is maintained by {@link #append(char[], int, int)}.
+     */
+    private final char rawInstruction[] = new char[INSTRUCTION_MAX_LENGTH];
+
+    /**
+     * The offset within {@link #rawInstruction} that new data should be
+     * appended. This value is maintained by {@link #append(char[], int, int)}.
+     */
+    private int rawInstructionOffset = 0;
+
     /**
      * Appends data from the given buffer to the current instruction.
      * 
@@ -130,6 +142,71 @@ private enum State {
      */
     public int append(char chunk[], int offset, int length) throws GuacamoleException {
 
+        int originalOffset = offset;
+        int originalLength = length;
+
+        // Process as much of the received chunk as possible
+        while (length > 0) {
+
+            int appended = processElement(chunk, offset, length);
+            if (appended == 0)
+                break;
+
+            length -= appended;
+            offset += appended;
+        }
+
+        // Update the raw copy of the received instruction with whatever data
+        // has now been processed
+        int charsParsed = originalLength - length;
+        if (charsParsed > 0) {
+
+            System.arraycopy(chunk, originalOffset, rawInstruction, rawInstructionOffset, charsParsed);
+            rawInstructionOffset += charsParsed;
+
+            // If the instruction is now complete, we're good to store the
+            // parsed instruction for future retrieval via next()
+            if (state == State.COMPLETE) {
+                parsedInstruction = new GuacamoleInstruction(elements[0], Arrays.asList(elements).subList(1, elementCount),
+                        Arrays.copyOf(rawInstruction, rawInstructionOffset));
+                rawInstructionOffset = 0;
+            }
+
+        }
+
+        return charsParsed;
+
+    }
+
+    /**
+     * Processes additional data from the given buffer, potentially adding
+     * another element to the current instruction being parsed. This function
+     * will need to be invoked multiple times per instruction until all data
+     * for that instruction is ready.
+     * <p>
+     * This function DOES NOT update {@link #parsedInstruction}. The caller
+     * ({@link #append(char[], int, int)}) must update this as necessary when
+     * the parser {@link #state} indicates the instruction is complete.
+     *
+     * @param chunk
+     *     The buffer containing the data to append.
+     *
+     * @param offset
+     *     The offset within the buffer where the data begins.
+     *
+     * @param length
+     *     The length of the data to append.
+     *
+     * @return
+     *     The number of characters appended, or 0 if complete instructions
+     *     have already been parsed and must be read via next() before more
+     *     data can be appended.
+     *
+     * @throws GuacamoleException
+     *     If an error occurs while parsing the new data.
+     */
+    private int processElement(char chunk[], int offset, int length) throws GuacamoleException {
+
         int charsParsed = 0;
 
         // Do not exceed maximum number of elements
@@ -215,8 +292,6 @@ else if (Character.isSurrogatePair(chunk[offset + charsParsed + elementLength -
                 // If semicolon, store end-of-instruction
                 case ';':
                     state = State.COMPLETE;
-                    parsedInstruction = new GuacamoleInstruction(elements[0],
-                            Arrays.asList(elements).subList(1, elementCount));
                     break;
 
                 // If comma, move on to next element
