Bugzilla – Full Text Bug Listing
| Summary: | [Refactoring]: remove singletons and static fields in RWT | ||
|---|---|---|---|
| Product: | [RT] RAP | Reporter: | Frank Appel <fr.appel> |
| Component: | RWT | Assignee: | Frank Appel <fr.appel> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | enhancement | ||
| Priority: | P3 | CC: | gunnar |
| Version: | 1.4 | ||
| Target Milestone: | 1.4 M6 | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | |||
| Bug Depends on: | 341546, 342131, 342526 | ||
| Bug Blocks: | |||
| Attachments: | |||
Created attachment 189823 [details]
Annotated version of previous patch
Hi Frank, thanks a bunch for your patch! We found a few minor issues and added some comments tagged with // TODO [RWTContext]. Please use this patch for further development.
Created attachment 190026 [details]
Patch that removes singletons and static fields in RWT
I addressed most of the annotated proposals and hopefully solved all problems of the previous patch version with this new version.
Additionally I removed the RWTContextProvider concept and stored the RWTContext instance in the ServletContext. The RWTContext instance is retrieved via the ServiceContext at runtime - Besides a new getter on the ServiceContext class this was possible without much effort or impact on existing code.
Due to the naming discussion regarding the Singleton postfix I changed this to Instance now. But I agree with Rüdiger that this is only an intermediate solution, since the singleton facades should probably end up in the RWTContext class. But this will be part of a follow up refactoring.
As a result of this refactoring I could streamline or remove several classes like RWTDelegate, WorkbenchPlugin, RWTFixture etc. Please have a detailed look at the changes after applying the patch.
Please have also a special look at the TODOs marked with [RWTContext] that explain some of the current solutions, point to open ends or may need further discussion...
Created attachment 190154 [details]
Modified version of the previous patch
Hi Frank, thanks again for this patch. I'm fine with the issues mentioned in the RWTContext TODOs - I think we can address these after applying the patch step by step.
I compared the performance before and after the patch using a simple JMeter test run with only a single session. Unfortunately, the results suggest that this change doubles the time in the READ and PROCESS_ACTION phases. I add a screenshot that shows the times spent in the single phases in µs for five consecutive runs. In this test READ goes up from average ~13ms to ~26ms and PROCESS_ACTION from ~6ms to ~13ms. I did not conduct load test with many concurrent sessions, but I think that this increase of processing time will affect our performance significantly.
Could you have another view on possible improvements in RWTContext#getSingleton() ? Would it be an option to buffer the context in the session store? If there is no quick and simple fix, I'd suggest to go back to the singleton solution in order to get the patch into CVS quickly.
While reviewing the patch I restructured the WorkbenchPlugin a bit, because I found that the HttpContextExtensionService expects the service reference of the HttpService as first parameter, but we mistakenly passed the reference of the HttpContextExtService itself. I also did some minor changes and updated Copyright headers. I attach the modified patch as basis for further development.
Created attachment 190155 [details]
Screenshot of the performance test
Created attachment 190237 [details]
Performance improvements
Performance analysis showed a bottleneck in ServiceContext.getRWTContext(). Seems that reading the ServletContext from the underlying HttpSession is somewhat expensive. Because of this I followed Ralf's proposal and buffered the RWTContext. This turned out to be a little bit more complicated as I thought first, but in the end it seems to work.
@Ralf could you please revise the changes and see if the JMeter results improve (enough)?
Created attachment 190501 [details] Screenshot of performance tests after latest patch Thanks for the improvement, Frank - it obviously solved the performance problem. I've run the same performance tests again with latest version of the patch ( attachment 190237 [details] ) (I run the tests with slightly different settings than last time, so the absolute times may vary). The results do not show any noticeable difference anymore. I also conducted some tests with concurrent sessions, which also look good. Committed this patch to CVS HEAD. Created attachment 190511 [details]
ThemeAdapterUtil Instance
Hi Ralf, I've found another class variable used for buffering theme adapters. I added a patch with the changes against CVS-Head. The changes are made in the same manner as the other static buffer replacements. Hopefully there are not much more of those left in the code...
(In reply to comment #7) > Hi Ralf, I've found another class variable used for buffering theme adapters. I > added a patch with the changes against CVS-Head. The changes are made in the > same manner as the other static buffer replacements. Hi Frank, thanks for this patch. Applied it to CVS. > Hopefully there are not much more of those left in the code... Would be great if there was tool support to search for all static non-final fields in the code base, wouldn't it? But now that the groundwork is done, I'm confident that we'll find remaining issues one by one... I also refactored the RWTContext to be immutable by moving the supported types parameter to the constructor. There are still some // TODO [RWTContext] in the code and the pending refactoring to get rid of the class doubles discussed earlier. As agreed with Frank, I'll close this bug now anyway, as the essential changes are in CVS HEAD. Hi Frank, we just realized that with your patch the LCA tests can not be run separately anymore. Could you have another look at this? As an example, running ButtonLCA_Test as a separate test from the IDE gives me this stack trace: java.lang.RuntimeException: Could not load custom service handlers. at org.eclipse.rwt.internal.service.ServiceManagerInstance.init(ServiceManagerInstance.java:105) at org.eclipse.rwt.internal.service.ServiceManagerInstance.ensureInitialization(ServiceManagerInstance.java:62) at org.eclipse.rwt.internal.service.ServiceManagerInstance.registerServiceHandler(ServiceManagerInstance.java:111) at org.eclipse.rwt.internal.service.ServiceManager.registerServiceHandler(ServiceManager.java:27) at org.eclipse.rwt.internal.engine.RWTServletContextListener.registerUICallBackServiceHandler(RWTServletContextListener.java:421) at org.eclipse.rwt.internal.engine.RWTServletContextListener$ContextInitializer.run(RWTServletContextListener.java:113) at org.eclipse.rwt.internal.engine.RWTContextUtil.runWithInstance(RWTContextUtil.java:117) at org.eclipse.rwt.Fixture.createRWTContext(Fixture.java:119) at org.eclipse.rwt.Fixture.createRWTContext(Fixture.java:111) at org.eclipse.rwt.Fixture.setUp(Fixture.java:210) at org.eclipse.swt.internal.widgets.buttonkit.ButtonLCA_Test.setUp(ButtonLCA_Test.java:544) at junit.framework.TestCase.runBare(TestCase.java:128) at junit.framework.TestResult$1.protect(TestResult.java:106) at junit.framework.TestResult.runProtected(TestResult.java:124) at junit.framework.TestResult.run(TestResult.java:109) at junit.framework.TestCase.run(TestCase.java:120) at junit.framework.TestSuite.runTest(TestSuite.java:230) at junit.framework.TestSuite.run(TestSuite.java:225) at org.eclipse.jdt.internal.junit.runner.junit3.JUnit3TestReference.run(JUnit3TestReference.java:130) at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:467) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:683) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:390) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:197) Caused by: java.lang.ClassNotFoundException: org.eclipse.rwt.service.ServiceHandler_Test$CustomHandler at java.net.URLClassLoader$1.run(URLClassLoader.java:199) at java.security.AccessController.doPrivileged(Native Method) at java.net.URLClassLoader.findClass(URLClassLoader.java:187) at java.lang.ClassLoader.loadClass(ClassLoader.java:289) at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:274) at java.lang.ClassLoader.loadClass(ClassLoader.java:235) at org.eclipse.rwt.internal.service.ServiceManagerInstance.init(ServiceManagerInstance.java:95) ... 23 more (In reply to comment #9) Hi Ralf, I will have a look at it. Just as an intermediate solution to keep up the work going you may use use the following approach. Add the following TestSuite into org.eclipse.rwt.test.all to run the ButtonLCA_Test. public class StandaloneTestSuite { public static Test suite() { String msg = "Suite for fragment tests that depends on RWT bundle"; TestSuite suite = new TestSuite( msg ); // $JUnit-BEGIN$ suite.addTest( new TestSuite( ButtonLCA_Test.class ) ); // $JUnit-END$ return suite; } } I'll work on a solution ASAP. Created attachment 190671 [details]
change of servicehandler.xml location
I've created an attachment that solves the problem.
The problem was cause by the location of the servicehandler.xml in the fixture project. This is referenced by both: rwt tests and rwt.q07 fragment. The custom servicehandler class declared in that servicehandler declaration file is located at rwt tests. Running the rwt.q07 fragment tests (or only one of it) causes the problem, since the classloader can't find the custom handler class, since it isn't on the launch configuration's classpath.
So why did it work before the patch was applied? It seem's that I couldn't withstand to change the poor exception handling of the old ServiceManager static init Block
static {
try {
// do important stuff here :-)
} catch( final Throwable thr ) {
System.err.println( "Could not load custom service handlers." );
thr.printStackTrace();
}
into
private void init() {
try {
// now do the important stuff here :-)
} catch( RuntimeException rte ) {
throw rte;
} catch( Exception exception ) {
String msg = "Could not load custom service handlers.";
throw new RuntimeException( msg, exception );
}
}
So the exceptions probably also occured in the old q07.tests in standalone mode but were simply catched away. Running the all test suite in the first place I didn't recognize this problem and thought the latter changes are ok.
In my opinion they are ok and moving the declaration file is the appropriate solution. I don't want my system to start up if one or all of the declared servicehandlers could not be found. Folks what do you think?
(In reply to comment #11) Just one additional note. I take a look and find that a test for the exception handling was still missing - so I prepared one in case we accept the solution :-) Hi Frank, thanks for this quick fix! Your explanation makes sense to me, it's clear that the custom service handler defined in ServiceHandler_Test cannot be found when running the q07 tests. So moving the servicehandler.xml to the bundle that contains this class is the obvious solution. I also prefer an immediate failure when a service handler cannot be loaded - it doesn't seem to make much sense to log the error and continue in this case. However, I still don't understand how the System.err.println() and thr.printStackTrace() could have been swallowed before - but this is just of forensic interest... Committed the patch to CVS. A test would not hurt ;-), if you prepared one already, please add it. |
Created attachment 189454 [details] Patch that removes singletons and static fields in RWT Mostly as a remainder of the w4toolkit library we have singletons and static variables distributed throughout the RWT code that makes it difficult to test RWT itself and hinders to restart the library in context of a OSGi bundle without throwing away the classloader. Also you are not able to run more than one RWT instance in an OSGi system. I would like to do a couple of refactorings to improve this situation and make it possible to easily run RWT standalone and use multiple instances of RWT in OSGi (Think of two http-services with different ports - one for public use e.g. customers, one for internal use e.g. administration app). As a first step I introduce one singleton structure that replaces all existing singletons and global variables (hope I didn't miss one :-)) under the hood by referring to that structure. This means programmatic access to singletons hasn't change. Changing the singleton types would rupture the library to much. I applied a patch to this post with that implementation. Unfortunately this still leaves one singleton in place. Discussing the situation with Rüdiger he came up with the idea to add the instance of the singleton to the ServletContext instance of the actual web application. Access to this instance during the lifecycle execution of a request will be provided through the existing ServiceContext implementation. This will be the next refactoring step. Please take your time to think whether there are any obstacles with this approach that I'm missing. Names of classes are preliminary and probably will undergo some revisions on the way to the final solution...