Fix variable & function thread safety

[ericd590]

Problem

Calling the same function multiple times from different threads results previously resulted the wrong value being returned. This is now fixed.
Variables were previously not always set to the correct value when multiple threads are modifying unrelated variables at the same time. This is now fixed.

Solution

1. Function return values are now ThreadLocal to prevent multiple threads accessing the same return values or keys.
2. The variable queue is now contained within the read lock when getting a variable, preventing races while queue is being processed.
3. The cache for variables and the function expression are synchronized.

Testing Completed

This was performed in vanilla Skript and skript-reflect. You can see how the vanilla tests were performed but make use of some uncommon features.

Vanilla tests

Click to view vanilla tests

• The test /test-functions tests that a function will always return the intended value when multiple threads are running the function at the same time.
• The test /test-variables tests that a variable will always be set to the correct value when multiple threads are modifying unrelated variables at the same time.

Steps to run:

1. Add the files below to your scripts folder
2. Set the following value to 10 or higher in your config. This is the only way (that I know of) that to run code asynchronously without addons.
3. Run /test-functions and /test-variables

You must have the following files in your scripts folder

loader.sk

function identity(input: object) :: object:
    return {_input}

command /test-variables:
    trigger:
        # each load is a separate thread
        loop 10 times:
            reload script file "test-variables"

command /test-functions:
    trigger:
        # each load is a separate thread
        loop 10 times:
            reload script file "test-functions"

test-functions.sk

on async load:
    set {_threadId} to random uuid
    clear {-test::%{_threadId}%}
    loop 50000 times:
        clear {_input}
        set {_input} to random integer between -2147483648 and 2147483647
        if identity({_input}) is not {_input}:
            broadcast "impossible"
    broadcast "finished"

test-variables.sk

on async load:
    set {_threadId} to random uuid
    clear {-test::%{_threadId}%}
    loop 500000 times:
        add 1 to {-test::%{_threadId}%}
    broadcast "%{-test::%{_threadId}%}%/500000"

Outputs

Note: The amount of repetitions were lowered during some of the tests before the changes to limit the responses, but were also tested without changes to the tests.

Before:

After:

Tests written using skript-reflect

• The test /test-reflect-functions tests that a function will always return the intended value when multiple threads are running the function at the same time.
• The test /test-reflect-variables tests that a variable will always be set to the correct value when multiple threads are modifying unrelated variables at the same time.

command /test-reflect-variables:
    permission: op
    trigger:
        clear {-test::*}
        create new section with {_threadId} stored in {_run}:
            loop 500000 times:
                add 1 to {-test::%{_threadId}%}
            broadcast "#%{_threadId}% %{-test::%{_threadId}%}%/500000"
        loop 5 times:
            run section {_run} async with loop-value

import:
    java.lang.Integer

function identity(input: object) :: object:
    return {_input}

command /test-reflect-functions:
    permission: op
    trigger:
        create new section with {_threadId} stored in {_run}:
            loop 50000 times: 
                set {_input} to random integer between Integer.MIN_VALUE and Integer.MAX_VALUE
                # testReturn returns the input. They should ALWAYS be the same.
                set {_result} to identity({_input})
                if {_result} is not {_input}:
                    broadcast "impossible return value. input: %{_input}%, result: %{_result}%"
                clear {_result}, {_input}
            broadcast "Completed thread #%{_threadId}%"
        loop 10 times:
            run section {_run} async with loop-value

Outputs

Note: The amount of repetitions were lowered during some of the tests before the changes to limit the responses, but were also tested without changes to the tests.

Before Changes:

After Changes:

Supporting Information

---

Completes: none
Related: none

[TheMug06]

Thank you for your interest in contributing to Skript! Note that you should target either dev/patch or dev/feature (depending on the PR), instead of master!

[sovdeeth]

Looks good to me, thanks for doing this. It's been on the list for a while now but never much of a priority

Have you tested to see if there's any impact to variable access speeds? I wouldn't expect one, but best to be sure.

[ericd590]

For single-threaded usage, it will have the exact same performance as the write lock can never be held while reading.

In theory it would be ever so slightly slower for multi-threaded usage as it would need to wait for writes before checking the queue, but its a negligible difference and necessary to prevent races.

Variables always having the correct value is infinitely more important than any loss in performance.

[sovdeeth]

For single-threaded usage, it will have the exact same performance as the write lock can never be held while reading.

In theory it would be ever so slightly slower for multi-threaded usage as it would need to wait for writes before checking the queue, but its a negligible difference and necessary to prevent races.

Variables always having the correct value is infinitely more important than any loss in performance.

I know, I'm just curious if it has any measurable impact

[ericd590]

Single-threaded performance

Before changes:

After changes:

Multi-threaded performance

Before changes:

After changes:

Code used

[sovdeeth]

wonderful thanks!