Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignore (Auto)Closeable for interface-based proxy decision [SPR-15779] #20334

Closed
spring-issuemaster opened this issue Jul 16, 2017 · 7 comments

Comments

Projects
None yet
2 participants
@spring-issuemaster
Copy link
Collaborator

commented Jul 16, 2017

Illia Krokhmalov opened SPR-15779 and commented

I want to create bean org.elasticsearch.action.bulk.BulkProcessor for Batch Job with scope "job" Like:
@Bean
@JobScope
//@Scope(value = "job", proxyMode = ScopedProxyMode.TARGET_CLASS)
public BulkProcessor bulkProcessor() {
return BulkProcessor.builder(client(), esBulkListener()).build();
}
Looks like this bean will be wrapped in 2 proxies: CGLIB for bean main and for the scope needs to JDK Proxy. As result:
java.lang.ClassCastException: com.sun.proxy.$Proxy57 cannot be cast to org.elasticsearch.action.bulk.BulkProcessor
at org.elasticsearch.action.bulk.BulkProcessor$$FastClassBySpringCGLIB$$f71ea361.invoke(<generated>) ~[elasticsearch-2.4.5.jar:2.4.5]
at org.springframework.cglib.proxy.MethodProxy.invoke(MethodProxy.java:204) ~[spring-core-4.3.9.RELEASE.jar:4.3.9.RELEASE]...

BulkProcessor - is abstract class with interface java.io.Closeable and it can not be proxied by interface.
Full example attached.


Affects: 4.3.9

Attachments:

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 17, 2017

Juergen Hoeller commented

In general, you have to enforce proxyTargetClass=true in such a scenario, typically at the point where your AOP advice is being configured (e.g. @EnableTransactionManagement).

That said, we have a special mechanism for ignoring configuration callback interfaces: So if the only interface(s) implemented are InitializingBean, DisposableBean etc, we nevertheless choose a target-class proxy by default since there clearly is no user-supplied service interface. We should be ignoring Closeable and AutoCloseable there as well, so I'm repurposing this issue as such an enhancement.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 17, 2017

Illia Krokhmalov commented

I'm sorry, specially for this response I put in description commented line //@Scope(value = "job", proxyMode = ScopedProxyMode.TARGET_CLASS) . proxyTargetClass does not work. Please fix me if I'm not right. Maybe I do something wrong.
@JobScope - by default enforce proxyTargetClass, therefore I should define @Scope(value = "job", proxyMode = ScopedProxyMode.INTERFACES) for ItemWriter, it's okay.

In DefaultAopProxyFactory::createAopProxy I see, that for my bean will have 2 proxies. Configs:
1.
org.springframework.aop.framework.ProxyFactory: 3 interfaces [org.springframework.aop.scope.ScopedObject, java.io.Serializable, org.springframework.aop.framework.AopInfrastructureBean]; 1 advisors [DefaultIntroductionAdvisor: advice [org.springframework.aop.support.DelegatingIntroductionInterceptor@1cb37ee4]; interfaces [org.springframework.aop.scope.ScopedObject, java.io.Serializable]]; targetSource [SimpleBeanTargetSource for target bean 'scopedTarget.bulkProcessor']; proxyTargetClass=true; optimize=false; opaque=false; exposeProxy=false; frozen=false
For this config will be created CGLib proxy

org.springframework.aop.framework.ProxyFactory: 4 interfaces [java.io.Closeable, org.springframework.aop.scope.ScopedObject, java.io.Serializable, org.springframework.aop.framework.AopInfrastructureBean]; 1 advisors [DefaultIntroductionAdvisor: advice [org.springframework.aop.support.DelegatingIntroductionInterceptor@5ae95707]; interfaces [org.springframework.aop.scope.ScopedObject, java.io.Serializable]]; targetSource [SimpleBeanTargetSource for target bean 'scopedTarget.scopedTarget.bulkProcessor']; proxyTargetClass=false; optimize=false; opaque=false; exposeProxy=false; frozen=false
//For this config will be created JDK proxy.

Then first CGLIB proxy will try to call second JDK proxy.. - it's a problem. proxyMode = ScopedProxyMode.TARGET_CLASS influent only to the first proxy. I don't know how to change it for second type.
From my prospective, if you will open the code, you will see that code is absolutely normal and should work fine. This exception should not happen.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 17, 2017

Juergen Hoeller commented

There seem to be two scoped proxies involved in your case: one scoped proxy for another scoped proxy, ending up with scopedTarget.scopedTarget.bulkProcessor (note the repeated prefix)? That's very unusual and certainly not intended as a valid runtime arrangement; I'll try to look into that rather peculiar case specifically.

Ignoring java.io.Closeable for auto-proxy decisions makes sense in any case (just like we do for DisposableBean and co already). Whenever an inner proxy gets created without an explicit target class setting, it should never assume that Closeable is a valid service interface.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 19, 2017

Juergen Hoeller commented

I couldn't reproduce your double-scoped-proxy scenario in unit tests... This might be something rather specific to your Spring Batch usage there.

In any case, I'm rolling the Closeable change into tomorrow's 4.3.10 release, ignoring Closeable and AutoCloseable for interface-based proxy decisions. This should make general double proxying work even without target class enforcement. If this is not sufficient for your scenario, please raise an issue with Spring Batch to track the specific scenario there. They'll get back to us here anyway if they figure that the mishandling is on the core framework's side.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 20, 2017

Illia Krokhmalov commented

Juergen, I attached project for reproduce.. just start it.
From my prev comment, you can see that, there is no Closable in interfaces. I think Closable has nothing to do with this problem.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 20, 2017

Juergen Hoeller commented

I should have been clearer there: In auto-proxying scenarios, a case like BulkProcessor can indeed lead to interface-based proxying by default just because of the Closeable interface that it implements. Ignoring that interface as a configuration callback interface is worthwhile in any case, even if it does not directly affect your scenario. I originally thought you had a target-class scoped proxy against an interface-based AOP proxy but that doesn't seem to be the case; it rather looks like a double scoping scenario for some reason. Nevertheless, the original conclusion I applied here is worth resolving and shipping, so I'd like to mark this issue as resolved for that particular purpose... and ship it that change in 4.3.10 this afternoon.

As for your repro case, it does fail for me as well in that Boot+Batch environment. I just failed to isolate its double-scoping in a core framework unit test, and I'll have to wrap it up here for 4.3.10 today. The double-scoping is really odd (never seen before) and possibly caused by some interfering Batch configuration mechanism, or by something else that's not obvious to me yet.

Sorry for repurposing the issue here for my change above but with the existing commits that's the easiest way out. At this point, please create a separate report for the Spring Batch project (https://jira.spring.io/browse/BATCH/). They'll certainly create a more specific issue back for the Spring Framework project if the double-scoping actually needs to be addressed at that level.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 20, 2017

Illia Krokhmalov commented

Okay.
You right. The easy way to fix it -
@Bean
public static JobScope jobScope() {
JobScope jobScope = new JobScope();
jobScope.setProxyTargetClass(true); //This line
return jobScope;
}
JobScope creates addition proxy. I'm not sure why, why it's need - proxy for proxy. And it's not so good, that I should find all places for configure proxyTargetClass.. (on the annotation for bean and in the scope configuration) In any case, would be good improve/fix it. Thanks for your time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.