Larry,
I must apologize for my lack of good communication!
In the case of Pedro's bug a call to the LayerManager.addCategory() method
results in a call to the LayerManager.fireCategoryChanged() method which in
turn calls the LayerManager.fireLayerEvent() method. The
LayerManager.fireLayerEvent() method accepts a Runnable as its argument, and
as far as I can tell launches a new thread. In this thread it iterates over
the collection of LayerListeners, sending each listener the event indicating
that a Category had been added.
Pedro was loading a project with many categories, and when the exception was
thrown OpenJUMP had made 2 subsequent calls to the LayerManager.addCategory()
method. While the thread created from the first call to addCategory was
iterating over the collection of LayerListeners associated with the
LayerMananger, the second call to the LayerManager.addCategory() method
created a second thread that tried to do the same thing. This caused the
ConcurrentModificationException error to be thrown.
The solution, as David pointed out, is to simply create a copy of the
LayerListener collection before it is iterated over. Jon Aquino had already
applied this patch to several of the methods in the LayerManager class, but
somehow this one slipped through.
I was concerned that the use of threads and a copy of the LayerListener
collection in this way could lead to a "phantom" LayerListener that would be
incorrectly sent an event. After reading David's last post a couple of times
I realize this is not the case. Although event broadcasting is really a
sequential series of method calls in Java, it is in "theory" supposed to be
a "simultaneous" event. There really is no potential for error as long as
the plug-in developer understands that OpenJUMP sitcks to the theoretical
model in this case, and not the acutal sequential implementation.
It took me a while to figure all of this out. I still have a lot to learn
about Java threads and OpenJUMP in general.
I'm sorry if my earlier ommission about the use of threads in the sequence
of logic executed after the LayerManager.addCategory() method call caused
confusion.
The Sunburned Surveyor
P.S. - I will write the quick fix that David first suggested and make a copy
of the modified LayerManager class available to you for SkyJUMP. I will also
pass it along to Vivid for the JUMP CVS and will commit it to the OpenJUMP
CVS.
One bug down...Two to go!
On 1/18/07, Larry Becker <[EMAIL PROTECTED]> wrote:
Sunburned,
It sounds like you have been studying up on the JUMP core quite a bit
lately, and have reached a pretty good understanding of how it works.
However, I'm confused about what the topic of Threads has to do with this
bug fix. Events are not Threads, as I'm sure you realize. As far as I have
been able to determine, the only separate Thread that JUMP uses is the
"timer-triggered one second render update" Thread. That Thread alone has
been responsible for may hours of debugging time for us, but I don't see it
as being the cause of Pedro's bug.
It IS the source of other bugs relating to (SkyJUMP) using a DataSource
ArcSDE layer backed by an Oracle database. Oracle databases are notoriously
finicky about multiple connections, so if you try to render N ArcSDE
DataSource layers, it will try to open N connections and run Oracle out of
connection memory. But I digress :-)
regards,
Larry
On 1/18/07, Sunburned Surveyor <[EMAIL PROTECTED]> wrote:
>
> David and Larry,
>
> Thanks for the help. My comments are listed below.
>
> Larry wrote: "If I understand the bug in question, it is not easy to
> reproduce. It has been my experience that you can't fix bugs unless you can
> reproduce them, otherwise you can't know when they are fixed. All you can
> do otherwise is to apply a lot of preventative measures that may improve the
> situation, or it might even make it worse. Does anyone have a scenario that
> will cause the bug to reproduce often enough to meet this requirement? If
> not, the prudent course IMHO is to wait until such a case occurs."
>
> You make a good point Larry. I don't currently know of code in
> OpenJUMP's core that will cause this bug to occur. However, I could write a
> simple plug-in in OpenJUMP that would cause this error to occur, although it
> might not be on a rigid and repeatable basis. I think the most dangerous
> thing about this type of code is the unpredictability. I can't tell you when
> this is going to mess things up for the user, only that the possibility is
> there.
>
> In most programs this is probably not a huge problem. I think things are
> a little different with OpenJUMP. OpenJUMP is designed to be extendable and
> I can think of many reasons why a plug-in developer would want to register
> one of the objects created for his plug-in as a listener to a LayerManager.
> In most cases a listener would be removed from the list of listeners when it
> is destroyed, so the situation I described in my previous e-mail is not
> typical. That's the danger of OpenJUMP's pluggable design. How often will a
> plug-in developer write code that is typical? I thought of at least one or
> two situations in my own work on OpenJUMP where a "removed listener" might
> still be in existence after removal and the problem I mentioned previously
> could occur.
>
> Thank you for your comments. You may be correct when you point out that
> I might be spending time on a relatively minor problem that is not likely to
> occur.
>
> David wrote: " I wouldn't worry about calling a listener with the event
> after it has been de-registered because the event handlers are not
> guaranteed to be executed in any given order."
>
> I might still be confused about the way this whole thing works. I don't
> think the order that event listeners are called is a factor in the problem I
> described previously. The problem is that we are creating a copy of
> collection of LayerListeners in the LayerManager. This means that we could
> have a LayerListener in the copied collection that could be called after a
> programmer thought that it had been removed as a listener from the
> LayerManager. It doesn't matter if this "phantom" listener is sent the event
> by the LayerManager first, last, or in the middle. What matters is that it
> is sent the event at all.
>
> I suppose you could argue that the Listener should receive the event
> even though it has been removed from the LayerManager, becuase at the the
> time the action occured that generated the thread that created the copy of
> the LayerListener collection, the LayerListener was registered as a
> listener. Is this the point you were trying to make when you said that in
> the "perfect" world all events are called at the "same moment".
>
> I can accept that. If this is the case I will simple create a copy of
> the LayerListener collection in the method that is causing Pedro's bug like
> you first suggested. :]
>
> I appreciate everyone's patience as I try to understand some code that's
> a little over my head.
>
> The Sunburned Surveyor
>
>
>
> On 1/18/07, David Zwiers < [EMAIL PROTECTED]> wrote:
>
> > SS,
> >
> >
> >
> > I think you've found the issue; thanks for the explanation. I wouldn't
> > worry about calling a listener with the event after it has been
> > de-registered because the event handlers are not guaranteed to be executed
> > in any given order.
> >
> >
> >
> > In a perfect world (and the way most of us think about event handlers)
> > all the handlers for a given event are executed in tandem. Thus we could
> > create N threads (one per handler), leaving them all in a yielded state
> > prior to starting the execution of any one thread. This would also be a
> > valid implementation. If there are handlers which are order dependant (and
> > there shouldn't be any), they probably should be refactored.
> >
> >
> >
> > HTH
> >
> > David
> >
> >
> > ------------------------------
> >
> > *From:* [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED]
> > ] *On Behalf Of *Sunburned Surveyor
> > *Sent:* January 18, 2007 7:52 AM
> > *To:* List for discussion of JPP development and use.
> > *Subject:* [JPP-Devel] Update on Bug #1487099
> >
> >
> >
> > I'm afraid I have to again correct my observations on Bug #1487099. I
> > was looking at the LayerManager class this morning and realized that the
> > method causing Pedro's ConcurrentModificationException hasn't yet been fixed
> > by creating a copy of the LayerListener collection. This fix was applied by
> > Jon Aquino to some other methods.
> >
> > After chewing over the problem for a while last night I realize that
> > creating a copy of the LayerListener collection stored by the LayerManager
> > class isn't a totally "thread safe" solution to this problem. It does
> > eliminate the possibility of the ConcurrentModificationExceptions but I
> > think it would allow a LayerListener to be sent an event when it should not
> > receive them.
> >
> > Let me give a quick example of this scenario.
> >
> > I did a little work on a Data Source Catalog that allows the user to
> > associate Layers and DataSources. As part of this extension for OpenJUMP I
> > might have created a LayerListener that presents a dialog box to the user
> > when a layer is deleted, or removed from the LayerManager. This dialog box
> > would ask the user if they would also like to delete any DataSources
> > associated with the Layer. Similarly, the LayerListener might present a
> > dialog box when a Layer is added to the LayerManager asking the user if they
> > would like to associate the new Layer with a DataSource.
> >
> > These 2 dialog boxes could get a little irritating, so I might include
> > an option that allows the user to turn it "off". This could easily be done
> > by removing the LayerListener from the LayerManager. This might also be done
> > programatically by other plug-ins that add or remove layers as part of their
> > operation.
> >
> > Here is the problem.
> >
> > The user performs some action that will cause a Layer to be added or
> > removed from the LayerManager. When this happens the LayerManager creates a
> > copy of the LayerListeners collection and begins to iterate over it calling
> > the appropriate methods of the LayerListeners in a new thread.
> >
> > The user then performs another action while this thread is running
> > that causes one of the LayerListeners to be removed from the LayerManager's
> > collection of LayerListeners. This LayerListener has not been removed from
> > the copy of the LayerListener collection that is being manipulated by the
> > thread created previously when a Layer was added or removed. That means the
> > LayerManager will pass the LayerChanged event to a LayerListener that is no
> > longer supposed to receive this event because it is present in the copied
> > collection. (It is no longer present in the original LayerListener
> > collection.)
> >
> > I know this would be a rare occurence, but it is possible. I think it
> > leaves the potential for some hard-to-locate bugs in the future. I believe
> > this problem can be solved in a thread safe way by restricting access the
> > the LayerListeners collection with synchronized methods.
> >
> > Like I said, I'm no expert at threads, and If I'm not understanding
> > this correctly I'd really appreciate some help!
> >
> > If my theory about this situation isn't all messed up I'll get started
> > on the "thread safe" solution for the next release of OpenJUMP. I'll work
> > with Vivid Solutions to get it incorporated into JUMP as well.
> >
> > Thanks for any input on this situation.
> >
> > The Sunburned Surveyor
> >
> >
> > -------------------------------------------------------------------------
> >
> > Take Surveys. Earn Cash. Influence the Future of IT
> > Join SourceForge.net's Techsay panel and you'll get the chance to
> > share your
> > opinions on IT & business topics through brief surveys - and earn cash
> >
> > http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
> >
> > _______________________________________________
> > Jump-pilot-devel mailing list
> > Jump-pilot-devel@lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/jump-pilot-devel
> >
> >
> >
>
>
> -------------------------------------------------------------------------
> Take Surveys. Earn Cash. Influence the Future of IT
> Join SourceForge.net's Techsay panel and you'll get the chance to share
> your
> opinions on IT & business topics through brief surveys - and earn cash
>
> http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
>
> _______________________________________________
> Jump-pilot-devel mailing list
> Jump-pilot-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/jump-pilot-devel
>
>
>
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share
your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
Jump-pilot-devel mailing list
Jump-pilot-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/jump-pilot-devel
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
Jump-pilot-devel mailing list
Jump-pilot-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/jump-pilot-devel
