Skip to content

Commit

Permalink
Issue #4383 - synchronize on getParts() and getPart() methods
Browse files Browse the repository at this point in the history 
This will stop two threads from parsing at the same time.

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
  • Loading branch information
@lachlan-roberts
lachlan-roberts committed Dec 10, 2019
1 parent 979f88f commit 6888e17
Show file tree
Hide file tree
Showing 4 changed files with 84 additions and 63 deletions.
90 changes: 48 additions & 42 deletions jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormInputStream.java
View file
Expand Up @@ -35,6 +35,7 @@
import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.stream.Collectors;
import javax.servlet.MultipartConfigElement;
import javax.servlet.ServletInputStream;
import javax.servlet.http.Part;
Expand Down Expand Up @@ -339,29 +340,33 @@ public MultiPartFormInputStream(InputStream in, String contentType, MultipartCon
if (in instanceof ServletInputStream)
{
if (((ServletInputStream)in).isFinished())
{
_parsed = true;
return;
}
}
}

/**
* @return whether the list of parsed parts is empty
* @deprecated use getParts().isEmpty()
*/
@Deprecated
public boolean isEmpty()
{
if (_parts.isEmpty())
return true;

Collection<List<Part>> values = _parts.values();
for (List<Part> partList : values)
synchronized (this)
{
if (!partList.isEmpty())
return false;
}
if (!_parsed)
throw new IllegalStateException();

if (_parts.isEmpty())
return true;

return true;
for (List<Part> partList : _parts.values())
{
if (!partList.isEmpty())
return false;
}

return true;
}
}

/**
Expand Down Expand Up @@ -390,27 +395,31 @@ public Collection<Part> getParsedParts()
*/
public void deleteParts()
{
MultiException err = null;
for (List<Part> parts : _parts.values())
// TODO: Can we cancel parsing somehow instead of blocking.
synchronized (this)
{
for (Part p : parts)
MultiException err = null;
for (List<Part> parts : _parts.values())
{
try
for (Part p : parts)
{
((MultiPart)p).cleanUp();
}
catch (Exception e)
{
if (err == null)
err = new MultiException();
err.add(e);
try
{
((MultiPart)p).cleanUp();
}
catch (Exception e)
{
if (err == null)
err = new MultiException();
err.add(e);
}
}
}
}
_parts.clear();
_parts.clear();

if (err != null)
err.ifExceptionThrowRuntime();
if (err != null)
err.ifExceptionThrowRuntime();
}
}

/**
Expand All @@ -421,18 +430,13 @@ public void deleteParts()
*/
public Collection<Part> getParts() throws IOException
{
if (!_parsed)
parse();
throwIfError();

Collection<List<Part>> values = _parts.values();
List<Part> parts = new ArrayList<>();
for (List<Part> o : values)
synchronized (this)
{
List<Part> asList = LazyList.getList(o, false);
parts.addAll(asList);
if (!_parsed)
parse();
throwIfError();
return _parts.values().stream().flatMap(List::stream).collect(Collectors.toList());
}
return parts;
}

/**
Expand All @@ -444,10 +448,13 @@ public Collection<Part> getParts() throws IOException
*/
public Part getPart(String name) throws IOException
{
if (!_parsed)
parse();
throwIfError();
return _parts.getValue(name, 0);
synchronized (this)
{
if (!_parsed)
parse();
throwIfError();
return _parts.getValue(name, 0);
}
}

/**
Expand Down Expand Up @@ -522,7 +529,6 @@ else if ("".equals(_config.getLocation()))

while (true)
{

len = _in.read(data);

if (len > 0)
Expand Down
1 change: 1 addition & 0 deletions jetty-server/src/main/java/org/eclipse/jetty/server/MultiParts.java
View file
Expand Up @@ -46,6 +46,7 @@ public interface MultiParts extends Closeable

Part getPart(String name) throws IOException;

@Deprecated
boolean isEmpty();

ContextHandler.Context getContext();
Expand Down
2 changes: 1 addition & 1 deletion jetty-server/src/main/java/org/eclipse/jetty/server/Request.java
View file
Expand Up @@ -2310,7 +2310,7 @@ public Collection<Part> getParts() throws IOException, ServletException
return getParts(null);
}

private Collection<Part> getParts(MultiMap<String> params) throws IOException
private synchronized Collection<Part> getParts(MultiMap<String> params) throws IOException
{
if (_multiParts == null)
_multiParts = (MultiParts)getAttribute(MULTIPARTS);
Expand Down
54 changes: 34 additions & 20 deletions jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java
View file
Expand Up @@ -351,16 +351,23 @@ public void testMultiPart() throws Exception
@Override
public void requestDestroyed(ServletRequestEvent sre)
{
MultiParts m = (MultiParts)sre.getServletRequest().getAttribute(Request.MULTIPARTS);
assertNotNull(m);
ContextHandler.Context c = m.getContext();
assertNotNull(c);
assertTrue(c == sre.getServletContext());
assertTrue(!m.isEmpty());
assertTrue(testTmpDir.list().length == 2);
super.requestDestroyed(sre);
String[] files = testTmpDir.list();
assertTrue(files.length == 0);
try
{
MultiParts m = (MultiParts)sre.getServletRequest().getAttribute(Request.MULTIPARTS);
assertNotNull(m);
ContextHandler.Context c = m.getContext();
assertNotNull(c);
assertTrue(c == sre.getServletContext());
assertTrue(!m.getParts().isEmpty());
assertTrue(testTmpDir.list().length == 2);
super.requestDestroyed(sre);
String[] files = testTmpDir.list();
assertTrue(files.length == 0);
}
catch (IOException e)
{
throw new RuntimeException(e);
}
}
});
_server.stop();
Expand Down Expand Up @@ -411,16 +418,23 @@ public void testUtilMultiPart() throws Exception
@Override
public void requestDestroyed(ServletRequestEvent sre)
{
MultiParts m = (MultiParts)sre.getServletRequest().getAttribute(Request.MULTIPARTS);
assertNotNull(m);
ContextHandler.Context c = m.getContext();
assertNotNull(c);
assertTrue(c == sre.getServletContext());
assertTrue(!m.isEmpty());
assertTrue(testTmpDir.list().length == 2);
super.requestDestroyed(sre);
String[] files = testTmpDir.list();
assertTrue(files.length == 0);
try
{
MultiParts m = (MultiParts)sre.getServletRequest().getAttribute(Request.MULTIPARTS);
assertNotNull(m);
ContextHandler.Context c = m.getContext();
assertNotNull(c);
assertTrue(c == sre.getServletContext());
assertTrue(!m.getParts().isEmpty());
assertTrue(testTmpDir.list().length == 2);
super.requestDestroyed(sre);
String[] files = testTmpDir.list();
assertTrue(files.length == 0);
}
catch (IOException t)
{
throw new RuntimeException(t);
}
}
});
_server.stop();
Expand Down

0 comments on commit 6888e17

Please sign in to comment.