Community
Participate
Working Groups
Steps to reproduce: 1. Windows > Preferences > Server > Runtime Environments 2. Add... > Basic > J2EE Preview 3. Finish > OK. 4. Windows > Preferences > Server > Runtime Environments 5. Select the J2EE Preview runtime in the list of available runtime. 6. Remove. 7. Add... > Basic > J2EE Preview (add it again) 8. Finish > OK. 9. File > New > Project... > Dynamic Web Project 10. Select the J2EE Preview for Target runtime 11. Set project name. The following validation error is shown: Runtime "J2EE Preview" is invalid. The name is already in use. Specify a different name. The workaround is to restart the IDE. This applies to other server runtimes - the J2EE Preview is only an example. Reproducible both in Galileo and Helios. There is no such validation error shown in the Server Runtime Environments preference page. It looks to be a problem in the Facet Framework - as far as I can see the initially created is cached in the RuntimeBridge objects and probably this causes the problem.
Over to server tools as that's where the bridge implementation is located. The API for bridges is very limited. It was never intended to be a permanent solution. Re-designing the bridge infrastructure is one of the todos for faceted project framework 2.x code line. In the current API, everything revolves around string identifiers. In particular, a runtime is considered the same if it's id is the same. One implication of this is that IRuntimeBridge.IStub implementation cannot hold a reference to the runtime object that's being bridged. It can only hold name/id and lookup the object on every access. I will attach a patch.
Created attachment 171658 [details] Patch This patch makes the bridged runtime stub hold a string id rather than a reference to an IRuntime instance. Would be good to see this included in Helios SR1.
*** This bug has been marked as a duplicate of bug 295494 ***
Ops.. wrong bug.. reopening
I have some concerns on how this is working as it will now be returning null and before it didn't. I would also like to sit down and think a little more about the actual logic, I can't see this making it into 3.2.1
Hmm... Seems bad to rule out fixing a bug in a service release, especially when the problem has been diagnosed and a patch attached. Let know what specific concerns you have. You didn't specify where null return value will now appear.
Just looking quickly through the patch, the change in behaviour is that in the past we were always creating a a new stub, but now it seems that there will be times when we return null
Perhaps you are reading the patch in the wrong direction? The old bridge method implementation would return null at times, while the new one will always return a non-null value...
*** Bug 324498 has been marked as a duplicate of this bug. ***
We are raising this an adopter hotbug issue. We hit this again recently in an important demo. We need this in 3.2.2 (Helios SR1).
Note that once the problem is hit the only workaround is Eclipse restart.
Hi Konstantin, Maybe I'm misinterpreting the changes, but does this imply that the facet framework/bridge code is caching stubs? How does this cache get cleared, i.e. when would it call getExportedRuntimeNames() or bridge() to ensure that it has newly created runtimes or that deleted ones are removed?
The stubs are cached. The runtime objects are created around them and those are held in the RuntimeManager. Every invocation into the RuntimeManager first calls to bridges to see if anything is changed (getExportedRuntimeNames). Then reconciliation is performed. The bridge method will only be called on new names.
k, thanks. It's certainly less critical, but we should go back and update the IRuntimeBridge javadoc as well. The current server tools code is already 'spec compliant', so if it has to change to get in line with the implementation of the framework (which I'm fine with) we should also go back and make this requirement clear.
Sure. We can update the javadoc, etc. Let's just fix the immediate problem first.
Konstantin, My bad for looking at the code in the opposite way. I still have concerns about this patch and that is part of the reason why I haven't looked into this, aside from time. Perhaps you can clarify or provide a new patch. In the old code it seems that we were doing a more extensive search for a valid runtime. The old code was trying to find a runtime using the name or the id: IRuntime[] runtimes = ServerCore.getRuntimes(); int size = runtimes.length; for (int i = 0; i < size; i++) { if (runtimes[i].getId().equals(name)) <-- compare 'name' with runtime.id return new Stub(runtimes[i]); if (runtimes[i].getName().equals(name)) <-- compare 'name' with runtime.name return new Stub(runtimes[i]); } I don't see how are we doing that with the new code. This is important because if I remember correctly back at sometime the facet metadata for a project was storing the translatable name of the runtime as the id, and not the wst.runtime id. Also I got my reservation of how pervasive is the problem considering that the problem has a workaround, and only happens when a user adds and removes the same runtime on one run. If after the workaround the product works correctly what are the chances of a hitting the problem again? In summary, my reservations are because: - We are RC2. the PMC board is getting strict on the bugs that are going in - Lack of documentation for the test of the fix (this can be alleviated by you providing information on what kind of test have you done, I would like to see that we tested migration scenarios? adopters product might have regulations on that area) - The problem has a workaround - The problem happens only when a user creates and deletes the same runtimes during one run.
Created attachment 178362 [details] Patch v2 Corrects the lookup to use both name and id as before.
I have attached a revised patch that restores name/id matching. - We are RC2. the PMC board is getting strict on the bugs that are going in Indeed. This could have been resolved much earlier with less fuss, yet here we are. :) - Lack of documentation for the test of the fix (this can be alleviated by you providing information on what kind of test have you done, I would like to see that we tested migration scenarios? adopters product might have regulations on that area) The test case is spelled out in the opening comment by Kaloyan. You can run it before and after the patch. Outside of that, regular server tools smoke test and release verification tests will provide for the baseline. - The problem has a workaround The workaround is not obvious to the user. The workaround of restarting Eclipse is drastic. - The problem happens only when a user creates and deletes the same runtimes during one run. This is fairly common. A typical way that user runs into this is to suspect that their is a problem with the runtime definition in workspace and then to attempt to recreate said runtime. Since default name is derived from type name, name re-use is very common. Note that SAP opened this bug. We at Oracle ran into this multiple time during our testing. We most recently ran into this during an important demonstration, which didn't exactly paint WTP in positive light. We have a release coming up that will be based on Helios SR1, so it is critical for us that this fix is not further delayed.
The logic of the code looks good on the new patch after addressing the id and name matching problem on the previous patch. This patch changes in a way that we are doing a look up every time we are trying to access. Methods like getRuntimeComponents() are being called fairly often. Therefore, doing the extra calculation of each call may have a performance impact on the call (hopefully, users don't have a large number of runtimes within their workspace so it shouldn't be too bad for a typical usage scenario). The thing that we are trying to address is in case the runtime changed, the cached runtime value doesn't gets updated. One way that can fix the problem is to forces a refresh on the cached runtime value when the cached runtime has been removed, e.g. using the runtime life cycle listener. Then, we don't have to do the recalculation every time we access. Given that we are late on the cycle, it is probably ok with the current proposed fix. However, we should consider changing the design to prevent the performance hit.
* 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. Adopter requested * Is there a work-around? If so, why do you believe the work-around is insufficient? There is a workaround, but it might not be obvious. The workaround is to restart eclipse and life will be good. * How has the fix been tested? Is there a test case attached to the bugzilla record? Has a JUnit Test been added? Tested the scenario described, and did a a couple of more adhoc testing. * Give a brief technical overview. Who has reviewed this fix? Angel, Elson (and probably Tim) reviewed the fix. * What is the risk associated with this fix? Medium, the code code might be called often
Changes committed to 32M
Thanks!
changes released to 32M
changes committed to HEAD(3.3)
changes released to HEAD(3.3)
I think the target milestone was accidentally unset. Fixing it...
Thanks Kosta. It seems to have happened when I accepted the hotbug request. Marking as resolved/fixed now.
New Gerrit change created: https://git.eclipse.org/r/108992
