Issue #4383 - add State diagram and other changes for review

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
jetty · Jan 21, 2020 · b217a35 · b217a35
1 parent c5d074e
commit b217a35
Show file tree

Hide file tree

Showing 2 changed files with 42 additions and 17 deletions.
diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/MultiPartFormInputStream.java b/jetty-server/src/main/java/org/eclipse/jetty/server/MultiPartFormInputStream.java
@@ -52,7 +52,24 @@
  * MultiPartInputStream
  * <p>
  * Handle a MultiPart Mime input stream, breaking it up on the boundary into files and strings.
- *
+ * </p>
+ * <p>
+ * Deleting the parts can be done from a different thread if the parts are parsed asynchronously.
+ * Because of this we use the state to fail the parsing and coordinate which thread will delete any remaining parts.
+ * The deletion of parts is done by the cleanup thread in all cases except the transition from ERROR->DELETED which
+ * is done by the parsing thread.
+ * </p>
+ * <pre>
+ *                              deleteParts()
+ *     +--------------------------------------------------------------+
+ *     |                                                              |
+ *     |                                          deleteParts()       v
+ *  UNPARSED -------> PARSING --------> PARSED  ------------------>DELETED
+ *                      |                                             ^
+ *                      |                                             |
+ *                      +----------------> ERROR ---------------------+
+ *                        deleteParts()             parsing thread
+ * </pre>
  * @see <a href="https://tools.ietf.org/html/rfc7578">https://tools.ietf.org/html/rfc7578</a>
  */
 public class MultiPartFormInputStream
@@ -62,8 +79,8 @@ private enum State
         UNPARSED,
         PARSING,
         PARSED,
-        CLOSING,
-        CLOSED
+        DELETED,
+        ERROR
     }
 
     private static final Logger LOG = Log.getLogger(MultiPartFormInputStream.class);
@@ -384,22 +401,29 @@ public void deleteParts()
         {
             switch (state)
             {
-                case CLOSED:
-                case UNPARSED:
-                    state = State.CLOSED;
+                case DELETED:
+                case ERROR:
                     return;
 
                 case PARSING:
-                    state = State.CLOSING;
+                    state = State.ERROR;
+                    return;
+
+                case UNPARSED:
+                    state = State.DELETED;
                     return;
 
                 case PARSED:
-                case CLOSING:
-                    state = State.CLOSED;
+                    state = State.DELETED;
                     break;
             }
         }
 
+        uncheckedDeleteParts();
+    }
+
+    private void uncheckedDeleteParts()
+    {
         MultiException err = null;
         for (List<Part> parts : _parts.values())
         {
@@ -488,7 +512,7 @@ protected void parse()
                     return;
 
                 default:
-                    _err = new IllegalStateException(state.name());
+                    _err = new IOException(state.name());
                     return;
             }
         }
@@ -537,7 +561,7 @@ else if ("".equals(_config.getLocation()))
                 {
                     if (state != State.PARSING)
                     {
-                        _err = new IllegalStateException(state.name());
+                        _err = new IOException(state.name());
                         return;
                     }
                 }
@@ -607,7 +631,8 @@ else if (len == -1)
                         state = State.PARSED;
                         break;
 
-                    case CLOSING:
+                    case ERROR:
+                        state = State.DELETED;
                         cleanup = true;
                         break;
 
@@ -617,7 +642,7 @@ else if (len == -1)
             }
 
             if (cleanup)
-                deleteParts();
+                uncheckedDeleteParts();
         }
     }
 

diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/MultiPartFormInputStreamTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/MultiPartFormInputStreamTest.java
@@ -564,8 +564,8 @@ public int read() throws IOException
         }).start();
 
         // The call to getParts should throw an error.
-        Throwable error = assertThrows(IllegalStateException.class, mpis::getParts);
-        assertThat(error.getMessage(), is("CLOSING"));
+        Throwable error = assertThrows(IOException.class, mpis::getParts);
+        assertThat(error.getMessage(), is("ERROR"));
 
         // There was no error with the cleanup.
         assertNull(cleanupError.get());
@@ -586,8 +586,8 @@ public void testParseAfterCleanUp() throws Exception
         mpis.deleteParts();
 
         // The call to getParts should throw because we have already cleaned up the parts.
-        Throwable error = assertThrows(IllegalStateException.class, mpis::getParts);
-        assertThat(error.getMessage(), is("CLOSED"));
+        Throwable error = assertThrows(IOException.class, mpis::getParts);
+        assertThat(error.getMessage(), is("DELETED"));
 
         // Even though we called getParts() we never even created the tmp directory as we had already called deleteParts().
         assertFalse(_tmpDir.exists());