javaee / hk2

A light-weight and dynamic dependency injection framework
https://javaee.github.io/hk2
Other
112 stars 83 forks source link

Avoid using proxies when injecting into the same scope #150

Closed glassfishrobot closed 10 years ago

glassfishrobot commented 11 years ago

This was originally reported as Jersey bug JERSEY-1761. I believe proxies are being injected all the time even if the scope of injected and target instances is same.

Here is an example:

// the following bean is request scoped: @Path("/")
public class AsyncResource {

    @Context UriInfo ui;  // this is request scoped proxy 
    @GET
    public void asyncGet(@Suspended final AsyncResponse response) {

          // now suspend and resume later on with
          Executors.newSingleThreadExecutor().submit(new Runnable() {

  @Override
  public void run() {   // the following code is run in a different thread outside of the request scope
        String hereWeGo = longRunningOp();  // does not matter 
        UriBuilder baseUriBuilder = ui.getBaseUriBuilder();    // here it breaks, but it would not if ui was not a proxy 
        ar.resume(baseUriBuilder.path(hereWeGo).build());
  }
           });
    }
}

Affected Versions

[2.1.*]

glassfishrobot commented 11 years ago

Reported by @japod

glassfishrobot commented 11 years ago

@jwells131313 said: I am changing this to an improvement. We might be able to add the concept of LazyProxy for those cases where proxies are being used for lazyiness, not for lifecycle.

glassfishrobot commented 11 years ago

aaronjwhiteside said: How is this an improvement? It's clearly a bug.

@Path("/")
public class TestResource {

    @GET
    public String testGet(@Context UriInfo uriInfo) {
        return "Hello World";
    }
}

If I call this method from multiple threads, they all contend for a single lock in:

java.lang.Thread.State: BLOCKED (on object monitor)
    at net.sf.cglib.core.AbstractClassGenerator.create(AbstractClassGenerator.java:190)
    - waiting to lock <0x00000000e220dc10> (a net.sf.cglib.core.AbstractClassGenerator$Source)
    at net.sf.cglib.proxy.Enhancer.createHelper(Enhancer.java:377)
    at net.sf.cglib.proxy.Enhancer.create(Enhancer.java:285)
    at org.jvnet.hk2.internal.Utilities$6.run(Utilities.java:2117)
    at java.security.AccessController.doPrivileged(Native Method)
    at org.jvnet.hk2.internal.Utilities.secureCreate(Utilities.java:2106)
    at org.jvnet.hk2.internal.Utilities.createService(Utilities.java:2157)
    at org.jvnet.hk2.internal.ServiceHandleImpl.getService(ServiceHandleImpl.java:93)
    - locked <0x00000000fa26f918> (a java.lang.Object)
    at org.glassfish.jersey.internal.inject.ContextInjectionResolver.resolve(ContextInjectionResolver.java:104)
    at org.glassfish.jersey.server.internal.inject.DelegatedInjectionValueFactoryProvider$1.provide(DelegatedInjectionValueFactoryProvider.java:96)
.....

So essentially CGLIB via HK2 is making this testGet() method single threaded. And it gets worse, if I happen to have another resource method with a @Context parameter that too is using the same CGLIB lock, so my entire application is now essentially single threaded.

glassfishrobot commented 11 years ago

@jwells131313 said: There are two reasons people use proxy's.

1. Because the lifecycle of objects in different scopes are different 2. Because they want the object initialized lazily

Right now @Proxiable encompasses both use cases.

The enhancement would be to add some mechanism such that you could mark proxiable things as being used for lazy or not. IF something is marked as "proxy me and I don't care about the lazy use case" then we could start NOT proxying things that are being injected into the same scope.

However, I'm also concerned about the stack trace you sent, because a proxy should ONLY be created the first time it is touched, not every time. So I don't think your claim of single-threadedness because of proxy creation should be true...

glassfishrobot commented 11 years ago

@japod said: I can confirm, Jersey's use case is the first one: because the lifecycle of objects in different scopes are different. We do not care about lazy initialisation in Jersey in this case. It means for the original Jersey issue, the solution as suggested by John should work for us. So if HK2 provided an option, "lazy use case not relevant here", Jersey would love to use it in order to have a non-proxy, "original" injectee injected when the injectee/injected scopes match (request scope in this case).

glassfishrobot commented 11 years ago

@jwells131313 said: The following enhancment has been done:

1. @Proxiable now takes a boolean variable proxyForSameScope. If proxyForSameScope is true (the default) then proxies will be used even when injecting into the same scope. If proxyForSameScope is false then proxies will NOT be used when injecting into the same scope 2. This behavior can be overridden on a per-service basis by setting the field ProxyForSameScope on a Descriptor. Whatever is in the Descriptor wins over whatever the value is for @Proxiable 3. #2 works even for those services that are in non-proxiable scopes but which are proxiable anyway 4. An annotation (@ProxyForSameScope) has been added so that it can be specified on classes during automatic class analysis 5. The Binding and BuilderHelper EDSL have been updated to set the value of ProxyForSameScope

I think that should do it for this mini-feature. Try it out and see if it works for you.

glassfishrobot commented 11 years ago

@japod said: John, thanks, the mini feature seems to work just fine for jersey 2. I have just tested with hk2 version 2.2.0-b04.

Is there any chance this could be back ported to hk2 2.1.x space? Shall i file a new report for that?

glassfishrobot commented 11 years ago

Issue-Links: blocks JERSEY-1761 JERSEY-1905

glassfishrobot commented 7 years ago

This issue was imported from java.net JIRA HK2-106

glassfishrobot commented 10 years ago

Marked as fixed on Friday, December 6th 2013, 4:02:54 am