You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by rdowner <gi...@git.apache.org> on 2015/07/01 13:47:03 UTC
[GitHub] incubator-brooklyn pull request: BROOKLYN-154: Don't call methods ...
GitHub user rdowner opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/730
BROOKLYN-154: Don't call methods that might not be there
This code attempted to call non-standard extensions on the `javax.ws.rs.core.Response` class. This change does the appropriate `instanceof` checks to ensure that no non-standard methods are called if
we have a plain old Response object.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/rdowner/incubator-brooklyn fix/BROOKLYN-154
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/730.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #730
----
commit b6a0023ab3d61bd7f0e9e49ffb9c762e8ac550bb
Author: Richard Downer <ri...@apache.org>
Date: 2015-07-01T11:43:05Z
BROOKLYN-154: Don't call methods that might not be there
This code attempted to call non-standard extensions on the
javax.ws.rs.core.Response class. This change does the appropriate
instanceof checks to ensure that no non-standard methods are called if
we have a plain old Response object.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: BROOKLYN-154: Don't call methods ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/730
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: BROOKLYN-154: Don't call methods ...
Posted by rdowner <gi...@git.apache.org>.
Github user rdowner commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/730#discussion_r34462093
--- Diff: usage/rest-client/src/main/java/brooklyn/util/http/BuiltResponsePreservingError.java ---
@@ -62,7 +64,8 @@ public BuiltResponsePreservingError(int status, Headers<Object> headers, Object
Exceptions.propagateIfFatal(e);
return new BuiltResponsePreservingError(status, headers, entity, new Annotation[0], e);
} finally {
- source.close();
+ if (source instanceof BaseClientResponse)
--- End diff --
@aledsage the thing is, there *isn't* a `close()` method on `Response` - at least not officially. There are multiple different versions of `Response` on the classpath and sometimes the "wrong" one (one without a `close()` method) ends up at the front. See the Jira issue for full details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: BROOKLYN-154: Don't call methods ...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/730#issuecomment-118084510
Merging this - @rdowner fixed this during testing of a downstream project. I presume there is good reason to guard the `close()` call in the finally block. We can add it back in if desired!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: BROOKLYN-154: Don't call methods ...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/730#issuecomment-118824307
What a mess that these deps include their own javax classes, and that they are different! I'd love to get a fix for that root problem, as even with this fix we seem at risk of similar errors, but I don't see how to do that easily.
This PR helps, and while we could reflectively looked for a `close()` method that seems like a worse band-aid, if we know that the `BaseClientResponse` is always what the resteasy API impl supplies then I'm fine with this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: BROOKLYN-154: Don't call methods ...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/730#discussion_r33762304
--- Diff: usage/rest-client/src/main/java/brooklyn/util/http/BuiltResponsePreservingError.java ---
@@ -62,7 +64,8 @@ public BuiltResponsePreservingError(int status, Headers<Object> headers, Object
Exceptions.propagateIfFatal(e);
return new BuiltResponsePreservingError(status, headers, entity, new Annotation[0], e);
} finally {
- source.close();
+ if (source instanceof BaseClientResponse)
--- End diff --
Why only call `close()` if it's an instance of `BaseClientResponse`? The close method is on `javax.ws.rs.core.Response`, so should we always call close?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---