Skip to content
This repository was archived by the owner on May 28, 2018. It is now read-only.

Conversation

@dadowns
Copy link

@dadowns dadowns commented Jan 26, 2015

InboundMessageContext.hasEntity() currently throws an IllegalStateException outside the try catch due to the extra call to entityContent.ensureNotClosed() if the entity stream is already closed.

This check is not needed here due to EntityInputStream.isEmpty() calling ensureNotClosed() internally.

@jerseyrobot
Copy link
Contributor

Can one of the admins verify this patch?

@AdamLindenthal
Copy link
Member

Jenkins, please test this patch.

@AdamLindenthal
Copy link
Member

Hi Dan,
many thanks for your contribution.

Before we can review your pull request, we need you to sign Oracle Contributor Agreement. It usually takes some time until the OCA is officially approved and listed on the page linked above (which is a relevant list for us).

Also, every pull request should have a issue opened in our bugtracker. It is a good practice to place mutually links from github to JIRA and vice versa.

More information about contributing to Jersey: https://jersey.java.net/scm.html#/Submitting_Patches_and_Contribute_Code

Thanks,
Adam

@dadowns
Copy link
Author

dadowns commented Jan 28, 2015

In the process of getting the OCA submitted, will update when that's done.

@dadowns
Copy link
Author

dadowns commented Apr 17, 2015

Ok, OCA is in (Daniel Downs) and issue https://java.net/jira/browse/JERSEY-2843 has been created. Sorry for the delay on that.

AdamLindenthal pushed a commit that referenced this pull request Jun 5, 2015
Make hasEntity() work with a closed stream.
@AdamLindenthal AdamLindenthal merged commit a45551b into javaee:master Jun 5, 2015
@AdamLindenthal
Copy link
Member

Merged, thanks for your contribution and this time sorry for the delay on our side.

Regards,
Adam

@dadowns
Copy link
Author

dadowns commented Jun 5, 2015

Thanks!

@mgajdos mgajdos added this to the 2.19 milestone Jun 8, 2015
@mgajdos
Copy link
Contributor

mgajdos commented Jun 10, 2015

Hi @dadowns,

sorry to inform you but this change has to be reverted – it's against the JAX-RS specification. See Javadoc to Response#hasEntity().

Michal

@mgajdos mgajdos removed this from the 2.19 milestone Jun 10, 2015
@dadowns
Copy link
Author

dadowns commented Jun 10, 2015

If that's the case then why have a try catch that explicitly catches the IllegalStateException and returns false? If that's the spec then.... ok, but not having any "safe" way to check if there's an entity seems broken in that every call to hasEntity must be wrapped in the same try catch that's inside hasEntity.

Since the spec wants a IllegalStateException then the whole method can just be:

return !entityContent.isEmpty();

as internally it'll throw the exception when the connection is closed.

@mgajdos
Copy link
Contributor

mgajdos commented Jun 15, 2015

Yeah, you're right about the simplification. Will look at it.

If you're not satisfied with what the spec offers, feel free to file an improvement request here. Thank you!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants