Bugzilla – Full Text Bug Listing
| Summary: | J2EEDeployableFactory needs to clear server cache when changed | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [WebTools] WTP Java EE Tools | Reporter: | Rob Stryker <stryker> | ||||||
| Component: | jst.j2ee | Assignee: | Rob Stryker <stryker> | ||||||
| Status: | RESOLVED FIXED | QA Contact: | Chuck Bridgham <cbridgha> | ||||||
| Severity: | major | ||||||||
| Priority: | P3 | CC: | ccc, david_williams, eyuen7, jasonpet, keith.chong.ca | ||||||
| Version: | 3.2 | Flags: | david_williams:
pmc_approved+
cbridgha: pmc_approved? (raghunathan.srinivasan) cbridgha: pmc_approved? (naci.dai) cbridgha: pmc_approved? (deboer) cbridgha: pmc_approved? (neil.hauge) cbridgha: pmc_approved? (kaloyan) cbridgha: review+ |
||||||
| Target Milestone: | 3.2.1 | ||||||||
| Hardware: | PC | ||||||||
| OS: | Linux | ||||||||
| Whiteboard: | PMC_approved | ||||||||
| Attachments: |
|
||||||||
This will need some approval... Chuck: Whats the schedule for 3.2.1? Can this make it in? If caching is enabled, this is very very important to have along with it to prevent null publishes from happening. Created attachment 174983 [details]
one line fix, tell all servers to update their module instances
The following testcase (in pseudo code) fails without this patch: 1) Create IServer through whatever you require to do so 2) Create dynamic web project 3) add text file to dynamic web project inside webcontent folder 4) add module to server, keep module stored in local variable IModule mod; 5) publish to the server, notice that server behaviour delegate gets a full publish request (added module) 6) CHANGE the text file's contents 7) Call Server.getPublishResourceDelta(mod), note ZERO length delta 8) Call Server.getPublishResourceDelta( ServerUtil.findModule(mod.getId()), note 1-length delta 9) Call Server.publish(mod), note that the server / publisher will see a ZERO-length delta Basically, once the text file is changed, a stack trace goes from the autobuilder into the ResourceManager, and eventually calling J2EEDeployableFactory.createModules(IProject). So just the fact that the autobuilder runs triggers the module factory to create a NEW instance of it's module AND module delegate. *AFTER* this happens, the j2ee deployable factory gets the resource change event it listened to for the changed text file, and finds its module delegate for the web project's module. Unfortunately, this is a NEW IModule and a NEW ModuleDelegate, so these NEW instances get told to refresh their cache. This means in the test case above, the old module (mod) is stale and is actually not to be found inside the ModuleFactory. The old module has still cached it's results. The server object still holds a reference to this stale module, because our module factory did not inform the server to update its module references. This one line fix is almost guaranteed to be safe. All it does is force the server to re-request each of its modules from their respective factories once more, to verify that they have the most up-to-date instance of the module for a given ID. This is related to Bug 304673 which was resolved and closed. Created attachment 174992 [details]
Test case plugin demonstrating the bug
File consists of 2 mock server classes, several utility classes for creating projects, copying files, etc, and a test case.
Test case is named 'JSTDeploymentTester.java'
Some performance concerns on the fix. Based on the change, that means all the cache, including all module within all the servers, will be cleared and recalculated every time when the resources are changed. Just 1 single file change clear all caches (including the unaffect resources). If that is the case, how useful are the module cache becomes? Hi Elson: The module cache's biggest gains in performance are during publish events and repeated calls to members() and getModules() (child module references). During a publish event, members() and getModules() are called several times, up to 6, both to calculate a delta, to perform incremental publishing, etc. So the large amount of performance gains during a publish should still be present. The tree would only need to be traversed once, and those results would be used for the subsequent 6 calls. (In all likelihood, the cache had already been refreshed by the most recent workspace change already, which means the publish would require zero additional calls). I've just tested this with the UI and the issue is not replicatable when using the UI... it is only replicatable when using the API directly. What this means to me is that something in the UI is already traversing the new module delegate every time a workspace change is happening. What *this* means is that there would be virtually zero performance hit. If something is *already* traversing the new cleared module after each workspace change and getting accurate results, then this change should only serve to fetch the already-created and already-traversed module! It's possible this has zero performance hit at all when the UI is being used... I have heard that there is concern that the module's resource cache is cleared on every workspace, even for modules that are not deployed. From my quick test, an object that is not deployed *will* in fact have its resource cache cleaned, however, on resource changes, nobody asks this module for its list of members because the object is not deployed and no one cares what its members are. Today the most aggressive 'poller' of a module to get its members() list is the Servers view (and the publish method). The servers view has to check on every resource change whether the module is synchronized or not. For modules that are not deployed, the servers view does *not* need to calculate a republish state. I see no other constant pollers of the members() or getModules() methods for non-published projects. This means non-deployed projects should suffer a basic no-op in this situation, and projects that *are* deployed should rightly be refreshed. I have just tested a workaround in my adopter product. Instead of trusting the server to have performed a delta on the proper module instance. The point I perform this delta is during the publish call to ServerBehaviourDelegate.publishModule(int kind, int deltaKind, IModule[] module, IProgressMonitor monitor) throws CoreException In this section of code, I have attempted to do the following: IModule newModule = ServerUtil.getModule(module[module.length-1].getId()); module[module.length-1] = newModule; delta = getPublishedResourceDelta(module); However this workaround does *not* work. It seems that at the beginning of the publish call, using the incorrect module instance, the server also caches the publish info delta. So when I try to get the delta for the newest module instance, it gives me the recently cached delta from the incorrect module. I guess this is now definitely a regression... I would like to point out how many caches we have going on here, which may lead to confusion. 1) First, a module factory keeps a reference to its current module instances and delegates (cache, kinda). 2) The module factory delegate also may keep such references for itself rather than trusting the parent class to do everything properly all the time. 3) a Module Delegate may or may not actually cache its own resources 4) A server keeps track of which modules are on it and caches the instances 5) the publish info object ALSO caches its results during a publish operation. It gets this information from the module object passed in. If a module / module delegate does *not* cache and simply parses the tree each time, the server keeping invalid instances is not a problem. If the module delegate does cache independently, old instances become a problem. So... why is the module factory making new instances of a module on a normal change to a text file? This shouldn't really be happening all that often. Well, I've been tracing through this test case a lot lately, and it seems that just the addition of this .txt file (or the modifying of it) is throwing a resource delta that is 3 elements long, including the .project file, the .settings folder, and finally the changed txt file. This seems to be stemming from the autobuilder, but I can't be sure. So this means every time there's a workspace change when the autobuilder is running, the ModuleFactoryDelegate's superclass (ProjectModuleFactoryDelegate) is erasing its cache and instructing its subclass to do the same and to create new modules for a given project. What this means is that every time the workspace changes (with autobuild on), 1) and 2) replace and get new modules. Old instances of 3) immediately become stale, and so 4)'s module references also immediately becomes stale. The cache for ModulePublishInfo actually seems to work properly ;) So that's good. With 1) and 2) being asked to create new Modules so often, it seems to me that the Server's list will become stale *very* quickly and *very* often, making the server's cache of modules basically meaningless. A way jeetools could work around this would be to add a further level of indirection so that our modules AND module delegates are just shells which point to an actual singleton caching instance stored by the factory. This way the server's references can go stale, but it won't matter. Just an idea. Hi I approve after looking at the potential results.... we are doing more perf testing locally here. But the potential for harm (stale results) is bad enough to fix. Initially performance doesn't look to be affected I also agree we NEED to revisit the caching mechanism's in 3.2.2 - we can open a new bug to track. The interaction between the delegates and server caches is not clear. The requirements on the server cache, and interaction with the delegates etc has never been well explained. * Explain why you believe this is a stop-ship defect. Or, if it is a "hotbug" (requested by an adopter) please document it as such.
This is requested by an adopter, and it is officially a regression in that behaviour has changed in a unit test.
* Is there a work-around? If so, why do you believe the work-around is insufficient?
Several workarounds were tried, but none worked.
* How has the fix been tested? Is there a test case attached to the bugzilla record? Has a JUnit Test been added?
I've used a junit test of our product to verify it has fixed. I hear IBM has also done performance testing on it and verified that results are not detrimental.
* Give a brief technical overview. Who has reviewed this fix?
An API was added for 3.2.0 which allows modules that cache to alert a server when there is a new changed module instance and the servers should update to the most recent instance. When jeetools switched to caching, this API was forgotten and not called. In order to alert the server that your most recent module delegate has changed members, this API should be used.
* What is the risk associated with this fix?
Aside to performance, I see almost no risk at all. In regards to performance, the risk is partially unknown but according to Chuck's comment, the performance hit (if there is one) is not noticable.
Committed to HEAD for WTP 3.3. (And tentatively WTP 3.2.1, assuming PMC approval) |
J2EEDeployableFactory.cleanAllDelegates should read: protected void cleanAllDelegates() { Iterator<FlatComponentDeployable> i = moduleDelegates.values().iterator(); while(i.hasNext()) { i.next().clearCache(); } modulesChanged(); } The addition required is the +modulesChanged(); This is due to the caching which was enabled recently. Without this patch, some APIs may become stale unless they constantly get new instances of the module by checking the module factory.