You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cxf.apache.org by nhtzr <gi...@git.apache.org> on 2017/03/31 23:46:24 UTC
[GitHub] cxf pull request #253: JAX-RS @Context fields throw NPE in OSGI hot deployed...
GitHub user nhtzr opened a pull request:
https://github.com/apache/cxf/pull/253
JAX-RS @Context fields throw NPE in OSGI hot deployed filters
- Add logic to inject using already set ThreadLocal proxy whenever possible.
- Update unit test, so getter and setter correspond to same field in Customer class.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/nhtzr/cxf 3.1.x-fixes
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cxf/pull/253.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 #253
----
commit cb69a6e68902c3cc7582e0d5e3e812810d3a0d9d
Author: Ezequiel Rosas <ez...@autodesk.com>
Date: 2017-03-31T16:47:22Z
Fix: Injection fails when OSGI filter is hot deployed.
Change: During setter injection, rely more on getter for referring to ThreadLocal
commit 301ad2f35f0f1f1b0e05f21d471e57fee206b93f
Author: Ezequiel Rosas <ez...@autodesk.com>
Date: 2017-03-31T23:33:29Z
Update unit test. setServletContext and getServletContext did not correspond to the same property
----
---
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] cxf pull request #253: [CXF-7309] JAX-RS @Context fields throw NPE in OSGI h...
Posted by nhtzr <gi...@git.apache.org>.
Github user nhtzr commented on a diff in the pull request:
https://github.com/apache/cxf/pull/253#discussion_r109824786
--- Diff: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/utils/InjectionUtils.java ---
@@ -1167,10 +1161,12 @@ public static void injectContextMethods(Object requestObject,
if (!cri.isSingleton()) {
InjectionUtils.injectThroughMethod(requestObject, method, o, message);
} else {
- ThreadLocalProxy<Object> proxy
- = (ThreadLocalProxy<Object>)cri.getContextSetterProxy(method);
+ Object proxy = extractFromSetter(requestObject, method);
+ if (!(proxy instanceof ThreadLocalProxy)) {
--- End diff --
Hi! Sorry for the late response!
The intention of this condition is to prevent NPE or ClassCastException that may happen in case `extractFromSetter()` fails or in case it returns some completely unexpected value.
The unexpected value could happen from the unfortunate coincidence in which a getter exists but it doesn't follow a Bean Property pattern.
Another possibility would be the property being overwritten by code outside cxf (That should be unlikely, but it is possible) and therefore no longer a proxy.
---
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] cxf pull request #253: [CXF-7309] JAX-RS @Context fields throw NPE in OSGI h...
Posted by nhtzr <gi...@git.apache.org>.
Github user nhtzr commented on a diff in the pull request:
https://github.com/apache/cxf/pull/253#discussion_r109967425
--- Diff: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/utils/InjectionUtils.java ---
@@ -1167,10 +1161,12 @@ public static void injectContextMethods(Object requestObject,
if (!cri.isSingleton()) {
InjectionUtils.injectThroughMethod(requestObject, method, o, message);
} else {
- ThreadLocalProxy<Object> proxy
- = (ThreadLocalProxy<Object>)cri.getContextSetterProxy(method);
+ Object proxy = extractFromSetter(requestObject, method);
+ if (!(proxy instanceof ThreadLocalProxy)) {
--- End diff --
I think it will be fine, because we are not messing with the unexpected value (so no code breakage outside) and Line 1169 is now unsafe unless we make that check (so no code breakage in this method).
In case of CXF-7309, the value returned here is another `ThreadLocalProxy` different from the one in `cri.getContextSetterProxy(method)`
This happens because of a singleton filter in a OSGI bundle. Whenever I hot re deploy that bundle, a new filter provider instance is created, and all OSGI proxy references (saved as `requestObject` here) are redirected to this new instance, but CXF has no way to know it has to update its internal `AbstractResourceInfo#getSetterProxyMap` to reflect that. All of this causes CXF to inject references into stale ThreadLocalProxies that no one is gonna use.
Hence the patch, which basically translates to "check getter first; check cri otherwise if the getter is not convenient"
As a side note: Now that you mention bean naming sense.
I reread the `@Context` documentation, and it does say that it should annotate bean property method (other options are a field or a parameter). :thinking:
Do you think it would be beneficial to drop the `AbstractResourceInfo#getSetterProxyMap` and instead rely on getters?
I think it would be good, but maybe the change is big enough for reconsideration.
I hope I addressed all you asked!
---
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] cxf pull request #253: [CXF-7309] JAX-RS @Context fields throw NPE in OSGI h...
Posted by sberyozkin <gi...@git.apache.org>.
Github user sberyozkin commented on a diff in the pull request:
https://github.com/apache/cxf/pull/253#discussion_r109885798
--- Diff: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/utils/InjectionUtils.java ---
@@ -1167,10 +1161,12 @@ public static void injectContextMethods(Object requestObject,
if (!cri.isSingleton()) {
InjectionUtils.injectThroughMethod(requestObject, method, o, message);
} else {
- ThreadLocalProxy<Object> proxy
- = (ThreadLocalProxy<Object>)cri.getContextSetterProxy(method);
+ Object proxy = extractFromSetter(requestObject, method);
+ if (!(proxy instanceof ThreadLocalProxy)) {
--- End diff --
Np, thanks. See if we see an unexpected value then it means there's a problem lying somewhere else, this is where my concern is. If some code somewhere depends on this 'unexpected' value then the proposed fix will break that code, right ? If a bean naming pattern is not followed then IMHO CXF should not worry about it. In case of CXF-7309, what is returned there ? Thanks
---
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] cxf pull request #253: [CXF-7309] JAX-RS @Context fields throw NPE in OSGI h...
Posted by sberyozkin <gi...@git.apache.org>.
Github user sberyozkin commented on a diff in the pull request:
https://github.com/apache/cxf/pull/253#discussion_r109372768
--- Diff: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/utils/InjectionUtils.java ---
@@ -1167,10 +1161,12 @@ public static void injectContextMethods(Object requestObject,
if (!cri.isSingleton()) {
InjectionUtils.injectThroughMethod(requestObject, method, o, message);
} else {
- ThreadLocalProxy<Object> proxy
- = (ThreadLocalProxy<Object>)cri.getContextSetterProxy(method);
+ Object proxy = extractFromSetter(requestObject, method);
+ if (!(proxy instanceof ThreadLocalProxy)) {
--- End diff --
Hi, can you explain please why this check is done, what else can the proxy be, apart from null or ThreadLocalProxy ? Thanks
---
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.
---