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

WW-4827 Not fully initialized ObjectFactory tries to create beans #153

Merged
merged 1 commit into from
Aug 3, 2017

Conversation

aleksandr-m
Copy link
Contributor

Inject Container in constructor of the ObjectFactory.

Can someone test cdi, osgi and plexus plugins.

Better ideas how to fix the issue are welcome. :)

@lukaszlenart
Copy link
Member

No better idea and I think we can assume when you need to inject the Container you should always do it in a constructor.

@yasserzamani
Copy link
Member

yasserzamani commented Aug 1, 2017

To keep backward-compatible if is very important, I think we may have following:

protected Object injectInternalBeans(Object obj) {
    if (obj != null) {
        if(container != null) container.inject(obj);
        else myPrivateInjectQueue.add(obj);
    }
    return obj;
}
   		
@Inject		
public void setContainer(Container container) {		
    this.container = container;
    if(null != this.container && myPrivateInjectQueue.getCount()>0)
    {
        foreach(Object obj in myPrivateInjectQueue) this.container.inject(obj);
        myPrivateInjectQueue.clear();
    }		
}

However I never tried and tested it but you can if you think it makes sense.

@lukaszlenart
Copy link
Member

I'm not sure if this is a good idea ... basically we need a @PostConstruct mechanism in Struts DI - this can be easily achieved by switching to Guice 3 and using CDI annotations.

@lukaszlenart
Copy link
Member

Hm... give me few minutes, maybe I will be able add support for it ;-)

@lukaszlenart
Copy link
Member

I have opened #155 - what do you think? @aleksandr-m can you test it locally as I do not have the exact setup.

@aleksandr-m
Copy link
Contributor Author

aleksandr-m commented Aug 1, 2017

Nope, #155 doesn't seem have any effect.

Add following files to project, run under *nix, execute some action which leads to the JSP, localizedTextProvider should not be null in system outs.

xwork-conversion.properties;

java.util.Date=com.DateConverter

JSP:

<s:bean var="d" name="java.util.Date" />
<s:property value="#d" />

DateConverter:

public class DateConverter extends StrutsTypeConverter {

    private LocalizedTextProvider localizedTextProvider;

    @Inject
    public void setLocalizedTextProvider(LocalizedTextProvider localizedTextProvider) {
        this.localizedTextProvider = localizedTextProvider;
    }

    @Override
    public Object convertFromString(Map context, String[] values, Class toClass) {
        System.out.println("DateConverter.convertFromString() localizedTextProvider=" +
                                      localizedTextProvider);
        return null;
    }

    @Override
    public String convertToString(Map context, Object obj) {
        System.out.println("DateConverter.convertToString() localizedTextProvider=" +
                                      localizedTextProvider);
        return "";
    }
}

@lukaszlenart
Copy link
Member

hm... but this example works for both branches - master and this:

DateConverter.convertToString() localizedTextProvider=com.opensymphony.xwork2.util.StrutsLocalizedTextProvider@342e1b8b

@lukaszlenart
Copy link
Member

(on OSX)

@lukaszlenart
Copy link
Member

One more question: JDK version?

@aleksandr-m
Copy link
Contributor Author

Ubuntu, oracle jdk8.

If following code prints setContainer before other setters (especially before setInterceptorFactory) it will work. On Ubuntu using oracle jdk8 setContainer is printed way below other setters and injection fails.

Method[] methods = ObjectFactory.class.getDeclaredMethods();
for (Method m : methods) {
    System.out.println(m);
}

@lukaszlenart
Copy link
Member

See these screenshots

master

2017-08-01_2119

this branch

2017-08-01_2124

as you see, on the master branch the object isn't fully initialised but when init() method is called when using this branch, the object got fully initialised.

Maybe you have discovered another place where there is some work done in setter which should be moved into the init() method with the PostInit interface.

@aleksandr-m
Copy link
Contributor Author

aleksandr-m commented Aug 1, 2017

It isn't about XWorkConverter per se. In #155 the injection of ConversionPropertiesProcessor still happens and TypeConverterCreator is injected next and ObjectFactory#buildBean is called and container is null in ObjectFactory.

BTW ObjectFactory.class.getDeclaredMethods() on Ubuntu, openJDK 7 also prints setContainer as the last method.

@lukaszlenart
Copy link
Member

... but where the ObjectFactory#buildBean is called? It should only be called when building non-internal beans and when all the internal beans were already instantiated. Can you post a call trace?

@lukaszlenart
Copy link
Member

I have changed code a bit, can you try to test it on your side?

@aleksandr-m
Copy link
Contributor Author

aleksandr-m commented Aug 1, 2017

With the new code in #155 the custom type converter isn't initialized at all. :(

With previous code in #155 the call trace of DateConverter is:

Daemon Thread [localhost-startStop-1] (Suspended (breakpoint at line 31 in DateConverter))	
	owns: ContainerImpl  (id=64)	
	owns: DefaultConfiguration  (id=65)	
	owns: ConfigurationManager  (id=66)	
	owns: HashMap<K,V>  (id=67)	
	owns: StandardContext  (id=68)	
	DateConverter.<init>() line: 31	
	NativeConstructorAccessorImpl.newInstance0(Constructor<?>, Object[]) line: not available [native method]	
	NativeConstructorAccessorImpl.newInstance(Object[]) line: 62	
	DelegatingConstructorAccessorImpl.newInstance(Object[]) line: 45	
	Constructor<T>.newInstance(Object...) line: 423	
	Class<T>.newInstance() line: 442	
	ObjectFactory.buildBean(Class, Map<String,Object>) line: 149	
	ObjectFactory.buildBean(String, Map<String,Object>, boolean) line: 186	
	ObjectFactory.buildBean(String, Map<String,Object>) line: 172	
	DefaultTypeConverterCreator.createTypeConverter(String) line: 22	
	DefaultConversionPropertiesProcessor.loadConversionProperties(String, boolean) line: 74	
	DefaultConversionPropertiesProcessor.process(String) line: 52	
	XWorkConverter.init() line: 199	
	ContainerImpl$8.call(InternalContext) line: 569	
	ContainerImpl.callInContext(ContextualCallable<T>) line: 637	
	ContainerImpl.inject(Class<T>) line: 564	
	LocatableFactory<T>.create(Context) line: 32	
	ContainerBuilder$4.create(InternalContext) line: 129	
	Scope$2$1.create(InternalContext) line: 52	
	ContainerImpl$ParameterInjector<T>.inject(Member, InternalContext) line: 493	
	ContainerImpl.getParameters(Member, InternalContext, ContainerImpl$ParameterInjector[]) line: 507	
	ContainerImpl.access$0(Member, InternalContext, ContainerImpl$ParameterInjector[]) line: 500	
	ContainerImpl$MethodInjector.inject(InternalContext, Object) line: 332	
	ContainerImpl$ConstructorInjector<T>.construct(InternalContext, Class<? super T>) line: 467	
	ContainerImpl.inject(Class<T>, InternalContext) line: 522	
	ContainerImpl$8.call(InternalContext) line: 566	
	ContainerImpl.callInContext(ContextualCallable<T>) line: 637	
	ContainerImpl.inject(Class<T>) line: 564	
	LocatableFactory<T>.create(Context) line: 32	
	ContainerBuilder$4.create(InternalContext) line: 129	
	Scope$2$1.create(InternalContext) line: 52	
	ContainerImpl$ParameterInjector<T>.inject(Member, InternalContext) line: 493	
	ContainerImpl.getParameters(Member, InternalContext, ContainerImpl$ParameterInjector[]) line: 507	
	ContainerImpl.access$0(Member, InternalContext, ContainerImpl$ParameterInjector[]) line: 500	
	ContainerImpl$MethodInjector.inject(InternalContext, Object) line: 332	
	ContainerImpl$ConstructorInjector<T>.construct(InternalContext, Class<? super T>) line: 467	
	ContainerImpl.inject(Class<T>, InternalContext) line: 522	
	ContainerImpl$8.call(InternalContext) line: 566	
	ContainerImpl.callInContext(ContextualCallable<T>) line: 637	
	ContainerImpl.inject(Class<T>) line: 564	
	LocatableFactory<T>.create(Context) line: 32	
	ContainerBuilder$4.create(InternalContext) line: 129	
	Scope$2$1.create(InternalContext) line: 52	
	ContainerImpl$ParameterInjector<T>.inject(Member, InternalContext) line: 493	
	ContainerImpl.getParameters(Member, InternalContext, ContainerImpl$ParameterInjector[]) line: 507	
	ContainerImpl.access$0(Member, InternalContext, ContainerImpl$ParameterInjector[]) line: 500	
	ContainerImpl$MethodInjector.inject(InternalContext, Object) line: 332	
	ContainerImpl$ConstructorInjector<T>.construct(InternalContext, Class<? super T>) line: 467	
	ContainerImpl.inject(Class<T>, InternalContext) line: 522	
	ContainerImpl$8.call(InternalContext) line: 566	
	ContainerImpl.callInContext(ContextualCallable<T>) line: 637	
	ContainerImpl.inject(Class<T>) line: 564	
	LocatableFactory<T>.create(Context) line: 32	
	ContainerBuilder$4.create(InternalContext) line: 129	
	Scope$2$1.create(InternalContext) line: 52	
	ContainerImpl$ParameterInjector<T>.inject(Member, InternalContext) line: 493	
	ContainerImpl.getParameters(Member, InternalContext, ContainerImpl$ParameterInjector[]) line: 507	
	ContainerImpl.access$0(Member, InternalContext, ContainerImpl$ParameterInjector[]) line: 500	
	ContainerImpl$MethodInjector.inject(InternalContext, Object) line: 332	
	ContainerImpl$ConstructorInjector<T>.construct(InternalContext, Class<? super T>) line: 467	
	ContainerImpl.inject(Class<T>, InternalContext) line: 522	
	ContainerImpl$8.call(InternalContext) line: 566	
	ContainerImpl.callInContext(ContextualCallable<T>) line: 637	
	ContainerImpl.inject(Class<T>) line: 564	
	LocatableFactory<T>.create(Context) line: 32	
	ContainerBuilder$4.create(InternalContext) line: 129	
	Scope$2$1.create(InternalContext) line: 52	
	ContainerImpl.getInstance(Class<T>, String, InternalContext) line: 536	
	ContainerImpl.getInstance(Class<T>, InternalContext) line: 546	
	ContainerImpl$10.call(InternalContext) line: 594	
	ContainerImpl.callInContext(ContextualCallable<T>) line: 628	
	ContainerImpl.getInstance(Class<T>) line: 592	
	DefaultConfiguration.reloadContainer(List<ContainerProvider>) line: 181	
	ConfigurationManager.getConfiguration() line: 63	
	Dispatcher.getContainer() line: 960	
	Dispatcher.init_PreloadConfiguration() line: 466	
	Dispatcher.init() line: 499	
	InitOperations.initDispatcher(HostConfig) line: 75	
	StrutsPrepareAndExecuteFilter.init(FilterConfig) line: 63	
	ApplicationFilterConfig.initFilter() line: 279	
	ApplicationFilterConfig.getFilter() line: 260	
	ApplicationFilterConfig.<init>(Context, FilterDef) line: 105	
	StandardContext.filterStart() line: 4958	
	StandardContext.startInternal() line: 5652	
	StandardContext(LifecycleBase).start() line: 145	
	ContainerBase$StartChild.call() line: 1571	
	ContainerBase$StartChild.call() line: 1561	
	FutureTask<V>.run() line: 266	
	ThreadPoolExecutor.runWorker(ThreadPoolExecutor$Worker) line: 1142	
	ThreadPoolExecutor$Worker.run() line: 617	
	Thread.run() line: 748	

@lukaszlenart
Copy link
Member

With the new code in #155 the custom type converter isn't initialized at all. :(

w00t!

@lukaszlenart
Copy link
Member

Would it be possible to share an application that replicates this behaviour? Sorry for bothering you but I want to understand what's going on :)

@lukaszlenart
Copy link
Member

I don't see any other options now to merge this. @aleksandr-m could you prepare that example app? I will be able to investigate what's wrong with my solution ;-)

@aleksandr-m
Copy link
Contributor Author

Sure. No problem.

Test project - https://github.com/aleksandr-m/struts2-objectfactory-container/

This commit adds custom date converter - aleksandr-m/struts2-objectfactory-container@49406c1.

This commit adds local copy of ContainerImpl in order to reproduce the issue in various environments - aleksandr-m/struts2-objectfactory-container@e3fb324

@lukaszlenart
Copy link
Member

Thanks a lot :) I see now what's going on but have no idea why :\

@asfgit asfgit merged commit 6f91d07 into apache:master Aug 3, 2017
@yasserzamani
Copy link
Member

@aleksandr-m , I modified ObjectFactory as below

...
    private List<Object> myPrivateInjectQueue;
...
    @Inject
    public void setContainer(Container container) {
        this.container = container;
        if(null != this.container && null!=myPrivateInjectQueue && myPrivateInjectQueue.size()>0)
        {
            for(Object obj : myPrivateInjectQueue) this.container.inject(obj);
            myPrivateInjectQueue.clear();
            myPrivateInjectQueue=null;
        }
    }
...
    protected Object injectInternalBeans(Object obj) {
        if (obj != null) {
            if(container != null) container.inject(obj);
            else {
                if(null==myPrivateInjectQueue)myPrivateInjectQueue=new ArrayList<Object>();
                myPrivateInjectQueue.add(obj);}
        }
        return obj;
    }

in your sample I got following ok result :)

moving setContainer method to the last element of the array
DateConverter.DateConverter()
moving setContainer method to the last element of the array
DateConverter.DateConverter()
DateConverter.convertToString() localizedTextProvider=com.opensymphony.xwork2.util.StrutsLocalizedTextProvider@3f6f10fa

@aleksandr-m
Copy link
Contributor Author

@yasserzamani Looks like a hack :) I would rather leave as it is, then to introduce that. If container is needed then it should be injected in the constructor. Take a look at spring object factory.

@yasserzamani
Copy link
Member

yasserzamani commented Aug 4, 2017

:) Yes I know, just a bit worry about backward compatibility between minor versions.

I also am studying your sample and @lukaszlenart 's pr to understand what is going on as I think same issues may exist because it seems an object's injections is not an atomic operations! shouldn't be?

@yasserzamani
Copy link
Member

Thanks a lot :) I see now what's going on but have no idea why :\

@lukaszlenart , as I debug, the trace is object factory creation then set1 (inject it's 1st property) then set2 (inject it's 2nd property) ... then ... then setK (inject it's Knd property) then your PostInit.init() which reaches this object factory with null container ... then setN (inject it's Nnd property) finally setContainer (inject it's container property).

As you see, if you would like to make it works, the ObjectFactory itself has to implement PostInit and do followings in it's init():

processRequired("struts-default-conversion.properties");
process("xwork-conversion.properties");

@lukaszlenart
Copy link
Member

The case is that on second container initialisation we do not create singletons upfront ... I have tried to change that but got some problems, not sure why. I must re-think how we initialise Struts internals ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants