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
base: master
from

Conversation

Projects
None yet
3 participants
@NicoNes
Contributor

NicoNes commented Aug 9, 2017

No description provided.

@asoldano

Thanks for having reported this and for the PR.
Besides for the useless commits that should be squashed and the comments that I've already put in the review, can you please confirm that what you're basically achieving in the PR here is addressing what's written in javax.ws.rs.core.Response javadoc at https://github.com/jax-rs/api/blob/2.0/src/jax-rs-api/src/main/java/javax/ws/rs/core/Response.java#L104 ?
Thanks

@@ -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;

This comment has been minimized.

@asoldano

asoldano Aug 17, 2017

Member

This is not needed, is it?

This comment has been minimized.

@NicoNes

NicoNes Aug 17, 2017

Contributor

Oups... useless you're right

}
//Check if the entity was previously fully consumed
try {
InputStream inputStream = getEntityStream();

This comment has been minimized.

@asoldano

asoldano Aug 17, 2017

Member

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.

This comment has been minimized.

@NicoNes

NicoNes Aug 17, 2017

Contributor

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 ?

This comment has been minimized.

@asoldano

asoldano Aug 17, 2017

Member

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

This comment has been minimized.

@NicoNes

NicoNes Aug 17, 2017

Contributor

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

This comment has been minimized.

@asoldano

asoldano Aug 17, 2017

Member

Makes sense, thanks

@NicoNes

This comment has been minimized.

Contributor

NicoNes commented Aug 17, 2017

Yes, this PR addresses what's written in javadoc of javax.ws.rs.core.Response.getEntity()

}
//Check if the entity was previously fully consumed
try {
InputStream inputStream = getEntityStream();

This comment has been minimized.

@ronsigal

ronsigal Aug 24, 2017

Collaborator

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

@ronsigal

This comment has been minimized.

Collaborator

ronsigal commented Aug 24, 2017

I remember why I used to think Response.getEntity() might be correct. The javadoc for Response.readEntity() says,

A message instance returned from this method will be cached for
subsequent retrievals via {@link #getEntity()}.

I interpreted that to imply that readEntity() had to be called before getEntity().

I don't know ...

@asoldano asoldano added the master label Aug 25, 2017

@asoldano asoldano referenced this pull request Sep 20, 2017

Merged

RESTEASY-1700 #1283

@asoldano

This comment has been minimized.

Member

asoldano commented Sep 20, 2017

@ronsigal @NicoNes , I've cleaned up this PR into #1283 and I'm leaning towards merging that. Any comment / concern?

@asoldano

This comment has been minimized.

Member

asoldano commented Sep 27, 2017

OK, I've merged #1283 .

@asoldano asoldano closed this Sep 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment