Menu
▾
▴
#16 TagSEA will sometimes lock the workbench on shutdown
There is some sort of race condition in the workbench/workspace. Follow these steps to reproduce:
1. Start Eclipse with a project opened which has a lot of waypoints in it.
2. Close the project
3. Shut down Eclipse
If Eclipse is shut down before TagSEA has the opportunity to delete all of the waypoint markers for the recently closed project, then the workbench will lock up and Eclipse won't shut down. There seems to be a race condition with gaining a lock on the workspace.
Discussion
I am a member of one of the groups working on the TagSEA Project.
I have been working hard to determine the cause of this bug. Here is what I have figured out so far.
In ResourceWaypointUI.java a null point exception occurs at the following line of code.
in:
public void handleEvent(Event event) {
if (!value.isDisposed()) {
In this case value is null and the key is not null but of type {*DISPOSED*}
I believe key in the Iterator<Image> it = map.keySet().iterator(); key set is being flagged as disposed. Thus changing the key to {*DISPOSED*} and when
Image value = map.get(key); is called in the form of:
Image value = map.get({*DISPOSED*}); no value is returned thus value is null.
I notice when I run the plugin:
In:
public void put(Image base, Image target) {
The following calls are made at the very start of the program, before everything has been initialized and I can see the tags.
baseDisp.addListener(SWT.Dispose, this);
targetDisp.addListener(SWT.Dispose, this);
I also notice that ordering of the following block of code does not matter.
Display baseDisp = (Display)base.getDevice();
if (!displays.contains(baseDisp)) {
System.out.println("Dispose Base: "+base.toString());
baseDisp.addListener(SWT.Dispose, this);
displays.add(baseDisp);
}
Display targetDisp = (Display)target.getDevice();
if (!displays.contains(targetDisp)) {
System.out.println("Dispose Target: "+target.toString());
targetDisp.addListener(SWT.Dispose, this);
displays.add(targetDisp);
}
If you put one before the other, the first one is always called. I have put them together in the same and statement and they still work, such that:
if (!displays.contains(targetDisp) && !displays.contains(baseDisp))) {
I believe the public void put(Image base, Image target) { method could be a primary source of the problem.
In public void put(Image base, Image target) { the map size is 0.
In public void handleEvent(Event event) { the map size is 1 with the value {*DISPOSED*}
Here is a short subset of the debug of output:
.....
21:49:22,114 DEBUG [B] Instance [29706031] created.
getImage(IWaypoint net.sourceforge.tagsea.model.internal.Waypoint@0)
ImageKeyedImageRegistryCreates new HashMap and HashSet
get Image {2047151512}
put(Image BaseImage {2047151512}, Image TargetImage {-301657345})
Begin Iteration
it.hasNext() false map size 0
End Iteration
baseDisp.toString()org.eclipse.swt.widgets.Display@13136e5 targetDisp.ToString()org.eclipse.swt.widgets.Display@13136e5
Dispose Base: Image {2047151512}
Dispose Target: Image {-301657345}
Only one SWT.Dispose works.
The above Dispose Base: text is when the call to:
baseDisp.addListener(SWT.Dispose, this);
or
targetDisp.addListener(SWT.Dispose, this);
So this is what I have discovered so far, I am still working hard to figuring out the root of the bug.
Hi David,
I've been looking into this report, and I can't figure out how it could be related to the workbench being locked.
The {DISPOSED} information that you are seeing is simply what the toString() method on a Widget displays when it has been disposed. It doesn't affect the map since the reference to the object is still valid, and the hashcode of the object is still valid (the system hash code is used, so using it as a key to a map should still be fine). In this case, null will not be returned, otherwise you would get a NullPointerException.
Could you please explain to me how this could be related to the race condition in the workbench. When changing the code, does the behaviour of Eclipse change such that it no longer gets locked up?
As for the information that you put in about the target and base displays: I haven't gotten a chance to look into it deeply. However, I don't think that I would like to incorporate your change because I believe the effects that you are seeing is due to the fact that most Eclipse applications run in a single display thread, and not because of anything wrong with the logic. If the program was being run with multiple displays (not multiple monitors... but on multiple machines, each with their own display thread), you might get a different result.
Hello Del,
From what I have observed:
1. The work bench locking is due to a NullPointerException
2. The "base" Image is the key to the map
3. The "base" Image is to my knowledge flagged as "disposed"
4. When the base is flagged as "disposed" the value of the base Image is modified
5. As the Image object is used as key to the map, the disposed modification changes they key value.
6. Upon shutdown, the plugin tries to locate corresponding "target" Images based on the "modified" base Image object value, this fails and returns null as no related value (target Image) is recovered from the modified "disposed" "base" Image object.
My modifications to the code are as follows:
1. Instead of using the "base" Image as the key I now you the unique identifier which is the ToString() of the base value. I discovered that the ToString() of each image gives a unique identifier and thus key. (runtime is question of the ToString() method)
2. I thus used the ToString() of the "base" Image as the key
3. I used a list to hold "base" and "target" Images
4. I desired to modify as little code as possible, not changing the functionality of existing methods, thus the final iteration of the fix.
5. Functionality of the methods remains the same, I refactored one while loop to make it easier to read
Summary:
1. Upon using the ToString() of the "base" Image, the modification of the "base" Image is negligible.
2. Using the string as the primary key, The "base" entry in the list and the "target" Image are found correctly using this key
3. Further code associated with checking if the value is disposed or not functions normally.
4. Specifically List at position 0 = "base" and base.isDisposed() == true, "target" Image, target.isDisposed() == false
5. The issue of calling target.isDisposed() when target = null is not longer an issue as the string key correctly locates the associated "target" value.
I hope this clarifies some of the issues I discovered and rationale regarding the fixes I have implemented.
The focus was minimizing modification to the existing code base, there are alternative solutions but would require more refactoring.
Thanks,
David
Hello Del,
To further clarify, I do believe the hash of the image (or "value" of the Image) is changed when it is flagged as disposed.
Using the string of the base as the key eliminates all locking up at shutdown.
From testing I discovered the flagging of the Image object as disposed changes the key.
This is purely based on my testing alone.
This is one of the resources I used.
http://www.netalive.org/swsu/archives/2005/08/disposing_resou.html
Although I did not directly use it as a resource only to get me thinking about potential issues.
I did analyze properties of the Image object before and after it was disposed and it looks like the Image internal properties were modified.
David
So the patch I have submitted and the comments below are in relation to the NullPointerException.
TagSEA will now close properly when one tag is in the system (before this was not the case). As you mentioned more tags cause the shutdown of Eclipse to hang.
I will investigate more into the cause of hanging.
I believe there are two bugs here, the null pointer exception and maybe a race condition as you mentioned.
I will investigate,
Cheers!
David
Hi David,
I looked more into the Image class, and it does seem that the hash code does change after the Image is disposed. I didn't expect this behaviour because the documentation of the hashCode() method clearly indicates that the hash code must not change over the life time of the object.
Images, however, use the operating system handle as the hash code of the object, and it gets set to 0 when the Image is disposed. This makes all disposed Images look "equal" according to the collections framework. Images, it seems, don't play well with the collections framework. So, this is a good catch on your part.
But using "toString()" as a key won't work either as the toString() value of the Image also changes when it is disposed. Ultimately, you would end up with pretty much the same problem.
An alternate solution would be to use the System hash code of the Image as the key.
I'm still not sure that this problem is actually the root cause, however. It might be an issue if a thrown exception is causing a thread that has a lock on the workspace to die before it releases the lock. Do you have a stack trace that might indicate something like that is going on?
The fact that your solution so far "fixes" the problem for when there is only one tag in the system seems to indicate that it doesn't actually solve the problem yet. It could just be a happy coincidence that the one tag can be deleted without a race condition occurring.
Try catching and printing all exceptions in the handleEvent() and getImage() methods in the ResourceWaypointUI class. It will probably cause a memory leak, but if it solves the problem of the system hanging then you have some evidence that you are on the right track.
Hello Del,
Thanks! In regards to using the ToString() as the key to the map.
I create the string key before the value is flagged as disposed. From my observations, the system remembers the key before it was disposed. Therefore I shouldn't see to many issues from using the ToString() as the key. More specifically, When I add the base to the map I obtain its ToString() key, then the value is disposed. Even if the ToString() of the stored base is modified over the lifetime of the plugin, the base Image TagSEA is looking for remains unchanged, therefore when looking up the key in the map, I ToString() the "base" Image TagSEA is looking for to match with the unchanged string in the system.
In the form of:
map<string key, List<Image>> where List<Image> contains at 0 base Image, and at 1 target Image
When get(Image base) is invoked:
I look up in the map. map.get(base.ToString()) which searches for the corresponding string key.
Where: List<Image> contains at 0 base Image = "disposed", and at 1 target Image = NOT "disposed".
the ToString() is essentially operating as a hash of the Image as a key which is obtained before it is modified.
If you would like me to change this implementation let me know.
Right now I am trying to determine where the race condition occurs, I will analyze handleEvent closely, any other related classes I should be looking at?
Thanks,
David
Hello again,
I'm sorry to be so blunt, but using toString() is just a bad idea all the way around. Look at the documentation for toString(). The only purpose for toString() is for an "informative representation that is easy for a person to read." There is no way to guarantee that the authors of SWT will not change their definition of "informative". toString() should never be thought of being consistent through the lifetime of an object, and therefore should not be used as a key into anything in the collections framework.
There is probably something wrong with the logic of this class, but I wouldn't go too far down this train anyway, if it in fact isn't the root cause of the problem. Like I said, try trapping all exceptions in the class first to make sure that it can't cause any thread death. Once you've done that, if it fixes the problem, then start trying to look for cleaner solutions. There is no point in following rabbit trails.
There is nothing wrong with being blunt. I have only created a solution based on what was given to me. Given alternative resources, I could render an alternative solution, now the hash code will work as a key as well. I am well aware of the informative nature of the ToString method. I did not really consider this to be a finalized solution, just as a model to solve this particular exception given the base working code. I have asked for recommendations and suggestions regarding this particular bug I found.
As I stands I am currently researching the nature of concurrency logic errors that have not been accounted for.
I will do as suggested.
Thanks :)