Hi No objections from my side. Please commit the changes.
regards, Leonardo Uribe 2016-01-12 14:23 GMT-05:00 Bill Lucy <[email protected]>: > 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 >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>> >>> >> >
