Bugzilla – Full Text Bug Listing
| Summary: | dispose() is called before execute() on Command from AbstractTool | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Tools] GEF | Reporter: | <h1055071> | ||||
| Component: | GEF-Legacy GEF (MVC) | Assignee: | gef-inbox <gef-inbox> | ||||
| Status: | CLOSED DUPLICATE | QA Contact: | |||||
| Severity: | major | ||||||
| Priority: | P3 | CC: | aboyko, agfitzp, ahunter.eclipse, carsten.pfeiffer | ||||
| Version: | 3.6 | ||||||
| Target Milestone: | --- | ||||||
| Hardware: | PC | ||||||
| OS: | All | ||||||
| Whiteboard: | |||||||
| Attachments: |
|
||||||
Pretty sure this is a duplicate of Bug 307720 . The fix for Bug 307720 is already in a GEF 3.6.1 build. (In reply to comment #1) > Pretty sure this is a duplicate of Bug 307720 . > > The fix for Bug 307720 is already in a GEF 3.6.1 build. I'll check this. Where are maintenance builds kept? I can't find 3.6.1 on the GEF downloads page. I checked this against GEF build GEF-ALL-I201006212050. The problem still exists, dispose() is called in setCurrentCommand(Command c) before it is executed. This regression is quite serious for us. Wonder if we can simply move command.dispose() to AbstractTool#deactivate(). Every tool gets deactivated at some point of time, so I think that would be the best place for it. (In reply to comment #3) > I checked this against GEF build GEF-ALL-I201006212050. The problem still > exists, dispose() is called in setCurrentCommand(Command c) before it is > executed. > > This regression is quite serious for us. Can you confirm you have version v20100621-2050 of org.eclipse.gef in your target when you test? Our GMF based editors reported back that the problem was fixed with this patched GEF. (In reply to comment #5) > (In reply to comment #3) > > I checked this against GEF build GEF-ALL-I201006212050. The problem still > > exists, dispose() is called in setCurrentCommand(Command c) before it is > > executed. > > > > This regression is quite serious for us. > > Can you confirm you have version v20100621-2050 of org.eclipse.gef in your > target when you test? Our GMF based editors reported back that the problem was > fixed with this patched GEF. Yes, I've definitely got that version (GEF-ALL-I201006212050) You can test it for yourself on the "Shapes" example as I detailed above by overriding dispose() in ConnectionCreateCommand. I thought only the command stack would be disposing commands. Disposing a command that has never been executed, undone, or redone sounds unnecessary and could still break clients (even before calling execute()). Created attachment 173765 [details]
Test class for "Shapes" example
Here's an edited version of ConnectionCreateCommand.java from the "Shapes" example to show the problem. Substitute this class in the "Shapes" example and run it. Draw some shapes and then make some connection lines.
You'll see the message "Calling execute() after Command is disposed!" written to the console when the connection is made as the variable IS_DISPOSED has been set to true in the dispose() method which has been called prior to canExecute() and execute().
Also, you'll see "Calling canExecute() after Command is disposed!" in the canExecute() method, which is another problem, potentially testing for execution on disposed (nulled) objects. As far as I understood, EditPolicies were supposed to always create *new* command instances instead of reusing them. In the Shape example, ShapeEditPart's subclass of GraphicalNodeEditPolicy overwrites getConnectionCompleteCommand() in order to forward the call to CreateConnectionRequest, which always returns the same command instance (getStartCommand()). So I would say this is an application bug, at least if the above assumption is correct. On the other hand, if EditPolicies are allowed reuse commands, I'd like to know how this should work after such a reused command was executed and disposed through a CommandStack. (In reply to comment #10) > As far as I understood, EditPolicies were supposed to always create *new* > command instances instead of reusing them. > > In the Shape example, ShapeEditPart's subclass of GraphicalNodeEditPolicy > overwrites getConnectionCompleteCommand() in order to forward the call to > CreateConnectionRequest, which always returns the same command instance > (getStartCommand()). > > So I would say this is an application bug, at least if the above assumption is > correct. On the other hand, if EditPolicies are allowed reuse commands, I'd > like to know how this should work after such a reused command was executed and > disposed through a CommandStack. Look, all I know is my client is now broken. Please remove this hack. After analyzing this a little more: the mentioned GraphicalNodeEditPolicy has two methods protected Command getConnectionCompleteCommand(CreateConnectionRequest) and protected Command getConnectionCreateCommand(CreateConnectionRequest) In the latter method, a command is created and returned, but also saved in the request instance. The command is passed to AbstractTool.setCurrentCommand(), but the command is not executed. When later AbstractTool.setCurrentCommand() is called another time, the previously set command is disposed, but still referenced in the request. Later, getConnectionCompleteCommand() simply returns the (already disposed) command instance that was stored in the request. > Look, all I know is my client is now broken. Please remove this hack.
Then you're back to a potential leak, because (lots of) commands are never disposed. I say potential, because not all commands actually have something to dispose.
(In reply to comment #13) > > Look, all I know is my client is now broken. Please remove this hack. > > Then you're back to a potential leak, because (lots of) commands are never > disposed. I say potential, because not all commands actually have something to > dispose. OK, so I ship a product that used to work but now doesn't? It was a big mistake to include this hack at the last minute without proper testing. This isn't the first time this has happened. One thing I learnt - I would never use GEF ever again. > So I would say this is an application bug, at least if the above assumption is
> correct. On the other hand, if EditPolicies are allowed reuse commands, I'd
getConnectionCompleteCommand is called over and over for the same start command as the mouse is moving over a target, but once you create the connection, that request and start command instance get garbage collected. Returning the request's start command instance is a valid optimization.
(In reply to comment #4) > Wonder if we can simply move command.dispose() to AbstractTool#deactivate(). > Every tool gets deactivated at some point of time, so I think that would be the > best place for it. That would be possible. You would need to keep a Set of all not executed commands that were ever given to the tool since the last deactivate. > Then you're back to a potential leak, because (lots of) commands are never
> disposed. I say potential, because not all commands actually have something to
> dispose.
Only commands which have been executed, undone, or redone should be disposed, not every command instance every created. As you've pointed out, other than in the command stack, you have no idea what the lifecycle of a command instance is, or whether others are still holding onto an instance that they later hand back to you to be executed.
Please, please, please just roll the code back to what it was. So there was a leak? I didn't see any catastrophes reported. At the very least there should be BACKWARDS COMPATIBILITY. And, yes, I am pissed off. GEF seems to be broken in some new way every week. (In reply to comment #7) > I thought only the command stack would be disposing commands. Disposing a > command that has never been executed, undone, or redone sounds unnecessary and > could still break clients (even before calling execute()). It's a little unusual to have a dispose() that is not symmetric to new(), but if that is how it's supposed to be then the whole stuff should indeed be reverted. And a little note to the dispose() apidocs about that. (In reply to comment #14) > It was a big mistake to include this hack at the last minute without proper > testing. This isn't the first time this has happened. The facts are that the original fix went into GEF RC1 that was released on May 18. The fix passed all the GEF/GMF smoke tests, Junit tasks, adhoc tests. Over a month later a couple of IBM teams complained about the fix. And now close to two months later Phillip you are complaining. Otherwise I have heard nothing from the GEF community that this is an issue. According to my IBM team, the fix for this issue is already in the GEF 3.6.1 build and they are happy with it. But are we suggesting to change it again, we need a new patch. (In reply to comment #14) > OK, so I ship a product that used to work but now doesn't? Which product is this? http://archi.cetis.ac.uk/ ? (In reply to comment #20) > (In reply to comment #14) > > It was a big mistake to include this hack at the last minute without proper > > testing. This isn't the first time this has happened. > > The facts are that the original fix went into GEF RC1 that was released on May > 18. The fix passed all the GEF/GMF smoke tests, Junit tasks, adhoc tests. > > Over a month later a couple of IBM teams complained about the fix. And now > close to two months later Phillip you are complaining. Otherwise I have heard > nothing from the GEF community that this is an issue. > > According to my IBM team, the fix for this issue is already in the GEF 3.6.1 > build and they are happy with it. But are we suggesting to change it again, we > need a new patch. > > (In reply to comment #14) > > OK, so I ship a product that used to work but now doesn't? > > Which product is this? http://archi.cetis.ac.uk/ ? Yes. This was a "proof of concept" application. My recommendation now is to avoid using GEF. (In reply to comment #21) > (In reply to comment #20) > > (In reply to comment #14) > > > It was a big mistake to include this hack at the last minute without proper > > > testing. This isn't the first time this has happened. > > > > The facts are that the original fix went into GEF RC1 that was released on May > > 18. The fix passed all the GEF/GMF smoke tests, Junit tasks, adhoc tests. > > > > Over a month later a couple of IBM teams complained about the fix. And now > > close to two months later Phillip you are complaining. Otherwise I have heard > > nothing from the GEF community that this is an issue. > > > > According to my IBM team, the fix for this issue is already in the GEF 3.6.1 > > build and they are happy with it. But are we suggesting to change it again, we > > need a new patch. > > > > (In reply to comment #14) > > > OK, so I ship a product that used to work but now doesn't? > > > > Which product is this? http://archi.cetis.ac.uk/ ? > > Yes. This was a "proof of concept" application. My recommendation now is to > avoid using GEF. There have been so may bugs in GEF that I have encountered (search for my name) that I settled on using a relatively stable release of GEF - I think it was one from May some time. I had thought that when moving up to Eclipse 3.6 an GEF 3. that things would have settled down now by now. I was wrong. As far as I am concerned, GEF is unstable and unreliable and "fixes" are applied in a random manner. Search the GEF forum. (In reply to comment #20) > (In reply to comment #14) > > It was a big mistake to include this hack at the last minute without proper > > testing. This isn't the first time this has happened. > > The facts are that the original fix went into GEF RC1 that was released on May > 18. The fix passed all the GEF/GMF smoke tests, Junit tasks, adhoc tests. > > Over a month later a couple of IBM teams complained about the fix. And now > close to two months later Phillip you are complaining. Otherwise I have heard > nothing from the GEF community that this is an issue. > > According to my IBM team, the fix for this issue is already in the GEF 3.6.1 > build and they are happy with it. But are we suggesting to change it again, we > need a new patch. > > (In reply to comment #14) > > OK, so I ship a product that used to work but now doesn't? > > Which product is this? http://archi.cetis.ac.uk/ ? Why bring up this nonsense? I've provided you with a test case which proves that this is broken. (In reply to comment #10) I think your argument is erroneous. > As far as I understood, EditPolicies were supposed to always create *new* > command instances instead of reusing them. That's not necessarily so in this case. Read on... > > In the Shape example, ShapeEditPart's subclass of GraphicalNodeEditPolicy > overwrites getConnectionCompleteCommand() in order to forward the call to > CreateConnectionRequest, which always returns the same command instance > (getStartCommand()). Let's look at the documentation for getConnectionCompleteCommand(): /** * Returns the Command that will create the connection. This is second part * of creation. {@link CreateConnectionRequest#getStartCommand()} is used * here to obtain the contribution from the EditPart from which the User * started the <i>creation</i>. * * @param request * the CreateConnectionRequest * @return the complete command to create a connection */ So this pattern of creating the Command in getConnectionCreateCommand() and re-using and finalising it in getConnectionCompleteCommand() seems to be the standard pattern as recommended by the API and I dare say implemented by most clients out there in the wild, including my own. Furthermore, it's used in the "Flow" example (ActivityNodeEditPolicy) as well as the "Shapes" example. > > So I would say this is an application bug, at least if the above assumption is > correct. On the other hand, if EditPolicies are allowed reuse commands, I'd > like to know how this should work after such a reused command was executed and > disposed through a CommandStack. No, I would say that this is a special case of re-using (completing) a Command and if you now expect people to start creating a brand new Command in getConnectionCompleteCommand() it would mean a). Revising the API docs to say so b). Changing the "Flow" and "Shapes" examples to return new Commands. Note - these two examples shiip with the GEF SDK and have become the reference implementations. c). Removing the method CreateConnectionRequest.setStartCommand(Command) because it is not needed. So the upshot of this is that to call dispose() on a Command between getConnectionCreateCommand() and getConnectionCompleteCommand() could cause some damage if that Command implements dispose() as I do in my app. And apologies to Anthony Hunter if I was short/rude/angry in some of the comments above. It has been difficult for me to develop with GEF with these unexpected changes. A mistake in Comment 24. That should read: c). Removing the method CreateConnectionRequest.getStartCommand() because it is not needed. *** This bug has been marked as a duplicate of bug 307720 *** |
Build Identifier: I20100608-0911 Eclipse 3.6 GEF 3.6 A new check has been added to the AbstractTool class in its setCurrentCommand(Command c) method: if (disposeCurrentCommand && command != null) { command.dispose(); } This has the unwanted side effect of calling dispose() on the Command *before* the Command's execute() method is called. The API states for Command.dispose(): /** * This is called to indicate that the <code>Command</code> will not be used * again. The Command may be in any state (executed, undone or redone) when * dispose is called. The Command should not be referenced in any way after * it has been disposed. */ This is a disaster for our code, since our Command to create connections (as declared in the GraphicalNodeEditPolicy) calls dispose() to clean up objects. The execute() method is then called on the Command with null objects. Reproducible: Always Steps to Reproduce: In the "Shapes" example in the ConnectionCreateCommand class over-ride dispose() thus: public void dispose() { System.out.println("dispose called on: " + this); // Clean up some objects... target = null; } Run the "Shapes" example, draw two objects and try to draw a connection between them. dispose() is called, then execute on the same Command.