Skip to content
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.

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

Resteasy 1700 #1244

Closed
wants to merge 16 commits into from
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import org.jboss.resteasy.util.InputStreamToByteArray;
import org.jboss.resteasy.util.ReadFromStream;
import org.jboss.resteasy.util.Types;
import org.omg.IOP.IOR;
Copy link
Member

Choose a reason for hiding this comment

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

This is not needed, is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oups... useless you're right


import javax.ws.rs.ProcessingException;
import javax.ws.rs.core.MediaType;
Expand Down Expand Up @@ -70,12 +71,38 @@ public void setClientConfiguration(ClientConfiguration configuration)
this.processor = configuration;
}

@Override
public Object getEntity()
{
abortIfClosed();
return super.getEntity();
}
@Override
public Object getEntity() {
abortIfClosed();
Object entity = super.getEntity();
if (entity != null) {
return entity;
}
//Check if the entity was previously fully consumed
try {
InputStream inputStream = getEntityStream();
Copy link
Member

Choose a reason for hiding this comment

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

The call to getEntityStream() here is possibly a problem. Basically, we need to make sure that method is not called while a ClientResponse object is garbage collected as a consequence of finalize method having been aggressively called. So far, we didn't bother about this problem because the only invoker to that protected method is already synchronized (readFrom(...) ). In any case, it would probably be safe to simply turn getEntityStream() into a synchronized method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh thanks, I did not pay attention to that.
However how can it be possible for the garbage collector to call finalize while another thread is calling readFrom() on the object being finalized ? I mean, finalize should be invoked on the object only if there is no reference on this object anymore. Am I missing something ?

Copy link
Member

@asoldano asoldano Aug 17, 2017

Choose a reason for hiding this comment

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

Unfortunately, the implementation of the finalizer mechanism is somehow bugged. I should go and verify this specific scenario to tell the exact condition that would cause an error (that's why I've said "possibly a problem" ;-) ), but we've been suffering from similar issues, some of which led to making those other methods synchronized there. If you want to look at an interesting video on this topic, check https://www.youtube.com/watch?v=UrGP6pfb0H8

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting, I 'll have a look at it thanks 👍 .
So ok I'll turn the getEntity() into a synchronized method.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense, thanks

Copy link
Collaborator

Choose a reason for hiding this comment

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

To be honest, I've never really understood the purpose or meaning of getEntity(), but this does seem to be correct.

if(inputStream.available() == 0){
throw new IllegalStateException();
}
return inputStream;
} catch (IOException e) {
throw new IllegalStateException(e);
}
}

@Override
public Class<?> getEntityClass() {
Class<?> classs = super.getEntityClass();
if (classs != null) {
return classs;
}
Object entity = null;
try {
entity = getEntity();
} catch (Exception e) {
}
return entity != null ? entity.getClass() : null;
}

@Override
public boolean hasEntity()
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
package org.jboss.resteasy.test.client;

import java.io.ByteArrayOutputStream;
import java.io.InputStream;

import javax.ws.rs.GET;
import javax.ws.rs.Path;
import javax.ws.rs.Produces;
import javax.ws.rs.QueryParam;
import javax.ws.rs.client.Client;
import javax.ws.rs.client.ClientBuilder;
import javax.ws.rs.client.Invocation;
import javax.ws.rs.core.MediaType;
import javax.ws.rs.core.Response;
import javax.ws.rs.core.Response.Status;
import javax.xml.bind.annotation.XmlRootElement;

import org.jboss.arquillian.container.test.api.Deployment;
import org.jboss.arquillian.container.test.api.RunAsClient;
import org.jboss.arquillian.junit.Arquillian;
import org.jboss.resteasy.client.jaxrs.internal.ClientResponse;
import org.jboss.resteasy.utils.PortProviderUtil;
import org.jboss.resteasy.utils.TestUtil;
import org.jboss.shrinkwrap.api.Archive;
import org.jboss.shrinkwrap.api.spec.WebArchive;
import org.junit.AfterClass;
import org.junit.Assert;
import org.junit.BeforeClass;
import org.junit.Test;
import org.junit.runner.RunWith;

@RunWith(Arquillian.class)
@RunAsClient
public class ClientResponseWithEntityTest {

@XmlRootElement
public static class Message {
private String message;

public Message() {
}

public String getMessage() {
return this.message;
}

public void setMessage(String message) {
this.message = message;
}
}

@Path("echo")
@Produces(MediaType.APPLICATION_XML)
public static class EchoResource {

@GET
public Response echo(@QueryParam("msg") String msg) {
Message message = new Message();
message.setMessage(String.valueOf(msg));
return Response.ok(message).build();
}

}

private static Client client;
private static final String DEP = "ClientResponseWithEntityTest";

@Deployment
public static Archive<?> deploy() {
WebArchive war = TestUtil.prepareArchive(DEP);
war.addClass(Message.class);
war.addClass(EchoResource.class);
return TestUtil.finishContainerPrepare(war, null, EchoResource.class);
}

@BeforeClass
public static void setup() {
client = ClientBuilder.newClient();
}

@AfterClass
public static void cleanup() {
client.close();
}

private static String generateURL() {
return PortProviderUtil.generateBaseUrl(DEP);
}

@Test
public void Should_ReturnEntity_When_NoNull() throws Exception {
Invocation.Builder request = client.target(generateURL()).path("echo").queryParam("msg", "Hello world")
.request(MediaType.APPLICATION_XML_TYPE);
try (ClientResponse response = (ClientResponse) request.get()) {
Assert.assertEquals(Status.OK.getStatusCode(), response.getStatus());
Assert.assertTrue(response.hasEntity());
Assert.assertNotNull(response.getEntity());
Assert.assertNotNull(response.getEntityClass());
}
}

@Test(expected = IllegalStateException.class)
public void Should_ThrowIllegalStateException_When_EntityIsConsumed() throws Exception {
Invocation.Builder request = client.target(generateURL()).path("echo").queryParam("msg", "Hello world")
.request(MediaType.APPLICATION_XML_TYPE);
try (ClientResponse response = (ClientResponse) request.get()) {
Assert.assertEquals(Status.OK.getStatusCode(), response.getStatus());
Assert.assertTrue(response.hasEntity());
InputStream entityStream = (InputStream) response.getEntity();
ByteArrayOutputStream baos = new ByteArrayOutputStream();
byte[] buffer = new byte[1024];
int wasRead = 0;
do {
wasRead = entityStream.read(buffer);
if (wasRead > 0) {
baos.write(buffer, 0, wasRead);
}
} while (wasRead > -1);
response.getEntity();
}
}

}