Bug Chat scripts are not run in a thread safe manner

Irrat

Member
Chat and gCLI are run in different threads, and are not thread safe. Which is a non-issue, until we introduce the chat script functionality.

If asked I could whip up something to reproduce this. But generally that would just be a chat script spamming a pref modification while a script in gCLI spams a pref modification until an error is thrown where the preferences were concurrently modified. This isn't just related to preferences only, chat scripts can do more than that after all.

Most people may not use a chat script, but it does have its uses. I myself like to use it to send myself a "chat_notify" when someone mentions me, and to communicate with other bots with the responding messages saved into a pref which the main script reads to decide the course of action. (Cagebots)

I'm not sure what a perfect solution for this would be, my current train of thought is that only one runtime_function can be called from any thread at a time.
Though for nested runtime executions, like a chat script running a cli command, or calling a cli command that calls a script that calls a cli command. I'm not sure.

Another consideration could be that chat script execution is now performed on the main thread, though what that would look like exactly I'm again not sure.

Putting aside what a chat script should and shouldn't be possible to do, setting preferences should be well within the list of possibilities.
 

Veracity

Developer
Staff member
So Preferences need to be thread safe. Considering that writes to preferences save to the file system, I completely agree.
 

Veracity

Developer
Staff member
You said "chat scripts" in general. Scripts are each run in their own Interpreter. Unless I am completely confused about how we designed and implemented those, any given Interpreter is synchronized and you cannot run the same script in more than one thread at a time.

We ran into major problems when Ezandora release Guide, which, somehow, makes the browser fire off periodic events to execute Guide, making it, essentially, a background asynchronous script - like a chat script. (It was mostly dealing with the "continuation state". Search for "guide-proof" so see various threads where we discussed this kind of issue.)

I think we solved that issue in general. But maybe not for specific operations - like getting/setting properties - which are apparently not synchronized.
 

Irrat

Member
Not what I was getting at sorry.

Here's an error I got yesterday, but the thing is. I have no idea why this error happened. My chat script should never have reached the state where it'd hit the modifiers. Which is why I'm reluctant to post it. Because I can't say why it got to that state.

Heck, the only functions it is calling are
Code:
chatClan,
  chatNotify,
  getProperty,
  myName,
  setProperty,
  toInt

Code:
Unexpected error, debug log printed.
class java.util.ConcurrentModificationException: null
java.util.ConcurrentModificationException
    at java.base/java.util.HashMap$HashIterator.nextNode(HashMap.java:1597)
    at java.base/java.util.HashMap$KeyIterator.next(HashMap.java:1620)
    at net.sourceforge.kolmafia.Modifiers.applyPassiveModifiers(Modifiers.java:2522)
    at net.sourceforge.kolmafia.KoLCharacter.recalculateAdjustments(KoLCharacter.java:4993)
    at net.sourceforge.kolmafia.Speculation.calculate(Speculation.java:141)
    at net.sourceforge.kolmafia.textui.command.SpeculateCommand.run(SpeculateCommand.java:22)
    at net.sourceforge.kolmafia.KoLmafiaCLI.doExecuteCommand(KoLmafiaCLI.java:453)
    at net.sourceforge.kolmafia.KoLmafiaCLI.executeCommand(KoLmafiaCLI.java:419)
    at net.sourceforge.kolmafia.KoLmafiaCLI.executeLine(KoLmafiaCLI.java:338)
    at net.sourceforge.kolmafia.textui.RuntimeLibrary.cli_execute(RuntimeLibrary.java:3125)
    at jdk.internal.reflect.GeneratedMethodAccessor76.invoke(Unknown Source)
    at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.base/java.lang.reflect.Method.invoke(Method.java:568)
    at net.sourceforge.kolmafia.textui.parsetree.LibraryFunction.execute(LibraryFunction.java:63)
    at net.sourceforge.kolmafia.textui.parsetree.FunctionCall.execute(FunctionCall.java:113)
    at net.sourceforge.kolmafia.textui.parsetree.BasicScope.execute(BasicScope.java:423)
    at net.sourceforge.kolmafia.textui.parsetree.Conditional.execute(Conditional.java:54)
    at net.sourceforge.kolmafia.textui.parsetree.If.execute(If.java:35)
    at net.sourceforge.kolmafia.textui.parsetree.BasicScope.execute(BasicScope.java:423)
    at net.sourceforge.kolmafia.textui.AshRuntime.executeScope(AshRuntime.java:236)
    at net.sourceforge.kolmafia.textui.AshRuntime.execute(AshRuntime.java:186)
    at net.sourceforge.kolmafia.textui.AshRuntime.execute(AshRuntime.java:179)
    at net.sourceforge.kolmafia.chat.ChatManager.invokeChatScript(ChatManager.java:687)
    at net.sourceforge.kolmafia.chat.ChatManager.processCommand(ChatManager.java:647)
    at net.sourceforge.kolmafia.chat.ChatManager.processEvent(ChatManager.java:446)
    at net.sourceforge.kolmafia.chat.ChatManager.processMessage(ChatManager.java:336)
    at net.sourceforge.kolmafia.chat.ChatManager.processMessages(ChatManager.java:323)
    at net.sourceforge.kolmafia.chat.ChatPoller.addEntry(ChatPoller.java:238)
    at net.sourceforge.kolmafia.textui.RuntimeLibrary.chat_notify(RuntimeLibrary.java:7710)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
    at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.base/java.lang.reflect.Method.invoke(Method.java:568)
    at net.sourceforge.kolmafia.textui.parsetree.LibraryFunction.execute(LibraryFunction.java:63)
    at net.sourceforge.kolmafia.textui.parsetree.FunctionCall.execute(FunctionCall.java:113)
    at net.sourceforge.kolmafia.textui.parsetree.BasicScope.execute(BasicScope.java:423)
    at net.sourceforge.kolmafia.textui.parsetree.Conditional.execute(Conditional.java:54)
    at net.sourceforge.kolmafia.textui.parsetree.If.execute(If.java:35)
    at net.sourceforge.kolmafia.textui.parsetree.BasicScope.execute(BasicScope.java:423)
    at net.sourceforge.kolmafia.textui.parsetree.UserDefinedFunction.execute(UserDefinedFunction.java:87)
    at net.sourceforge.kolmafia.textui.AshRuntime.executeScope(AshRuntime.java:258)
    at net.sourceforge.kolmafia.textui.AshRuntime.execute(AshRuntime.java:186)
    at net.sourceforge.kolmafia.textui.AshRuntime.execute(AshRuntime.java:179)
    at net.sourceforge.kolmafia.chat.ChatManager.invokeChatScript(ChatManager.java:687)
    at net.sourceforge.kolmafia.chat.ChatManager.processCommand(ChatManager.java:647)
    at net.sourceforge.kolmafia.chat.ChatManager.processMessage(ChatManager.java:373)
    at net.sourceforge.kolmafia.chat.ChatManager.processMessages(ChatManager.java:323)
    at net.sourceforge.kolmafia.chat.ChatPoller.handleNewChat(ChatPoller.java:531)
    at net.sourceforge.kolmafia.chat.ChatPoller.run(ChatPoller.java:215)
 

Veracity

Developer
Staff member
Ok. There are other data structures that are not thread safe.

Your idea is to synchronize RuntimeLibrary, say - even though lots (most?) of the functions are doing read-only operations.

Your example shows KoLmafia.recalculateAdjustments - which is definitely not a read-only operation.

So, synchronize that method?

In other words, synchronize operations that modify global data, rather than taking a big hammer and synchronizing all runtime functions, whether or not they interfere with each other.

It feels like you saw a problem and suggested a superficial solution, rather than diving deeper to identify the underlying issue and working from there.
 

Veracity

Developer
Staff member
I guess I take issue with the premise implied by the title of this thread:

“Chat scripts are not run in a thread safe manner”.

What is a “thread safe manner”?

Only one thread can run a particular script Interpreter. That seems like the essence of “running (a script) in a thread safe manner”.

And then you mention that the issue is that your script and others both modify global data which is not thread safe.

Yes. That is a problem - with the global data structures, NOT with the “manner” in which the scripts are run.
 

Irrat

Member
Yeah, the worst part is that I even rewrote the post a few times to make it more understandable.

I have a real problem with communicating my thoughts.
 

Veracity

Developer
Staff member
Consider things which can modify our internal state:

- scripts (including asynchronous things like Guide)
- commands typed into the gCLI
- stuff coming from the Relay Browser
- user actions on the GUI.

We do attempt to disable GUI components while things are running. And stop scripts and gCLI from interfering. But we absolutely do not freeze the Relay Browser. And, presumably, relay scripts like Guide.

Rather than focus on script vs. script, I’d prefer to focus on data integrity of our model.
 
Top