Merge pull request #4391 from eclipse/jetty-9.4.x-3512-httpclient_multipart_close

Fixes #3512 - File descriptor is not released after zip file uploaded…

jetty-client/src/main/java/org/eclipse/jetty/client/HttpContent.java

@@ -26,6 +26,7 @@ import java.util.Iterator;
 import org.eclipse.jetty.client.api.ContentProvider;
 import org.eclipse.jetty.util.BufferUtil;
 import org.eclipse.jetty.util.Callback;
+import org.eclipse.jetty.util.IO;
 import org.eclipse.jetty.util.log.Log;
 import org.eclipse.jetty.util.log.Logger;
 
@@ -218,15 +219,8 @@ public class HttpContent implements Callback, Closeable
     @Override
     public void close()
     {
-        try
+        if (iterator instanceof Closeable)
-        {
+            IO.close((Closeable)iterator);
-            if (iterator instanceof Closeable)
-                ((Closeable)iterator).close();
-        }
-        catch (Throwable x)
-        {
-            LOG.ignore(x);
-        }
     }
 
     @Override

jetty-client/src/main/java/org/eclipse/jetty/client/util/MultiPartContentProvider.java

@@ -38,6 +38,7 @@ import org.eclipse.jetty.http.HttpFields;
 import org.eclipse.jetty.http.HttpHeader;
 import org.eclipse.jetty.io.RuntimeIOException;
 import org.eclipse.jetty.util.Callback;
+import org.eclipse.jetty.util.IO;
 import org.eclipse.jetty.util.log.Log;
 import org.eclipse.jetty.util.log.Logger;
 
@@ -345,10 +346,16 @@ public class MultiPartContentProvider extends AbstractTypedContentProvider imple
                         if (iterator.hasNext())
                             return iterator.next();
                         ++index;
-                        if (index == parts.size())
+                        if (index < parts.size())
-                            state = State.LAST_BOUNDARY;
+                        {
-                        else
                             state = State.MIDDLE_BOUNDARY;
+                            if (iterator instanceof Closeable)
+                                IO.close((Closeable)iterator);
+                        }
+                        else
+                        {
+                            state = State.LAST_BOUNDARY;
+                        }
                         break;
                     }
                     case MIDDLE_BOUNDARY:
@@ -380,14 +387,14 @@ public class MultiPartContentProvider extends AbstractTypedContentProvider imple
         @Override
         public void succeeded()
         {
-            if (iterator instanceof Callback)
+            if (state == State.CONTENT && iterator instanceof Callback)
                 ((Callback)iterator).succeeded();
         }
 
         @Override
         public void failed(Throwable x)
         {
-            if (iterator instanceof Callback)
+            if (state == State.CONTENT && iterator instanceof Callback)
                 ((Callback)iterator).failed(x);
         }
 

jetty-client/src/test/java/org/eclipse/jetty/client/util/MultiPartContentProviderTest.java

@@ -20,6 +20,7 @@ package org.eclipse.jetty.client.util;
 
 import java.io.BufferedWriter;
 import java.io.ByteArrayInputStream;
+import java.io.Closeable;
 import java.io.File;
 import java.io.IOException;
 import java.io.OutputStream;
@@ -31,10 +32,12 @@ import java.nio.file.Path;
 import java.nio.file.StandardOpenOption;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.Iterator;
 import java.util.List;
 import java.util.Random;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
 import javax.servlet.MultipartConfigElement;
 import javax.servlet.ServletException;
 import javax.servlet.http.HttpServletRequest;
@@ -435,6 +438,46 @@ public class MultiPartContentProviderTest extends AbstractHttpClientServerTest
         assertTrue(responseLatch.await(5, TimeUnit.SECONDS));
     }
 
+    @ParameterizedTest
+    @ArgumentsSource(ScenarioProvider.class)
+    public void testEachPartIsClosed(Scenario scenario) throws Exception
+    {
+        String name1 = "field1";
+        String value1 = "value1";
+        String name2 = "field2";
+        String value2 = "value2";
+        start(scenario, new AbstractMultiPartHandler()
+        {
+            @Override
+            protected void handle(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException
+            {
+                Collection<Part> parts = request.getParts();
+                assertEquals(2, parts.size());
+                Iterator<Part> iterator = parts.iterator();
+                Part part1 = iterator.next();
+                assertEquals(name1, part1.getName());
+                assertEquals(value1, IO.toString(part1.getInputStream()));
+                Part part2 = iterator.next();
+                assertEquals(name2, part2.getName());
+                assertEquals(value2, IO.toString(part2.getInputStream()));
+            }
+        });
+
+        AtomicInteger closeCount = new AtomicInteger();
+        MultiPartContentProvider multiPart = new MultiPartContentProvider();
+        multiPart.addFieldPart(name1, new CloseableStringContentProvider(value1, closeCount::incrementAndGet), null);
+        multiPart.addFieldPart(name2, new CloseableStringContentProvider(value2, closeCount::incrementAndGet), null);
+        multiPart.close();
+        ContentResponse response = client.newRequest("localhost", connector.getLocalPort())
+            .scheme(scenario.getScheme())
+            .method(HttpMethod.POST)
+            .content(multiPart)
+            .send();
+
+        assertEquals(200, response.getStatus());
+        assertEquals(2, closeCount.get());
+    }
+
     private abstract static class AbstractMultiPartHandler extends AbstractHandler
     {
         @Override
@@ -448,4 +491,49 @@ public class MultiPartContentProviderTest extends AbstractHttpClientServerTest
 
         protected abstract void handle(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException;
     }
+
+    private static class CloseableStringContentProvider extends StringContentProvider
+    {
+        private final Runnable closeFn;
+
+        private CloseableStringContentProvider(String content, Runnable closeFn)
+        {
+            super(content);
+            this.closeFn = closeFn;
+        }
+
+        @Override
+        public Iterator<ByteBuffer> iterator()
+        {
+            return new CloseableIterator<>(super.iterator());
+        }
+
+        private class CloseableIterator<T> implements Iterator<T>, Closeable
+        {
+            private final Iterator<T> iterator;
+
+            public CloseableIterator(Iterator<T> iterator)
+            {
+                this.iterator = iterator;
+            }
+
+            @Override
+            public boolean hasNext()
+            {
+                return iterator.hasNext();
+            }
+
+            @Override
+            public T next()
+            {
+                return iterator.next();
+            }
+
+            @Override
+            public void close()
+            {
+                closeFn.run();
+            }
+        }
+    }
 }