Bug Unexpected error when opening item manager

Ryo_Sangnoir

Developer
Staff member
When viewing the item manager (for food), or consuming food through the item manager, I get an unexpected error (everything works as normal, though -- it freezes but recovers itself). There are two main stack traces:

Code:
Unexpected error, debug log printed.
class java.lang.NullPointerException: null
java.lang.NullPointerException
	at net.sourceforge.kolmafia.swingui.panel.UseItemEnqueuePanel$ConsumableFilterField.isVisible(UseItemEnqueuePanel.java:824)
	at net.java.dev.spellcast.utilities.LockableListModel.updateSingleFilter(LockableListModel.java:828)
	at net.java.dev.spellcast.utilities.LockableListModel.updateFilter(LockableListModel.java:801)
	at net.sourceforge.kolmafia.persistence.ConcoctionDatabase.refreshConcoctionsNow(ConcoctionDatabase.java:1549)
	at net.sourceforge.kolmafia.persistence.ConcoctionDatabase.refreshConcoctions(ConcoctionDatabase.java:1380)
	at net.sourceforge.kolmafia.request.GenericRequest.execute(GenericRequest.java:1635)
	at net.sourceforge.kolmafia.request.GenericRequest.run(GenericRequest.java:1349)
	at net.sourceforge.kolmafia.request.ChatRequest.run(ChatRequest.java:126)
	at net.sourceforge.kolmafia.request.RelayRequest.getTabbedChatMessages(RelayRequest.java:3391)
	at net.sourceforge.kolmafia.request.RelayRequest.handleChat(RelayRequest.java:3298)
	at net.sourceforge.kolmafia.request.RelayRequest.run(RelayRequest.java:3528)
	at net.sourceforge.kolmafia.RequestThread.postRequest(RequestThread.java:286)
	at net.sourceforge.kolmafia.RequestThread.postRequest(RequestThread.java:249)
	at net.sourceforge.kolmafia.webui.RelayAgent.readServerResponse(RelayAgent.java:596)
	at net.sourceforge.kolmafia.webui.RelayAgent.performRelay(RelayAgent.java:162)
	at net.sourceforge.kolmafia.webui.RelayAgent.run(RelayAgent.java:135)

and

Code:
Unexpected error, debug log printed.
class java.lang.IllegalArgumentException: Comparison method violates its general contract!
java.lang.IllegalArgumentException: Comparison method violates its general contract!
	at java.util.ComparableTimSort.mergeHi(Unknown Source)
	at java.util.ComparableTimSort.mergeAt(Unknown Source)
	at java.util.ComparableTimSort.mergeCollapse(Unknown Source)
	at java.util.ComparableTimSort.sort(Unknown Source)
	at java.util.Arrays.sort(Unknown Source)
	at java.util.Arrays.sort(Unknown Source)
	at java.util.ArrayList.sort(Unknown Source)
	at java.util.Collections.sort(Unknown Source)
	at net.java.dev.spellcast.utilities.LockableListModel.sort(LockableListModel.java:164)
	at net.java.dev.spellcast.utilities.LockableListModel.sort(LockableListModel.java:155)
	at net.sourceforge.kolmafia.persistence.ConcoctionDatabase.refreshConcoctionsNow(ConcoctionDatabase.java:1545)
	at net.sourceforge.kolmafia.persistence.ConcoctionDatabase.refreshConcoctions(ConcoctionDatabase.java:1380)
	at net.sourceforge.kolmafia.request.GenericRequest.execute(GenericRequest.java:1635)
	at net.sourceforge.kolmafia.request.GenericRequest.run(GenericRequest.java:1349)
	at net.sourceforge.kolmafia.request.ChatRequest.run(ChatRequest.java:126)
	at net.sourceforge.kolmafia.request.RelayRequest.getTabbedChatMessages(RelayRequest.java:3391)
	at net.sourceforge.kolmafia.request.RelayRequest.handleChat(RelayRequest.java:3298)
	at net.sourceforge.kolmafia.request.RelayRequest.run(RelayRequest.java:3528)
	at net.sourceforge.kolmafia.RequestThread.postRequest(RequestThread.java:286)
	at net.sourceforge.kolmafia.RequestThread.postRequest(RequestThread.java:249)
	at net.sourceforge.kolmafia.webui.RelayAgent.readServerResponse(RelayAgent.java:596)
	at net.sourceforge.kolmafia.webui.RelayAgent.performRelay(RelayAgent.java:162)
	at net.sourceforge.kolmafia.webui.RelayAgent.run(RelayAgent.java:135)

I'm running the latest mafia on Java 8.
 

Veracity

Developer
Staff member
FWIW, neither of those stack traces was triggered from the Item Manager.
They both occurred while processing chat messages from the relay browser.
In both cases, they were refreshing concoctions and manipulating the "usables" list.
We do that at the end of processing every request - even chat requests, which seems like a waste of effort.

Requests are in different threads, and refreshing concoctions is synchronized. So are all operations on a LockableListModel which modifies the list - but simply looking at the list (as in the Item Manager) is not, so if another thread is manipulating it, race condition.

Does this happen to you often? I've never seen anything like this.
 

Veracity

Developer
Staff member
I don't feel like playing with the guts of LockableListModel right now. It would not be hard to make the "get" type operations synchronize on actualElements, but I don't have time to experiment and test and see how things operate under stress. Maybe later.

I will submit a tiny patch to make chat requests not refresh concoctions, since they should be unable to affect concoctions at all; you can use chat acommands to "use" or what have you, but those all actually result in real "use"-type requests, not as chat requests. That will not fix your root cause - although it should prevent the exact exceptions you are reporting - but it will make KoLmafia more efficiently, at least.

Revision 19603.
 

Ryo_Sangnoir

Developer
Staff member
They tend to come in a rush -- I'll have a long run (many sessions over weeks) with no problems at all, then I'll have a session full of this. In that case, I normally just close and re-open mafia, and the problem goes away. It's been happening more often more recently, but I doubt there's actually a pattern here -- it's probably just luck.

Thanks for the change!
 

Veracity

Developer
Staff member
I don't feel like playing with the guts of LockableListModel right now. It would not be hard to make the "get" type operations synchronize on actualElements, but I don't have time to experiment and test and see how things operate under stress. Maybe later.
I recall the last time I looked into synchronization in the LockableListModel. It is, in spite of its name, not "Lockable". Here's a comment at the top of the class:

Code:
/**
 * Lockable aspects of this class have been removed due to incompatibilities with Swing; synchronization between two
 * threads when one is the Swing thread turns out to have a lot of problems. It retains its original name for
 * convenience purposes only.
 */
That is the crux.

Ideally, when you have a shared data structure like this which can be read and/or modified by multiple threads, you'd want something like this in each thread that manipulates it:

lock the data structure
do all the reads/writes/deletes/whatever on the data structure
unlock the data structure.

What we have, after my extensive synchronization effort on the class is this:

When we need to modify the data structure:

lock
add/delete/sort/whatever
unlock

This keeps the structure from getting corrupted from concurrent modification. We haven't seen that kind of error for years.

When we need to read the data structure:

Do it. Read/search/"contains"/whatever

This will result in errors if another thread has the structure locked down and is modifying it at the same time.

The big problem is that our main (but not only) "reader" of these data structures is Swing. If the data structure is "locked" when Swing wants to read it, the GUI freezes. It does eventually unfreeze, but a non-responsive GUI is a major UX issue. Plus, Swing is not structured to "lock" a whole data structure. When it is notified that a data model has changed, it expects to be able to iterate over the elements and display them (or not), with the expectation that the structure is not actively changing out from under it.

Hence the above comment in the source code. Perhaps KoLmafia could have been designed differently from the beginning to be more robust with how it interacts with Swing. But it's been this way since before I joined the project, and although I have made things a lot more solid, there are still flaws.

And I just don't have time to re-engineer it.
 

fronobulax

Developer
Staff member
Among the areas where my ignorance exceeds my knowledge is Swing in general and multi-threading. In that spirit I have a vague recollection that when you wanted to lock something that Swing was using you spawned another thread, did the locking things there and had some kind of callback to tell Swing that whatever it wanted to do is now safe. SwingWorker perhaps? I'll see if looking at the code jogs my memory but I figured I'd say something because folks here are pretty good about telling me when I am chasing my tail or going down a rabbit hole.
 
Top