cxf-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From nhtzr <...@git.apache.org>
Subject [GitHub] cxf pull request #253: [CXF-7309] JAX-RS @Context fields throw NPE in OSGI h...
Date Wed, 05 Apr 2017 16:42:52 GMT
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.
---

Mime
View raw message