Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Edge Case Running Status of Sandbox #205

Merged
merged 2 commits into from
Jul 23, 2024

Conversation

IntegerLimit
Copy link
Contributor

@IntegerLimit IntegerLimit commented Jul 23, 2024

This PR fixes edge cases where the running status of the GroovyScript sandbox is reported, falsely, as not running.

Reproduction Case:

import gregtech.integration.groovy.GroovyScriptModule
import net.minecraft.item.ItemStack

println('Hello World!')

println(GroovyScriptModule.isCurrentlyRunning())

var test = { ItemStack stack ->
    println("Hello World (2)! " + stack.toString())
}

test(item('minecraft:apple'))

println(GroovyScriptModule.isCurrentlyRunning())

This script uses GT just to report the status of the sandbox, but it should work without gt dependency. This was reproduced in an instance with just GroovyScript and its deps, + GT and its deps.

Output of running the above script:

[21:59:45] [SERVER/INFO] [groovyscript]: ========== Reloading Groovy scripts ==========
[21:59:45] [SERVER/INFO] [placeholdername]: Running scripts in loader 'postInit'
[21:59:45] [SERVER/INFO] [placeholdername]:  - running main
Hello World!
true
Hello World (2)! 1xitem.apple@0
false

Copy link
Contributor

@Wizzerinus Wizzerinus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that,

@brachy84
Copy link
Member

Wtf? How does that fix your issue?

@IntegerLimit
Copy link
Contributor Author

Fixing review comment now. I believe the fix still works with the proper fix, but will recheck.

@brachy84
Copy link
Member

brachy84 commented Jul 23, 2024

Its closures... When they stop running is always set to false

@IntegerLimit
Copy link
Contributor Author

Its closures... When they stop running is always set to false

Breaks for closures... in scripts themselves, outside of event handlers, etc.

@IntegerLimit
Copy link
Contributor Author

Fix tested, works as intended.

@IntegerLimit
Copy link
Contributor Author

Essentially, here's why I think this issue occurs:
Closures running inside scripts still have that runClosure method called to run them.
When the closure finishes, running is set to false, therefore breaking the status.

Closures running in scripts are... pretty common.

@brachy84 brachy84 merged commit cca1525 into CleanroomMC:master Jul 23, 2024
@brachy84 brachy84 added bug Something isn't working sandbox For internal changes to the sandbox labels Aug 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working sandbox For internal changes to the sandbox
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants