Bugzilla – Full Text Bug Listing
| Summary: | Cannot remove a test invocation to a removed HTTP request. | ||
|---|---|---|---|
| Product: | z_Archived | Reporter: | jkubasta |
| Component: | TPTP | Assignee: | DuWayne Morris <dmorris> |
| Status: | CLOSED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | P1 | CC: | jkubasta, jptoomey, newboya1988, paulslau, slavescu |
| Version: | unspecified | Keywords: | plan |
| Target Milestone: | --- | Flags: | paulslau:
review+
jptoomey: review+ |
| Hardware: | PC | ||
| OS: | Windows XP | ||
| Whiteboard: | |||
| Attachments: | |||
|
Description
jkubasta
Reported by product team. Please see if reproducible with 441 I tested this with 4.4.1 and found that you can removean HTTP request from the Http Requests tab of the Hyades URL Test Suite, you do not get an error when you save the changes. I also noticed if you remove an HTTP request from the Behavior tab first, then you can remove the same HTTP request from the HTTP Requests tab, it works OK as well. However, if you remove an HTTP request from the HTTP Requests tab first, you can not remove the same request from the Behavior tab. Regardless if you remove an HTTP request from the HTTP Requests tab or the Behavior tab, I never had a problem saving the testsuite, regenerating it or executing it. Although the removed HTTP Requests still show up in the Behavior tab, then are not in the generated test or test log. I may not understand the logic of how the removed requests are supposed to show up in Behavior tab, it seems like we need them removed from the Behavior tab as well. We should be consistent for all test types. That is, if the test case/method/request is remove, the associated invocation in the behavior is removed as well. For example: https://bugs.eclipse.org/bugs/show_bug.cgi?id=139810 Mark, please provide a sizing. Duwayne, please provide a sizing. Please provide a sizing for this defect before Monday, November 12, 2007. Deferred from I5 to I6 with PMC approval. Deferring to I7 as discussed by PMC/AG (http://wiki.eclipse.org/TPTP-AG-20080331#Test_Project). This is a similar symptom to https://bugs.eclipse.org/bugs/show_bug.cgi?id=225963. Is the stack trace the same? I have found the cause for this problem. In EMFUtil.processObjectReferences, there is a line to unset the behavior reference in the execution invocation object. Specifically: element.eUnset(ref); // line 195 This was evidently done to strip out any references to the behavior when it is removed to avoid the potential of corrupted data. The unintended side-effect of this is that later, when the code is executed to removeAllInvocations, the lack of the reference causes the invocation object to not be removed. In the HyadesTPFTestSuiteAdapter, I have modified getInvocationsFromFragement method to add invocations (BVRExecutionOccurrence) to the delete list where the getOtherBehavior returns null: TPFBehavior otherBehavior = ((BVRExecutionOccurrence)fragment).getOtherBehavior(); This has the benefit of repairing any existing URL testsuite by removing these invocations the firet time an HTTP request is removed. I will attach a patch shortly. Created attachment 96875 [details]
patch for org.eclipse.hyades.models.common.facades.behavioral.impl
Paul, please review so we can ask for approval to deliver.
Added Paul to review list. *** Bug 225963 has been marked as a duplicate of this bug. *** Patch reviewed. We need test cases for this defect and 22596, including the below scenario. This patch resolves the symptoms in this defect and 22596 but a NullPointerException is thrown after the following: 1) Create a test suite with one test case. 2) In the behavior, create a test invocation to the test case, a loop, and a child test invocation to the test case. 3) Save the test suite. 4) Remove the test case, focus shifts to the behavior tab (which it should not) and the following exception is logged to the .log file: java.lang.NullPointerException at org.eclipse.hyades.models.hierarchy.util.ContainmentTraverser.populateChildrenIterator(ContainmentTraverser.java:129) at org.eclipse.hyades.models.hierarchy.util.ContainmentTraverser.traverseEObjectChildrenIterator(ContainmentTraverser.java:148) at org.eclipse.hyades.models.hierarchy.util.ContainmentTraverser.traverseEObject(ContainmentTraverser.java:144) at org.eclipse.hyades.models.hierarchy.util.EObjectsTraverser.traverse(EObjectsTraverser.java:109) at org.eclipse.hyades.models.hierarchy.util.ContainmentTraverser.traverseEObjectChildrenIterator(ContainmentTraverser.java:149) at org.eclipse.hyades.models.hierarchy.util.ContainmentTraverser.traverseEObject(ContainmentTraverser.java:144) at org.eclipse.hyades.models.hierarchy.util.EObjectsTraverser.traverse(EObjectsTraverser.java:109) at org.eclipse.hyades.models.hierarchy.util.ContainmentTraverser.traverseEObjectChildrenIterator(ContainmentTraverser.java:149) at org.eclipse.hyades.models.hierarchy.util.ContainmentTraverser.traverseEObject(ContainmentTraverser.java:144) at org.eclipse.hyades.models.hierarchy.util.EObjectsTraverser.traverse(EObjectsTraverser.java:109) at org.eclipse.hyades.models.hierarchy.util.ContainmentTraverser.traverseEObjectChildrenIterator(ContainmentTraverser.java:149) at org.eclipse.hyades.models.hierarchy.util.ContainmentTraverser.traverseEObject(ContainmentTraverser.java:144) at org.eclipse.hyades.models.hierarchy.util.EObjectsTraverser.traverse(EObjectsTraverser.java:109) at org.eclipse.hyades.models.hierarchy.util.EObjectsTraverser.traverse(EObjectsTraverser.java:95) at org.eclipse.hyades.models.hierarchy.util.EMFUtil.delete(EMFUtil.java:115) at org.eclipse.hyades.models.hierarchy.util.EMFUtil.delete(EMFUtil.java:86) at org.eclipse.hyades.models.hierarchy.util.EMFUtil.delete(EMFUtil.java:136) at org.eclipse.hyades.test.core.util.EMFUtil.remove(EMFUtil.java:611) at org.eclipse.hyades.test.ui.internal.model.ui.RemoveChildrenAction.breakFeatures(RemoveChildrenAction.java:146) at org.eclipse.hyades.test.ui.internal.model.ui.RemoveChildrenAction.run(RemoveChildrenAction.java:126) at org.eclipse.hyades.test.ui.internal.editor.form.util.EObjectTreeSection.doRemove(EObjectTreeSection.java:646) at org.eclipse.hyades.test.ui.internal.editor.form.util.EObjectTreeSection.buttonSelected(EObjectTreeSection.java:615) at org.eclipse.hyades.test.ui.internal.editor.form.util.TreeSection$PartAdapter.buttonSelected(TreeSection.java:55) at org.eclipse.hyades.test.ui.internal.editor.form.base.SharedPartWithButtons$SelectionHandler.buttonSelected(SharedPartWithButtons.java:58) at org.eclipse.hyades.test.ui.internal.editor.form.base.SharedPartWithButtons$SelectionHandler.widgetSelected(SharedPartWithButtons.java:49) at org.eclipse.swt.widgets.TypedListener.handleEvent(TypedListener.java:227) at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:83) at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1002) at org.eclipse.swt.widgets.Display.runDeferredEvents(Display.java:3773) at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3372) at org.eclipse.ui.internal.Workbench.runEventLoop(Workbench.java:2375) at org.eclipse.ui.internal.Workbench.runUI(Workbench.java:2339) at org.eclipse.ui.internal.Workbench.access$4(Workbench.java:2205) at org.eclipse.ui.internal.Workbench$4.run(Workbench.java:478) at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:288) at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:473) at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:149) at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:106) at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:193) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:110) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:79) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:362) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:175) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:64) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:615) at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:549) at org.eclipse.equinox.launcher.Main.basicRun(Main.java:504) at org.eclipse.equinox.launcher.Main.run(Main.java:1236) at org.eclipse.equinox.launcher.Main.main(Main.java:1212) (In reply to comment #14) Note, this behavior also occurs in TPTP I6 but should be resolved under this defect. Also, when the test case is removed, the first test invocation is successfully removed leaving only the test invocation under the loop (due to the NPE). The model code behavior has changed in 4.5i7. Now, the code in HyadesTPFTestSuiteAdapter that removes the invocations of the behavior never gets called. When attempting to remove an HTTP request, the first invocation is removed. Attempt to remove the request again, and the second invocation is removed. Once there is only one invocation left, removing behavior actually removes behavior and the remaining single invocation. So, who is making these changes? Looks like it is mostly cleaned up except that all invocations should be removed when the request is removed. (In reply to comment #16) > The model code behavior has changed in 4.5i7. Now, the code in > HyadesTPFTestSuiteAdapter that removes the invocations of the behavior never > gets called. > > When attempting to remove an HTTP request, the first invocation is removed. > Attempt to remove the request again, and the second invocation is removed. > Once there is only one invocation left, removing behavior actually removes > behavior and the remaining single invocation. > > So, who is making these changes? Looks like it is mostly cleaned up except > that all invocations should be removed when the request is removed. Using the TPTP-4.5.0-200805120832 build, one of original symptoms for URL tests (removing a HTTP Request causing a NPE when there is one test invocation in the behavior) cannot be reproduced. However, the same symptom exists for AGR tests (see https://bugs.eclipse.org/bugs/show_bug.cgi?id=225963#c1). In addition, two or more test invocations of the same test case/method/HTTP Request in the behavior and loops containing one or more test invocations causes the same NPE. Created attachment 101374 [details]
patch for org.eclipse.hyades.models.hierarchy.util.EMFUtil.java
The changes made in the model code recently have repaired the NPE problems. The remaining issue needed that I found was to not call eUnset on the behavior invocation fragments and instead let the HyadesTPFTestSuiteAdapter cleanup the model by removing invocations associated with a removed behavior as it was obviously designed to do. eUnset nulls out the "otherBehavior" property.
Since I am not very familiar with model code, a review is needed to assure that this "minor" change does not break other things. Specifically, are there other types of model elements besides behavior invocations that have an "otherBehavior" property that would not get properly cleaned up during a delete if eUnset is not called?
Testing so far includes URL test and AGR tests, including multiple invocations on multiple behaviors and including loops. I am doing some added testing to look for issues.
Paul, Please review. I have left the first patch as well. I consider the first patch optional at this late date near release. However, it does cover the case of removing any new or existing invocations that have a null "otherBehavior" property and it is a simple change. DuWayne (In reply to comment #19) > Paul, > > > Please review. I have left the first patch as well. I consider the first > patch optional at this late date near release. However, it does cover the case > of removing any new or existing invocations that have a null "otherBehavior" > property and it is a simple change. > > DuWayne > Given the lack of familiarity I have with the code, this patch is in the hierarchy model and not the test model, and how far we are along in the release, I am hesitant to approve this patch. Marius: Would you be able to do us a favor and review this patch? Joanna: FYI, since this patch is in the hierarchy model. The patch for EMFUtil might affect other scenarios, so I don't think it is safe to uncomment that line until you make sure that nothing else is affected. Can you do the cleanup for those references before EMFUtil is called? at org.eclipse.hyades.test.core.util.EMFUtil.remove(EMFUtil.java:611) at org.eclipse.hyades.test.ui.internal.model.ui.RemoveChildrenAction.breakFeatures(RemoveChildrenAction.java:146) (In reply to comment #21) > The patch for EMFUtil might affect other scenarios, so I don't think it is safe > to uncomment that line until you make sure that nothing else is affected. > Thanks Marius. Duwayne, we will need to better understand the root cause of the problem so we can propose a solution specific to the Test Model. The model code is broken. An HTTP request cannot be removed without an exception.
Testing a fix now. Added the last two lines in this code snippet:
protected boolean traverse(Iterator iter) {
while (iter.hasNext()) {
EObject traversedElement = (EObject) iter.next();
EObject visitorElement = getElement(traversedElement);
if (traversedElement == null)
break;
The issue in org.eclipse.hyades.models.hierarchy.util.EObjectsTraverser is that it is throwing a NPE because traversedElement and visitorElement are assummed to not be null. When this compound iterator does NOT contain an EObject, but rather a "FastList" element provider, the EObjects above are NULL. The result is an exception as follows:
java.lang.NullPointerException
at org.eclipse.hyades.models.hierarchy.util.ContainmentTraverser.populateChildrenIterator(ContainmentTraverser.java:129)
at org.eclipse.hyades.models.hierarchy.util.ContainmentTraverser.traverseEObjectChildrenIterator(ContainmentTraverser.java:148)
at org.eclipse.hyades.models.hierarchy.util.ContainmentTraverser.traverseEObject(ContainmentTraverser.java:144)
at org.eclipse.hyades.models.hierarchy.util.EObjectsTraverser.traverse(EObjectsTraverser.java:109)
at org.eclipse.hyades.models.hierarchy.util.ContainmentTraverser.traverseEObjectChildrenIterator(ContainmentTraverser.java:149)
at org.eclipse.hyades.models.hierarchy.util.ContainmentTraverser.traverseEObject(ContainmentTraverser.java:144)
at org.eclipse.hyades.models.hierarchy.util.EObjectsTraverser.traverse(EObjectsTraverser.java:109)
at org.eclipse.hyades.models.hierarchy.util.ContainmentTraverser.traverseEObjectChildrenIterator(ContainmentTraverser.java:149)
at org.eclipse.hyades.models.hierarchy.util.ContainmentTraverser.traverseEObject(ContainmentTraverser.java:144)
at org.eclipse.hyades.models.hierarchy.util.EObjectsTraverser.traverse(EObjectsTraverser.java:109)
at org.eclipse.hyades.models.hierarchy.util.ContainmentTraverser.traverseEObjectChildrenIterator(ContainmentTraverser.java:149)
at org.eclipse.hyades.models.hierarchy.util.ContainmentTraverser.traverseEObject(ContainmentTraverser.java:144)
at org.eclipse.hyades.models.hierarchy.util.EObjectsTraverser.traverse(EObjectsTraverser.java:109)
at org.eclipse.hyades.models.hierarchy.util.EObjectsTraverser.traverse(EObjectsTraverser.java:95)
at org.eclipse.hyades.models.hierarchy.util.EMFUtil.delete(EMFUtil.java:115)
at org.eclipse.hyades.models.hierarchy.util.EMFUtil.delete(EMFUtil.java:86)
at org.eclipse.hyades.models.hierarchy.util.EMFUtil.delete(EMFUtil.java:136)
at org.eclipse.hyades.test.core.util.EMFUtil.remove(EMFUtil.java:611)
at org.eclipse.hyades.test.ui.internal.model.ui.RemoveChildrenAction.breakFeatures(RemoveChildrenAction.java:146)
at org.eclipse.hyades.test.ui.internal.model.ui.RemoveChildrenAction.run(RemoveChildrenAction.java:126)
at org.eclipse.hyades.test.ui.internal.editor.form.util.EObjectTreeSection.doRemove(EObjectTreeSection.java:646)
at org.eclipse.hyades.test.ui.internal.editor.form.util.EObjectTreeSection.buttonSelected(EObjectTreeSection.java:615)
at org.eclipse.hyades.test.ui.internal.editor.form.util.TreeSection$PartAdapter.buttonSelected(TreeSection.java:55)
at org.eclipse.hyades.test.ui.internal.editor.form.base.SharedPartWithButtons$SelectionHandler.buttonSelected(SharedPartWithButtons.java:58)
at org.eclipse.hyades.test.ui.internal.editor.form.base.SharedPartWithButtons$SelectionHandler.widgetSelected(SharedPartWithButtons.java:49)
at org.eclipse.swt.widgets.TypedListener.handleEvent(TypedListener.java:227)
at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:84)
at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1002)
at org.eclipse.swt.widgets.Display.runDeferredEvents(Display.java:3801)
at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3400)
at org.eclipse.ui.internal.Workbench.runEventLoop(Workbench.java:2387)
at org.eclipse.ui.internal.Workbench.runUI(Workbench.java:2351)
at org.eclipse.ui.internal.Workbench.access$4(Workbench.java:2203)
at org.eclipse.ui.internal.Workbench$5.run(Workbench.java:493)
at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:288)
at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:488)
at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:149)
at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:112)
at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:193)
at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:110)
at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:79)
at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:379)
at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:179)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:45)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
at java.lang.reflect.Method.invoke(Method.java:591)
at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:549)
at org.eclipse.equinox.launcher.Main.basicRun(Main.java:504)
at org.eclipse.equinox.launcher.Main.run(Main.java:1236)
at org.eclipse.equinox.launcher.Main.main(Main.java:1212)
Created attachment 102224 [details]
Patch for org.eclipse.hyades.models.heirarchy.util
Adding a patch for EObjectsTraverser. Both patches must be applied. The first patch had been previously reviewed. Since unSet() is called, the first patch is needed to remove the invocations that have the "otherBehavior" set to null.
Paul, please review, testing looks good. Note that removal of an HTTP request does not always throw an exception. It depends on the contents of the TestSuite. The exception seems to always happen when there are two or more invocations of the request being removed. The problem with the error in EobjectsTraverser is that it could be causing huge problems in many areas, since the traverser is called into action whenever an object is deleted through EMFUtil.
(In reply to comment #24) > The model code is broken. An HTTP request cannot be removed without an > exception. Could these inconsistencies be due to changes in your development environment? I am not aware of any changes to the model code in the last week, and the build reports confirm no changes since 2008-05-23 12:25 (last build report available). (In reply to comment #25) > Created an attachment (id=102224) [details] > Patch for org.eclipse.hyades.models.heirarchy.util > > Adding a patch for EObjectsTraverser. Both patches must be applied. The first > patch had been previously reviewed. Since unSet() is called, the first patch > is needed to remove the invocations that have the "otherBehavior" set to null. > > Paul, please review, testing looks good. Note that removal of an HTTP request > does not always throw an exception. It depends on the contents of the > TestSuite. The exception seems to always happen when there are two or more > invocations of the request being removed. The problem with the error in > EobjectsTraverser is that it could be causing huge problems in many areas, > since the traverser is called into action whenever an object is deleted through > EMFUtil. > A couple of comments: -This solution does not solve the root cause of the null object in the list. Do you have idea on the root cause? Given the point we're at in the release, I am concerned with making model changes that affect all projects without understanding the root cause the problem. -If this fix is the right fix, we should be continue traversing the list instead of breaking out of the loop and returning true. -If this fix is the right fix, I would assume the same fix should be applied to org.eclipse.hyades.models.hierarchy.util.EObjectsTraverser.traverse(List). Paul, There is a misunderstanding. I stated that there was NOT a null object in the list. There is a case where the last object in the list of this compound iterator does NOT contain an EObject, but rather a "FastList" element provider object. This results in the EObjects being NULL because of the (EObject) type cast performed on the iterator. Then we are calling into the following, passing in a null EObject: org.eclipse.hyades.models.hierarchy.util.ContainmentTraverser.traverseEObjectChildrenIterator which then calls into: org.eclipse.hyades.models.hierarchy.util.ContainmentTraverser.populateChildrenIterator The above class throws the NPE because it was not expecting a null object being passed in. No, the code did not change in the last week. It is behaving the same as when you discovered this problem on 4/22 in comment #14. It seems there were other changes in the model code somewhere, perhaps even in the eclipse modeling framework. That not withstanding, the change I am proposing is to correct bad code. This is a compound iterator and the code is assumming each object in the iterator is an EObject when it is NOT always the case. There is nothing but harm that can come from calling traverseEObjectChildrenIterator with a null object. This causes the bulk of the work to process the delete to be suddenly abandoned when the exception is thrown. This model is very badly broken, the fix I am proposing corrects bad code or at the very least is not checking the objects in the compound iterator list. This is a quick fix that cures the ills and cleans up properly. Perhaps we should talk on the phone to discuss this further. The way this is working now will probably break several products. (In reply to comment #28) Thanks for your summary. I have spoken with Marius about the patch and we both agree that we need to determine when and why the FastList is added to the end of that list. Granted, handling the null element case in the traverse() method is beneficial (assuming we do not change the behavior of the method), but more information is required before we can fully evaluate the proposed patches. Let me know if you want to have a call to discuss further, possibly including Marius. The FastList object is routinely added to the iterator as follows in the ContainmentTraverser class when traverseEObjectChildrenIterator is called:
protected boolean traverseEObjectChildrenIterator(EObject object) {
CompoundIterator childrenIterator = childrenIteratorPool.alloc();
The inner class method childrenIteratorPool.alloc creates and adds the FastList as follows:
public List alloc() {
List list;
if (lists.size() == (currentEntryIndex + 1)) {
list = new FastList();
lists.add(list);
currentEntryIndex++;
} else {
currentEntryIndex++;
list = (List) lists.get(currentEntryIndex);
}
return list;
}
I am investigating why this normally did not historically create a problem, even though the fix I have proposed is appropriate and the code in the traverse method is wrong as it stands since it assumes that all iteration list objects are EObjects.
I have tested TPTP 4.4.1 and a 4.5 build from October. The same NPE happens there: java.lang.NullPointerException at org.eclipse.hyades.models.hierarchy.util.ContainmentTraverser.populateChildrenIterator(ContainmentTraverser.java:129) at org.eclipse.hyades.models.hierarchy.util.ContainmentTraverser.traverseEObjectChildrenIterator(ContainmentTraverser.java:148) at org.eclipse.hyades.models.hierarchy.util.ContainmentTraverser.traverseEObject(ContainmentTraverser.java:144) at org.eclipse.hyades.models.hierarchy.util.EObjectsTraverser.traverse(EObjectsTraverser.java:109) at org.eclipse.hyades.models.hierarchy.util.ContainmentTraverser.traverseEObjectChildrenIterator(ContainmentTraverser.java:149) So, despite my earlier statements, it is evidently not a recent change that has caused this. My belief is that I have found the root cause of the problem and it has been this way a long time. Whether the FastList is needed or not in the list might be another question, but the ContainmentTraverser should handle this properly. Still looking at why this does not happen until there are at least two invocations of a request in the testsuite when the request is deleted. It turns out that org.eclipse.hyades.models.hierarchy.util.CompoundIterator.java is designed to use FastList.java, so that was not the issue. The error happens when the CompoundIterator list being processed by EObjectsTraverser.traverse is in the process of deleting one or more invocations from the behavior tab. The list being processed at that point actually consists of the invocations and loop objects listed in the behavior tab. When an invocation is removed, it is somehow removed from the CompoundIterator List without changing the size of the list. So, if there were 13 elements, the list size stays at 13 after one or more of the list elements have been removed. Thus, CompoundIterator.next() is actually returning null because the position is being incremented past the actual adjusted size of the list. Therefore, while my patch will work, it should not be the case that objects are being removed from the list without adjusting the size. I set a breakpoint in the remove method of CompoundIterator and noticed that this method is not being called. So, next I need to find where the invocation object is actually being removed from the list wihout affecting the size of the list. It turns out that org.eclipse.hyades.models.hierarchy.util.CompoundIterator.java is designed to use FastList.java, so that was not the issue. The error happens when the CompoundIterator list being processed by EObjectsTraverser.traverse is in the process of deleting one or more invocations from the behavior tab. The list being processed at that point actually consists of the invocations and loop objects listed in the behavior tab. When an invocation is removed, it is somehow removed from the CompoundIterator List without changing the size of the list. So, if there were 13 elements, the list size stays at 13 after one or more of the list elements have been removed. Thus, CompoundIterator.next() is actually returning null because the position is being incremented past the actual adjusted size of the list. Therefore, while my patch will work, it should not be the case that objects are being removed from the list without adjusting the size. I set a breakpoint in the remove method of CompoundIterator and noticed that this method is not being called. So, next I need to find where the invocation object is actually being removed from the list wihout affecting the size of the list. Not sure if I correctly understood the scenario, but it is illegal to modify a collection while it is being iterated. If the iterator detects the modification, it will throw a ConcurrentModificationException, but otherwise the behavior is undefined. There are at least two simple ways to deal with this restriction in Java. Once is to call toArray() and loop over the array contents instead. Another is to defer making changes to the collection until the iterator is complete (i.e. keep a list of the elements you want to add or remove, but don't actually do the work until you're done iterating over the collection.) Thanks Joe.
The calls to callVisitors case: AfterChildren is actually removing the test invocation from the iterator:
protected boolean callVisitors(int flag, EObject element) {
boolean ret = true;
for (int i = visitors.size() - 1; i >= 0; i--) {
try {
switch (flag) {
case BEFORE_CHILDREN:
ret = ((EObjectVisitor) visitors.get(i)).beforeChildren(element) && ret;
break;
case AFTER_CHILDREN:
ret = ((EObjectVisitor) visitors.get(i)).afterChildren(element) && ret;
break;
}
} catch (RuntimeException re) {
ModelDebugger.log(re);
}
}
return ret;
}
The above calls into EMFUtil, in the following method:
public static Set delete(final EObject object, final boolean resolve, final ContainmentTraverser containmentTraverser) {
final Set changedResources = new HashSet();
final List removeList = new FastList();
final List deleteList = new FastList();
EObjectVisitor objectVisitor = new EObjectVisitor() {
public boolean beforeChildren(EObject element) {
return true;
}
public boolean afterChildren(EObject element) {
if (isContainedIn(element, object)) {
deleteList.add(element);
} else {
processObjectReferences(object, changedResources, resolve, true, removeList, containmentTraverser, element);
}
return true;
}
};
The call to processObjectReferences with the delete flag set true is removing the invocation while the iterator is in use. Setting the delete flag to false correct the problem and all the appropriate invocations are removed without the exception.
Given the reluctance by everyone to change model code even though it is broken, not sure what to do next.
Created attachment 102683 [details]
Patch for org.eclipse.hyades.heirarchy.util
Hi Paul, Joe, and Marius,
I modified my patch slightly to make sure we don't ever process a null return from the CompoundIterator and will still process any object returned by the iterator that is not null, instead of immediately performing a break on the first null. The first patch in test code had already been reviewed and takes care of deleting invocations that remain and have been eUnSet called on them to null out the reference to a deleted request.
There is now an understanding of where and why the model code is broken and that it has been broken a very long time (at least 4.4.1). At this late stage in 4.5, I'm not sure it is worth the risk of covering all the cases with a code change that would be needed for a proper fix in EMFUtil. Moreover, I am not familiar enough with model code to feel confident in quickly designing a complete fix that will not break other things.
I propose that we deliver the patches I have attached as a fix for 4.5 and defer the remainder of the model code fix to a later date. The patch makes sense to protect against a null object in any case, this defect aside. In addition, there are plenty of other prioities that need attention immmediately.
In my opinion, this makes for a low-risk straight forward fix, since there is only harm that can come from making these method calls in EObjectTraverser.traverse passing in a null object.
All in favor?
(In reply to comment #36) > I propose that we deliver the patches I have attached as a fix for 4.5 and > defer the remainder of the model code fix to a later date. The patch makes > sense to protect against a null object in any case, this defect aside. In > addition, there are plenty of other prioities that need attention immmediately. > > In my opinion, this makes for a low-risk straight forward fix, since there is > only harm that can come from making these method calls in > EObjectTraverser.traverse passing in a null object. > > All in favor? > +1 Comment on attachment 96875 [details]
patch for org.eclipse.hyades.models.common.facades.behavioral.impl
Assuming this patch is made obsolete by the new patch.
(In reply to comment #36) Good job Duwayne. > Created an attachment (id=102683) [details] > Patch for org.eclipse.hyades.heirarchy.util > > Hi Paul, Joe, and Marius, > > I modified my patch slightly to make sure we don't ever process a null return > from the CompoundIterator and will still process any object returned by the > iterator that is not null, instead of immediately performing a break on the > first null. The first patch in test code had already been reviewed and takes > care of deleting invocations that remain and have been eUnSet called on them to > null out the reference to a deleted request. Can you please create one patch for all the fixes proposed under this defect for testing and set the review flag on the defect for Joe and myself? > There is now an understanding of where and why the model code is broken and > that it has been broken a very long time (at least 4.4.1). At this late stage > in 4.5, I'm not sure it is worth the risk of covering all the cases with a code > change that would be needed for a proper fix in EMFUtil. Moreover, I am not > familiar enough with model code to feel confident in quickly designing a > complete fix that will not break other things. Please open a major severity defect against the Platform.UI componet with all of the applicable details from this defect. > I propose that we deliver the patches I have attached as a fix for 4.5 and > defer the remainder of the model code fix to a later date. The patch makes > sense to protect against a null object in any case, this defect aside. In > addition, there are plenty of other prioities that need attention immmediately. > > In my opinion, this makes for a low-risk straight forward fix, since there is > only harm that can come from making these method calls in > EObjectTraverser.traverse passing in a null object. I believe the benefit of this fit will far outweigh the risk. > All in favor? +1 Created attachment 102870 [details]
Combined patch for Model and Test code changes
Consolidated the changes for changes to both plugins to a single patch per request.
added review request for Joe per request. Reset the review flag for Paul to review and test the final patch. Rarely am I opposed to checking for null. :-) Looks good to me. Joanna, note the change to the hierarchy model that Joe has reviewed/approve, representing the Platform Project. Reviewed, tested and approved. Delivered changes into HEAD. Fixed and verified, closing. (In reply to comment #39) > > There is now an understanding of where and why the model code is broken and > > that it has been broken a very long time (at least 4.4.1). At this late stage > > in 4.5, I'm not sure it is worth the risk of covering all the cases with a code > > change that would be needed for a proper fix in EMFUtil. Moreover, I am not > > familiar enough with model code to feel confident in quickly designing a > > complete fix that will not break other things. > > Please open a major severity defect against the Platform.UI componet with all > of the applicable details from this defect. See defect #234907. |