The patch looks good to me. It seems to me that consensus here is that this is an acceptable change; unless there are any objections, I'm going to go ahead and commit your changes.
Best, Bill Lucy On Tue, Jan 12, 2016 at 10:32 AM, Hank Ibell <[email protected]> wrote: > I did some additional testing and I was not able to access another web > module's flows with the proposed patch installed. In my testing, trying to > do so just leads to a 404. > > Should anything else need to be investigated so that this issue can be > resolved? > > Regards, > Hank Ibell > > On Wed, Dec 16, 2015 at 9:45 PM, Leonardo Uribe <[email protected]> wrote: > >> Hi >> >> TA>> The CDI spec clearly states: >> TA>> The container instantiates a single instance of each extension at >> the beginning of >> TA>> the application initialization process and maintains a reference to >> it until >> TA>> the application shuts down. >> >> Ok, that's the only observation I had, so from my side the patch can be >> applied. >> >> regards, >> >> Leonardo Uribe >> >> 2015-12-16 17:57 GMT-05:00 Hank Ibell <[email protected]>: >> >>> After rereading Leonardo's response, he mentioned >>> >>> "The solution proposed in the patch cause a problem when two webapps >>> uses faces flow, because one app could find the flows of the other one >>> (variable for FlowBuilderCDIExtension)." >>> >>> I will test this when I have a chance. >>> >>> On Wed, Dec 16, 2015 at 5:24 PM, Hank Ibell <[email protected]> wrote: >>> >>>> I did another test with multiple web modules in an EAR with slightly >>>> different flows that have the same flow id. I was able to successfully >>>> enter the correct flow of each web module despite the flow ids being the >>>> same and with the extension being shared. >>>> >>>> Does anyone know if there is another case in mind that could break >>>> something? >>>> >>>> Regards, >>>> Hank Ibell >>>> >>>> On Wed, Dec 16, 2015 at 4:31 PM Thomas Andraschko < >>>> [email protected]> wrote: >>>> >>>>> Oh, i see. >>>>> The behavior is different in a EAR... >>>>> That could really break something. >>>>> >>>>> 2015-12-16 22:26 GMT+01:00 Thomas Andraschko < >>>>> [email protected]>: >>>>> >>>>>> The CDI spec clearly states: >>>>>> The container instantiates a single instance of each extension at the >>>>>> beginning of the application initialization process and maintains a >>>>>> reference to it until the application shuts down. >>>>>> >>>>>> So +1 for applying your patch. >>>>>> In DeltaSpike we have many Extensions which would behave weird if the >>>>>> Extensions instances would be shared across applications. >>>>>> >>>>>> 2015-12-16 22:12 GMT+01:00 Hank Ibell <[email protected]>: >>>>>> >>>>>>> Hello, >>>>>>> >>>>>>> I tested MyFaces on WildFly and WebSphere Liberty, and it looks like >>>>>>> FlowBuilderCDIExtension is not shared between different web applications >>>>>>> that are not in the same EAR. >>>>>>> >>>>>>> Here is what I see from WildFly: >>>>>>> ###################################### >>>>>>> WildFly: Deployment of an EAR with the two web modules: >>>>>>> FlowBuilderCDIExtension is shared between the two web modules >>>>>>> >>>>>>> 11:40:36,826 INFO [stdout] (MSC service thread 1-5) HWIBELL: >>>>>>> FlowBuilderCDIExtension: *Extension >>>>>>> org.apache.myfaces.flow.cdi.FlowBuilderCDIExtension@cb0c841b*: >>>>>>> 11:40:36,827 INFO [stdout] (MSC service thread 1-5) HWIBELL: >>>>>>> FlowBuilderCDIExtension: Found flow Producer for Producer Method [Flow] >>>>>>> with qualifiers [@FlowDefinition @Any] declared as >>>>>>> [[BackedAnnotatedMethod] >>>>>>> @Produces @FlowDefinition public >>>>>>> internal.FlowFactory.defineTestFlow(@FlowBuilderParameter FlowBuilder)] >>>>>>> declared on Managed Bean [class internal.FlowFactory] with qualifiers >>>>>>> [@Any >>>>>>> @Default] >>>>>>> 11:40:36,884 INFO [stdout] (MSC service thread 1-5) HWIBELL: >>>>>>> FlowBuilderCDIExtension: *Extension >>>>>>> org.apache.myfaces.flow.cdi.FlowBuilderCDIExtension@cb0c841b*: >>>>>>> 11:40:36,884 INFO [stdout] (MSC service thread 1-5) HWIBELL: >>>>>>> FlowBuilderCDIExtension: Found flow Producer for Producer Method [Flow] >>>>>>> with qualifiers [@FlowDefinition @Any] declared as >>>>>>> [[BackedAnnotatedMethod] >>>>>>> @Produces @FlowDefinition public >>>>>>> internal.FlowFactory.defineTestFlow(@FlowBuilderParameter FlowBuilder)] >>>>>>> declared on Managed Bean [class internal.FlowFactory] with qualifiers >>>>>>> [@Any >>>>>>> @Default] >>>>>>> ... >>>>>>> >>>>>>> ###################################### >>>>>>> WildFly: Deployment of two separate web modules: >>>>>>> FlowBuilderCDIExtension is different for each web application >>>>>>> >>>>>>> 2015-12-16 13:56:16,328 INFO [stdout] (MSC service thread 1-4) >>>>>>> HWIBELL: FlowBuilderCDIExtension: *Extension >>>>>>> org.apache.myfaces.flow.cdi.FlowBuilderCDIExtension@fee8cb50*: >>>>>>> 2015-12-16 13:56:16,331 INFO [stdout] (MSC service thread 1-4) >>>>>>> HWIBELL: FlowBuilderCDIExtension: Found flow Producer for Producer >>>>>>> Method >>>>>>> [Flow] with qualifiers [@FlowDefinition @Any] declared as >>>>>>> [[BackedAnnotatedMethod] @Produces @FlowDefinition public >>>>>>> internal1.FlowFactory.defineTestFlow(@FlowBuilderParameter FlowBuilder)] >>>>>>> declared on Managed Bean [class internal1.FlowFactory] with qualifiers >>>>>>> [@Any @Default] >>>>>>> 2015-12-16 13:56:16,430 INFO [stdout] (MSC service thread 1-3) >>>>>>> HWIBELL: FlowBuilderCDIExtension: *Extension >>>>>>> org.apache.myfaces.flow.cdi.FlowBuilderCDIExtension@988bd32f*: >>>>>>> 2015-12-16 13:56:16,430 INFO [stdout] (MSC service thread 1-3) >>>>>>> HWIBELL: FlowBuilderCDIExtension: Found flow Producer for Producer >>>>>>> Method >>>>>>> [Flow] with qualifiers [@FlowDefinition @Any] declared as >>>>>>> [[BackedAnnotatedMethod] @Produces @FlowDefinition public >>>>>>> internal2.FlowFactory.defineTestFlow(@FlowBuilderParameter FlowBuilder)] >>>>>>> declared on Managed Bean [class internal2.FlowFactory] with qualifiers >>>>>>> [@Any @Default] >>>>>>> ###################################### >>>>>>> >>>>>>> The results from WebSphere Liberty are the same. Does anyone the >>>>>>> best place to verify the lifetime of a CDI Extension instance? >>>>>>> >>>>>>> >>>>>>> >>>>>>> On Wed, Dec 16, 2015 at 10:52 AM, Leonardo Uribe <[email protected]> >>>>>>> wrote: >>>>>>> >>>>>>>> I'm not sure how the extension instance is created and its >>>>>>>> lifetime. That's not documented, so we need to check that. >>>>>>>> On Dec 16, 2015 4:03 AM, "Thomas Andraschko" < >>>>>>>> [email protected]> wrote: >>>>>>>> >>>>>>>>> Are you sure Leo? >>>>>>>>> FlowBuilderCDIExtension should exist per WebApp. >>>>>>>>> >>>>>>>>> 2015-12-15 22:39 GMT+01:00 Leonardo Uribe <[email protected]>: >>>>>>>>> >>>>>>>>>> Hi >>>>>>>>>> >>>>>>>>>> I remember the current solution works in a case where myfaces >>>>>>>>>> jars are shared by different web applications. Suppose a TomEE >>>>>>>>>> environment. >>>>>>>>>> The solution proposed in the patch cause a problem when two webapps >>>>>>>>>> uses >>>>>>>>>> faces flow, because one app could find the flows of the other one >>>>>>>>>> (variable >>>>>>>>>> for FlowBuilderCDIExtension). >>>>>>>>>> >>>>>>>>>> I don't have idea if the solution proposed works in that case, so >>>>>>>>>> we need to check that before apply it. >>>>>>>>>> >>>>>>>>>> regards, >>>>>>>>>> >>>>>>>>>> Leonardo Uribe >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> 2015-12-15 14:58 GMT-05:00 Hank Ibell <[email protected]>: >>>>>>>>>> >>>>>>>>>>> Hello Thomas, >>>>>>>>>>> >>>>>>>>>>> I did think injecting the FlowBuilderCDIExtension would work -- >>>>>>>>>>> I was quite surprised when it did. Also, after looking at the code >>>>>>>>>>> again, I >>>>>>>>>>> agree that the lists should be ArrayList instead. Thank you for the >>>>>>>>>>> quick >>>>>>>>>>> review and suggestions! >>>>>>>>>>> >>>>>>>>>>> The new patch has been attached to this email and to the JIRA. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Regards, >>>>>>>>>>> Hank Ibell >>>>>>>>>>> >>>>>>>>>>> On Mon, Dec 14, 2015 at 4:09 PM, Thomas Andraschko < >>>>>>>>>>> [email protected]> wrote: >>>>>>>>>>> >>>>>>>>>>>> Hi, >>>>>>>>>>>> >>>>>>>>>>>> i did a small review: >>>>>>>>>>>> >>>>>>>>>>>> 1) Why you don't use @Inject for the FlowBuilderCDIExtension in >>>>>>>>>>>> the FlowBuilderFactoryBean? >>>>>>>>>>>> 2) Why do you use CopyOnWriteArrayList? ArrayList should be >>>>>>>>>>>> fine as the both lists are AppScoped and should only be used on >>>>>>>>>>>> startup. >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> 2015-12-14 21:53 GMT+01:00 Hank Ibell <[email protected]>: >>>>>>>>>>>> >>>>>>>>>>>>> Hello Thomas, >>>>>>>>>>>>> >>>>>>>>>>>>> Thank you for the information. :) I will wait for Leo's review >>>>>>>>>>>>> then. >>>>>>>>>>>>> >>>>>>>>>>>>> Regards, >>>>>>>>>>>>> Hank Ibell >>>>>>>>>>>>> >>>>>>>>>>>>> On Mon, Dec 14, 2015 at 3:20 PM, Thomas Andraschko < >>>>>>>>>>>>> [email protected]> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>>> Hi, >>>>>>>>>>>>>> >>>>>>>>>>>>>> first of all: thanks for the patch. >>>>>>>>>>>>>> >>>>>>>>>>>>>> As the last release-vote just passed last week, please have a >>>>>>>>>>>>>> little patience. >>>>>>>>>>>>>> >>>>>>>>>>>>>> AFAIR the flows-feature was the developed by Leo, so it would >>>>>>>>>>>>>> be the best if he could review it. >>>>>>>>>>>>>> Otherwise i will check it. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Regards, >>>>>>>>>>>>>> Thomas >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> 2015-12-14 3:46 GMT+01:00 Hank Ibell <[email protected]>: >>>>>>>>>>>>>> >>>>>>>>>>>>>>> Hello, >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> It has been about a week since MYFACES-4022 [link >>>>>>>>>>>>>>> <https://issues.apache.org/jira/browse/MYFACES-4022>] has >>>>>>>>>>>>>>> been opened and a potential patch has been submitted. There has >>>>>>>>>>>>>>> been no >>>>>>>>>>>>>>> feedback on the issue however. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Is there anything else that is needed so that we can resolve >>>>>>>>>>>>>>> this issue as soon as possible? >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Regards, >>>>>>>>>>>>>>> Hank Ibell >>>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>> >>>>>> >>>>> >>> >> >
