Skip to content

HDDS-10777. S3 gateway error when parsing xml in concurrent execution #6609

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
adoroszlai merged 3 commits into from
May 8, 2024
Merged

HDDS-10777. S3 gateway error when parsing xml in concurrent execution #6609

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e5acbd6
HDDS-10777. S3 gateway error when parsing xml in concurrent execution
guohao-rosicky Apr 30, 2024
0d53980
test
guohao-rosicky Apr 30, 2024
d4041bb
HDDS-10777. S3 gateway error when parsing xml in concurrent execution
guohao-rosicky May 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 4 additions & 12 deletions .../java/org/apache/hadoop/ozone/s3/endpoint/CompleteMultipartUploadRequestUnmarshaller.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,19 +43,7 @@
public class CompleteMultipartUploadRequestUnmarshaller
implements MessageBodyReader<CompleteMultipartUploadRequest> {

private final JAXBContext context;
private final XMLReader xmlReader;

public CompleteMultipartUploadRequestUnmarshaller() {
try {
context = JAXBContext.newInstance(CompleteMultipartUploadRequest.class);
SAXParserFactory saxParserFactory = SAXParserFactory.newInstance();
saxParserFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
xmlReader = saxParserFactory.newSAXParser().getXMLReader();
} catch (Exception ex) {
throw new AssertionError("Can not instantiate " +
"CompleteMultipartUploadRequest parser", ex);
}
Copy link
Contributor

@ivandika3 ivandika3 May 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might also need to change for PutBucketAclRequestUnmarshaller since it has a similar pattern (although not used as often).

Copy link
Contributor Author

@guohao-rosicky guohao-rosicky May 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure.

}
@Override
public boolean isReadable(Class<?> aClass, Type type,
Expand All @@ -70,6 +58,10 @@ public CompleteMultipartUploadRequest readFrom(
MultivaluedMap<String, String> multivaluedMap,
InputStream inputStream) throws IOException, WebApplicationException {
try {
JAXBContext context = JAXBContext.newInstance(CompleteMultipartUploadRequest.class);
SAXParserFactory saxParserFactory = SAXParserFactory.newInstance();
saxParserFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
XMLReader xmlReader = saxParserFactory.newSAXParser().getXMLReader();
UnmarshallerHandler unmarshallerHandler =
context.createUnmarshaller().getUnmarshallerHandler();
XmlNamespaceFilter filter =
Expand Down
16 changes: 4 additions & 12 deletions ...way/src/main/java/org/apache/hadoop/ozone/s3/endpoint/MultiDeleteRequestUnmarshaller.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,19 +42,7 @@
public class MultiDeleteRequestUnmarshaller
implements MessageBodyReader<MultiDeleteRequest> {

private final JAXBContext context;
private final XMLReader xmlReader;

public MultiDeleteRequestUnmarshaller() {
try {
context = JAXBContext.newInstance(MultiDeleteRequest.class);
SAXParserFactory saxParserFactory = SAXParserFactory.newInstance();
saxParserFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
xmlReader = saxParserFactory.newSAXParser().getXMLReader();
} catch (Exception ex) {
throw new AssertionError("Can't instantiate MultiDeleteRequest parser",
ex);
}
}

@Override
Expand All @@ -68,6 +56,10 @@ public MultiDeleteRequest readFrom(Class<MultiDeleteRequest> type,
Type genericType, Annotation[] annotations, MediaType mediaType,
MultivaluedMap<String, String> httpHeaders, InputStream entityStream) {
try {
JAXBContext context = JAXBContext.newInstance(MultiDeleteRequest.class);
SAXParserFactory saxParserFactory = SAXParserFactory.newInstance();
saxParserFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
Copy link
Contributor

@ivandika3 ivandika3 May 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might not need to instantiate this for every request. Could you help check whether instantiating only XMLReader for each request is enough?

Copy link
Contributor Author

@guohao-rosicky guohao-rosicky May 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ivandika3 Thanks for the review.
Yes, this modification solves the problem as well, I've done a test, please see the new code.

XMLReader xmlReader = saxParserFactory.newSAXParser().getXMLReader();
UnmarshallerHandler unmarshallerHandler =
context.createUnmarshaller().getUnmarshallerHandler();

Expand Down
44 changes: 44 additions & 0 deletions ...a/org/apache/hadoop/ozone/s3/endpoint/TestCompleteMultipartUploadRequestUnmarshaller.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,16 @@

import java.io.ByteArrayInputStream;
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import java.util.UUID;
import java.util.concurrent.CompletableFuture;

import org.junit.jupiter.api.Test;

import static java.nio.charset.StandardCharsets.UTF_8;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;

/**
* Class tests Unmarshall logic of {@link CompleteMultipartUploadRequest}.
Expand Down Expand Up @@ -75,6 +79,7 @@ public void fromStreamWithoutNamespace() throws IOException {
}

private void checkContent(CompleteMultipartUploadRequest request) {
assertNotNull(request);
assertEquals(2, request.getPartList().size());

List<CompleteMultipartUploadRequest.Part> parts =
Expand All @@ -89,4 +94,43 @@ private CompleteMultipartUploadRequest unmarshall(
return new CompleteMultipartUploadRequestUnmarshaller()
.readFrom(null, null, null, null, null, inputBody);
}

@Test
public void concurrentParse() {
CompleteMultipartUploadRequestUnmarshaller unmarshaller =
new CompleteMultipartUploadRequestUnmarshaller();
byte[] bytes = ("<CompleteMultipartUpload>" + "<Part><ETag>" + part1 +
"</ETag><PartNumber>1</PartNumber" + "></Part><Part><ETag>" +
part2 + "</ETag><PartNumber>2" +
"</PartNumber></Part></CompleteMultipartUpload>").getBytes(
UTF_8);

List<CompletableFuture<CompleteMultipartUploadRequest>> futures =
new ArrayList<>();
for (int i = 0; i < 20; i++) {
futures.add(CompletableFuture.supplyAsync(() -> {
try {
//GIVEN
ByteArrayInputStream inputBody = new ByteArrayInputStream(bytes);
//WHEN
return unmarshall(unmarshaller, inputBody);
} catch (IOException e) {
return null;
}
}));
}

for (CompletableFuture<CompleteMultipartUploadRequest> future : futures) {
CompleteMultipartUploadRequest request = future.join();
//THEN
checkContent(request);
}
}

private CompleteMultipartUploadRequest unmarshall(
CompleteMultipartUploadRequestUnmarshaller unmarshaller,
ByteArrayInputStream inputBody) throws IOException {
return unmarshaller
.readFrom(null, null, null, null, null, inputBody);
}
}