Hi Azeez, On Thu, Aug 30, 2012 at 8:46 PM, Afkham Azeez <[email protected]> wrote:
> > Adding such static methods is VERY wrong when you are living within an > OSGi container. Besides, this is bad OOP . CarbonServerManager is an > instantiated class, and it is wrong to statically inject objects into such > classes. The proper way to do this would be through OSGi services. This may > be working now and make everybody happy temporarily, but we have to design > & develop our components as much as possible keeping in mind that OSGi > bundles can be independently stopped & started. All sorts off issues such > as stale references etc. show up when bad practices like this are followed. > Please fix this in the proper way, before this API starts to pollute other > code. > > First, thank you for pointing out the issue and suggesting the possible correction. I've actually first considered the OSGi service based approach initially, but was thinking in the wrong line assuming that StartupFinalizerServiceComponent being the last service component to be initialized, it cannot register a service to be used by other components, which will result in a cyclic dependency. Anyways, by now looking at "CarbonCoreServiceComponent", I see there is a much cleaner approach used by making our interface the service interface itself, and register them as separate OSGi services. So I've now modified CarbonCoreServiceComponent for it to inject itself of "ServerStartupHandler" services (I've changed the name of the interface / it's methods to keep the naming consistency). So now, anyone requiring the required functionality, will need to implement the interface "org.wso2.carbon.core.ServerStartupHandler", and register it as an OSGi service, so its callback will be called when the server startup happens. The modified classes can be found at [1], [2], and [3]. > There is another issue I see here. We have this Carbon construct called > ListenerManagerRequiredService > , and unless all such services become available, the transports will not > be started. This ensures that no requests will be served by the server > until it has properly started up, because it is in an inconsistent state. > This CarbonStartupListener is the reverse of that. It notifies components > that the server has completed start up & is read for serving requests, > which means the server was started while some components were not properly > initialized which can lead to other problems if people incorrectly start > using this API. > I don't think this is a valid argument for our requirement, because if we go in that line, we can never get the required functionality we need. Basically yes, some components will not be fully initialized in specific areas pending notification of server startup; server startup here means, all the processing done from other components minus the logic that will be executed by these listeners. So yeah, it is the responsibility of the implementors of "ServerStartupHandler" to understand that at this occasion, it will be possible that others components also that uses this same listener mechanism would not have processed that section, and should do operations according to that, for example, a CarbonStartupListener implementation can safely assume that all other parts of the server would have initialized which anyway happened before we started using this listener, i.e. transports being up etc.. But things like, all the scheduled tasks of ntask being fully scheduled, cannot be guaranteed because, this is also done using another ServerStartupHandler. > Statis methods have been added even to StartupFinalizerServiceComponent. >> SCs are instantiated by the OSGi SCR & we are not supposed to do such >> things. Perhaps, there are other such badly written code segments, but we >> must fix those & avoid following bad practices. >> > This is the actual place I needed the static method at, which is the service component where we can see the startup finishing. Since I couldn't directly call this static method since it's not an exported class to the outside world, I proxied the call through another method in "CarbonServerManager" class. Anyways, now these parts are removed with the OSGi service based implementation. [1] https://svn.wso2.org/repos/wso2/carbon/kernel/branches/4.0.0/core/org.wso2.carbon.core/4.0.1/src/main/java/org/wso2/carbon/core/ServerStartupHandler.java [2] https://svn.wso2.org/repos/wso2/carbon/kernel/branches/4.0.0/core/org.wso2.carbon.core/4.0.1/src/main/java/org/wso2/carbon/core/internal/CarbonCoreServiceComponent.java [3] https://svn.wso2.org/repos/wso2/carbon/kernel/branches/4.0.0/core/org.wso2.carbon.core/4.0.1/src/main/java/org/wso2/carbon/core/internal/StartupFinalizerServiceComponent.java Cheers, Anjana. > >> >> > >> The changed code can be found in the following locations: >> >> * >> https://svn.wso2.org/repos/wso2/carbon/kernel/branches/4.0.0/core/org.wso2.carbon.core/4.0.1/src/main/java/org/wso2/carbon/core/init/CarbonServerManager.java >> * >> https://svn.wso2.org/repos/wso2/carbon/kernel/branches/4.0.0/core/org.wso2.carbon.core/4.0.1/src/main/java/org/wso2/carbon/core/init/CarbonStartupListener.java >> * >> https://svn.wso2.org/repos/wso2/carbon/kernel/branches/4.0.0/core/org.wso2.carbon.core/4.0.1/src/main/java/org/wso2/carbon/core/internal/StartupFinalizerServiceComponent.java >> >> And the usage of the above functionality in ntask can be found at: >> >> * >> https://svn.wso2.org/repos/wso2/carbon/platform/branches/4.0.0/components/ntask/org.wso2.carbon.ntask.core/4.0.1/src/main/java/org/wso2/carbon/ntask/core/service/impl/TaskServiceImpl.java >> >> Cheers, >> Anjana. >> -- >> *Anjana Fernando* >> Associate Technical Lead >> WSO2 Inc. | http://wso2.com >> lean . enterprise . middleware >> >> _______________________________________________ >> Dev mailing list >> [email protected] >> http://wso2.org/cgi-bin/mailman/listinfo/dev >> >> > > > -- > *Afkham Azeez* > Director of Architecture; WSO2, Inc.; http://wso2.com > Member; Apache Software Foundation; http://www.apache.org/ > * <http://www.apache.org/>** > email: **[email protected]* <[email protected]>* cell: +94 77 3320919 > blog: **http://blog.afkham.org* <http://blog.afkham.org>* > twitter: **http://twitter.com/afkham_azeez*<http://twitter.com/afkham_azeez> > * > linked-in: **http://lk.linkedin.com/in/afkhamazeez* > > * > * > *Lean . Enterprise . Middleware* > > -- *Anjana Fernando* Associate Technical Lead WSO2 Inc. | http://wso2.com lean . enterprise . middleware
_______________________________________________ Dev mailing list [email protected] http://wso2.org/cgi-bin/mailman/listinfo/dev
