From f76687ad68fd0b467728dea60ef60b41c2f1418a Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Fri, 24 Jan 2020 09:49:00 +1100 Subject: [PATCH] Issue 4383 - changes from review Signed-off-by: Lachlan Roberts --- .../server/MultiPartFormInputStream.java | 28 +++++++++++-------- .../server/MultiPartFormInputStreamTest.java | 2 +- 2 files changed, 18 insertions(+), 12 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 index 5d8eaffaa4a..bc7fe0445b4 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/MultiPartFormInputStream.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/MultiPartFormInputStream.java @@ -56,10 +56,16 @@ import org.eclipse.jetty.util.log.Logger; *

* 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 + * The deletion of parts is done by the cleanup thread in all cases except the transition from DELETING->DELETED which * is done by the parsing thread. *

*
{@code
+ * UNPARSED - Parsing has not started, there are no parts which need to be cleaned up.
+ * PARSING  - The parsing thread is reading from the InputStream and generating parts.
+ * PARSED   - Parsing has complete and no more parts will be generated.
+ * DELETING - deleteParts() has been called while we were in PARSING state, parsing thread will do the delete.
+ * DELETED  - The parts have been deleted, this is the terminal state.
+ *
  *                              deleteParts()
  *     +--------------------------------------------------------------+
  *     |                                                              |
@@ -67,8 +73,8 @@ import org.eclipse.jetty.util.log.Logger;
  *  UNPARSED -------> PARSING --------> PARSED  ------------------>DELETED
  *                      |                                             ^
  *                      |                                             |
- *                      +----------------> ERROR ---------------------+
- *                        deleteParts()             parsing thread
+ *                      +---------------> DELETING -------------------+
+ *                        deleteParts()               parsing thread
  * }
* @see https://tools.ietf.org/html/rfc7578 */ @@ -79,8 +85,8 @@ public class MultiPartFormInputStream UNPARSED, PARSING, PARSED, - DELETED, - ERROR + DELETING, + DELETED } private static final Logger LOG = Log.getLogger(MultiPartFormInputStream.class); @@ -406,11 +412,11 @@ public class MultiPartFormInputStream switch (state) { case DELETED: - case ERROR: + case DELETING: return; case PARSING: - state = State.ERROR; + state = State.DELETING; return; case UNPARSED: @@ -423,10 +429,10 @@ public class MultiPartFormInputStream } } - uncheckedDeleteParts(); + delete(); } - private void uncheckedDeleteParts() + private void delete() { MultiException err = null; for (List parts : _parts.values()) @@ -631,7 +637,7 @@ public class MultiPartFormInputStream state = State.PARSED; break; - case ERROR: + case DELETING: state = State.DELETED; cleanup = true; break; @@ -642,7 +648,7 @@ public class MultiPartFormInputStream } if (cleanup) - uncheckedDeleteParts(); + delete(); } } 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 index 733bb742151..119c0d77470 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/MultiPartFormInputStreamTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/MultiPartFormInputStreamTest.java @@ -563,7 +563,7 @@ public class MultiPartFormInputStreamTest // The call to getParts should throw an error. Throwable error = assertThrows(IOException.class, mpis::getParts); - assertThat(error.getMessage(), is("ERROR")); + assertThat(error.getMessage(), is("DELETING")); // There was no error with the cleanup. assertNull(cleanupError.get());