refactor: replace usage of ThreadManagerSubsystem with reactor Scheduler #4799

pollend · 2021-06-27T02:31:14Z

This replaces the ThreadManagerSubsystem with Scheduler provided by rxjava. pretty straightforward since this subsystem is only ever used in the core engine and not to an extent that would make sense. the threads allocated are mostly unused and sit idle.

toTest:

verify that game previews are saved
verify that a screen shot could be taken with F12
game startup is dependent on some of this threading.

Contributes to #4798

pollend · 2021-06-27T02:32:07Z

engine/src/main/java/org/terasology/engine/core/GameThread.java

+    public static Scheduler io() {
+        return AccessController.doPrivileged((PrivilegedAction<Scheduler>) Schedulers::io);
+    }
+
+    public static Scheduler newThread() {
+        return AccessController.doPrivileged((PrivilegedAction<Scheduler>) Schedulers::newThread);
+    }
+
+    public static Scheduler computation() {
+        return AccessController.doPrivileged((PrivilegedAction<Scheduler>) Schedulers::computation);
+    }
+


we need to have these in a privileged scope.

Why do they need to be in privileged scope? Let's document this here for the future.

a lot of thread actions are guarded by the sandbox.

lgtm-com · 2021-06-27T02:43:33Z

This pull request introduces 1 alert when merging a01066a into 614315e - view on LGTM.com

new alerts:

1 for Dereferenced variable may be null

lgtm-com · 2021-06-27T19:10:29Z

This pull request introduces 1 alert when merging c9b8f11 into 7409510 - view on LGTM.com

new alerts:

1 for Dereferenced variable may be null

lgtm-com · 2021-06-27T21:26:13Z

This pull request introduces 1 alert when merging eb44e11 into 7409510 - view on LGTM.com

new alerts:

1 for Dereferenced variable may be null

lgtm-com · 2021-06-27T23:34:57Z

This pull request introduces 1 alert when merging 11a257a into fbeed19 - view on LGTM.com

new alerts:

1 for Dereferenced variable may be null

PR: #4799

lgtm-com · 2021-06-29T13:22:19Z

This pull request introduces 1 alert when merging 23b174e into fbeed19 - view on LGTM.com

new alerts:

1 for Dereferenced variable may be null

skaldarnar

Please also have a look at the alert reported by LGTM

skaldarnar · 2021-07-03T14:36:14Z

engine/src/main/java/org/terasology/engine/core/GameThread.java

@@ -18,14 +23,33 @@
 * </ul>
 *
 */
+@API


Why did the GameThread become public API?

do we need a separate API?

see #4799 (comment)

skaldarnar · 2021-07-03T14:36:42Z

engine/src/main/java/org/terasology/engine/core/GameThread.java

+    public static Scheduler io() {
+        return AccessController.doPrivileged((PrivilegedAction<Scheduler>) Schedulers::io);
+    }
+
+    public static Scheduler newThread() {
+        return AccessController.doPrivileged((PrivilegedAction<Scheduler>) Schedulers::newThread);
+    }
+
+    public static Scheduler computation() {
+        return AccessController.doPrivileged((PrivilegedAction<Scheduler>) Schedulers::computation);
+    }
+


Why do they need to be in privileged scope? Let's document this here for the future.

skaldarnar · 2021-07-03T14:38:24Z

engine/src/main/java/org/terasology/engine/core/module/ExternalApiWhitelist.java

+            .add("io.reactivex.rxjava3.annotations")
+            .add("io.reactivex.rxjava3.core")
+            .add("io.reactivex.rxjava3.disposables")
+            .add("io.reactivex.rxjava3.exceptions")
+            .add("io.reactivex.rxjava3.flowables")
+            .add("io.reactivex.rxjava3.functions")
+            .add("io.reactivex.rxjava3.internal")
+            .add("io.reactivex.rxjava3.observables")
+            .add("io.reactivex.rxjava3.observers")
+            .add("io.reactivex.rxjava3.parallel")
+            .add("io.reactivex.rxjava3.plugins")
+            .add("io.reactivex.rxjava3.processors")
+            .add("io.reactivex.rxjava3.schedulers")
+            .add("io.reactivex.rxjava3.subjects")
+            .add("io.reactivex.rxjava3.subscribers")


How did you decide what to add here, and what is not added (assuming that there must be something we leave out, or otherwise we could have used a single entry io.reactivex.rxjava3).

This particular diff has been changed to replace the rxjava packages with reactor packages, but the same question still applies.

or otherwise we could have used a single entry io.reactivex.rxjava3

I took a look at the places that use this list (which is both in this repo and nui-reflect), and it seemed like it was all .equals without any wildcard or prefix matching.

skaldarnar · 2021-07-03T14:41:30Z

facades/PC/src/main/java/org/terasology/engine/Terasology.java

@@ -153,16 +154,16 @@ public static void main(String[] args) {
                engine.run(new StateHeadlessSetup());
            } else if (loadLastGame) {
                engine.initialize(); //initialize the managers first
-                engine.getFromEngineContext(ThreadManager.class).submitTask("loadGame", () -> {


It looks like with ThreadManager we could give the tasks a name such that they are identifiable later, e.g., in logs. Is there a similar concept in RxJava?

It looks like there is a subscriber Context that is useful for things like this.

#4839 added a GameScheduler.scheduleParallel method that takes a name which it uses in the same way as ThreadManager.submitTask did.

It doesn't address future use cases where we take advantage of Flux's model for pipelines, but it suffices for one-shot Runnables like this.

should be fine for our use cases. i'm not too happy but it should be good for our use cases at this point. not really a ThreadMonitor though :?

….com:MovingBlocks/Terasology into refactor/remove-usage-ThreadManagerSubsystem

lgtm-com · 2021-07-04T05:40:24Z

This pull request introduces 1 alert when merging cc44241 into acb3181 - view on LGTM.com

new alerts:

1 for Dereferenced variable may be null

lgtm-com · 2021-07-04T06:23:29Z

This pull request introduces 1 alert when merging 7cf9a36 into acb3181 - view on LGTM.com

new alerts:

1 for Dereferenced variable may be null

lgtm-com · 2021-07-04T15:24:36Z

This pull request introduces 1 alert when merging 875b4d3 into d3d4a8c - view on LGTM.com

new alerts:

1 for Dereferenced variable may be null

This reverts commit 27e8b0e.

…isting ThreadMonitor

keturn · 2021-08-14T17:45:47Z

GameScheduler is marked @API. gameMain() provides access to scheduling on GameThread, which is not @API.

I brought this up in today's meeting, and it wasn't clear to any of us if/when modules need access to this.

skaldarnar's proposal was to leave this implemented, but at a permission level modules wouldn't be able to actually use, with a comment along the lines of "change this if we have a documented use case for it" or something like that.

(I don't think we can implement it exactly as proposed, because I don't think the @API system gives us per-method control, but we can make it package-private or something.)

pollend · 2021-08-15T14:13:55Z

GameScheduler is marked @API. gameMain() provides access to scheduling on GameThread, which is not @API.

I brought this up in today's meeting, and it wasn't clear to any of us if/when modules need access to this.

skaldarnar's proposal was to leave this implemented, but at a permission level modules wouldn't be able to actually use, with a comment along the lines of "change this if we have a documented use case for it" or something like that.

(I don't think we can implement it exactly as proposed, because I don't think the @API system gives us per-method control, but we can make it package-private or something.)

how do you propose letting a module schedule something back on the main thread?

DarkWeird · 2021-08-18T14:54:00Z

Try to take screenshot:

17:48:07.881 [main] ERROR o.t.engine.core.TerasologyEngine - Uncaught exception, attempting clean game shutdown
java.lang.ExceptionInInitializerError: null
	at reactor.core.scheduler.Schedulers.newParallel(Schedulers.java:545)
	at reactor.core.scheduler.Schedulers.lambda$static$4(Schedulers.java:1135)
	at reactor.core.scheduler.Schedulers.cache(Schedulers.java:1168)
	at reactor.core.scheduler.Schedulers.parallel(Schedulers.java:238)
	at org.terasology.engine.core.GameScheduler.scheduleParallel(GameScheduler.java:69)
	at org.terasology.engine.rendering.opengl.ScreenGrabber.saveScreenshot(ScreenGrabber.java:112)
	at org.terasology.corerendering.rendering.dag.nodes.FinalPostProcessingNode.process(FinalPostProcessingNode.java:173)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1541)
	at org.terasology.engine.rendering.world.WorldRendererImpl.render(WorldRendererImpl.java:350)
	at org.terasology.engine.core.modes.StateIngame.render(StateIngame.java:214)
	at org.terasology.engine.core.subsystem.lwjgl.LwjglGraphics.postUpdate(LwjglGraphics.java:76)
	at org.terasology.engine.core.TerasologyEngine.tick(TerasologyEngine.java:509)
	at org.terasology.engine.core.TerasologyEngine.mainLoop(TerasologyEngine.java:460)
	at org.terasology.engine.core.TerasologyEngine.runMain(TerasologyEngine.java:436)
	at org.terasology.engine.core.TerasologyEngine.run(TerasologyEngine.java:402)
	at org.terasology.engine.Terasology.call(Terasology.java:183)
	at org.terasology.engine.Terasology.call(Terasology.java:67)
	at picocli.CommandLine.executeUserObject(CommandLine.java:1933)
	at picocli.CommandLine.access$1200(CommandLine.java:145)
	at picocli.CommandLine$RunLast.executeUserObjectOfLastSubcommandWithSameParent(CommandLine.java:2332)
	at picocli.CommandLine$RunLast.handle(CommandLine.java:2326)
	at picocli.CommandLine$RunLast.handle(CommandLine.java:2291)
	at picocli.CommandLine$AbstractParseResultHandler.execute(CommandLine.java:2159)
	at picocli.CommandLine.execute(CommandLine.java:2058)
	at org.terasology.engine.Terasology.main(Terasology.java:115)
Caused by: java.security.AccessControlException: access denied ("java.lang.RuntimePermission" "modifyThread")
	at java.base/java.security.AccessControlContext.checkPermission(AccessControlContext.java:472)
	at java.base/java.security.AccessController.checkPermission(AccessController.java:897)
	at java.base/java.lang.SecurityManager.checkPermission(SecurityManager.java:322)
	at org.terasology.gestalt.module.sandbox.ModuleSecurityManager.checkPermission(ModuleSecurityManager.java:48)
	at java.base/java.util.concurrent.ThreadPoolExecutor.checkShutdownAccess(ThreadPoolExecutor.java:748)
	at java.base/java.util.concurrent.ThreadPoolExecutor.shutdownNow(ThreadPoolExecutor.java:1405)
	at java.base/java.util.concurrent.ScheduledThreadPoolExecutor.shutdownNow(ScheduledThreadPoolExecutor.java:870)
	at java.base/java.util.concurrent.Executors$DelegatedExecutorService.shutdownNow(Executors.java:693)
	at reactor.core.scheduler.ParallelScheduler.<clinit>(ParallelScheduler.java:57)
	... 25 common frames omitted

Need to initialize schedulers eager. Or add warp this place with doPrivilegeAction
This is first call to scheduler and it fail because called from CoreRendering module

keturn · 2021-08-24T00:44:17Z

GameScheduler is marked @API. gameMain() provides access to scheduling on GameThread, which is not @API.

I brought this up in today's meeting, and it wasn't clear to any of us if/when modules need access to this.

Some discussion convinced me we do have a use case that requires this:

We want to allow modules to make OpenGL calls, so we must give them a way to run things on the thread that owns the OpenGL Context. (OpenGL works by pushing things on to a command queue, and you don't want to be mixing that up by doing it from multiple threads.)

While we don't currently have module code that makes use of GameThread for OpenGL this way, it's not much of a stretch for me to see that we might. Especially as we use Reactor to move more work off the main thread.

DarkWeird · 2021-08-24T04:10:40Z

@keturn
OpenGL thread at my way currently.

CoreRendering is using opengl from modules.
It runs at main thread currently.

keturn · 2021-08-24T16:39:49Z

Try to take screenshot:

Oh, this is my fault! When I wrote the method that takes the task name, I didn't run it through the other method with the AccessController wrapping.

I'll try the eager initialization.

DarkWeird · 2021-08-24T16:54:06Z

Try to take screenshot:

Oh, this is my fault! When I wrote the method that takes the task name, I didn't run it through the other method with the AccessController wrapping.

I'll try the eager initialization.

Eager works nice.
In graphics scheduler PR - parallel using for interval flux. For splashscreen render loop.

Privileges are only required for initialization; this way we don't need `doPrivileged` in the public method.

…age-ThreadManagerSubsystem

lgtm-com · 2021-08-27T22:20:53Z

This pull request introduces 1 alert when merging fd32176 into 66264a8 - view on LGTM.com

new alerts:

1 for Useless null check

lgtm-com · 2021-08-27T22:45:56Z

This pull request introduces 1 alert when merging c80f299 into 66264a8 - view on LGTM.com

new alerts:

1 for Useless null check

keturn · 2021-08-27T23:15:26Z

If you make changes to the suppression of alerts in a pull request, please note that theses changes will only be effective once that pull request is merged.

eh? that's weird, LGTM-bot.

keturn

Okay, I think the permission-modifying code is isolated enough and adequately commented now.

As for the matter of why we never had to add PermissionProperties before, I think it's a combination:

The old ThreadManager seems to have been running code outside the security context, so it's possible things were sneaking by, but mostly
There aren't that many things called from the module sandbox that require reading system properties.

I'm totally stumped on why wildcards don't work for PermissionProperties, but I am not worried enough about the maintenance cost of that list to put more importance on it for the time being.

pollend · 2021-11-05T21:03:54Z

Okay, I think the permission-modifying code is isolated enough and adequately commented now.

As for the matter of why we never had to add PermissionProperties before, I think it's a combination:
1. The old ThreadManager seems to have been running code outside the security context, so it's possible things were sneaking by, but mostly

2. There aren't that many things called from the module sandbox that require reading system properties.
I'm totally stumped on why wildcards don't work for PermissionProperties, but I am not worried enough about the maintenance cost of that list to put more importance on it for the time being.

I already tried wildcards :P

refactor: replace usage of ThreadManagerSubsystem with RxJava Scheduler

a01066a

github-actions bot added the Type: Refactoring Request for or implementation of pure and automatic refactorings, e.g. renaming, to improve clarity label Jun 27, 2021

pollend commented Jun 27, 2021

View reviewed changes

pollend marked this pull request as draft June 27, 2021 04:17

chore: expose reactivex to module space

c9b8f11

pollend mentioned this pull request Jun 27, 2021

Refactor usage of concurrency with reactor #4798

Open

11 tasks

This was referenced Jun 27, 2021

refactor: rework pathfinding logic from Terasology/Behaviors#76

Open

refactor: replace TaskMaster with Reactor Terasology/FlexiblePathfinding#24

Open

Merge branch 'develop' into refactor/remove-usage-ThreadManagerSubsystem

eb44e11

pollend marked this pull request as ready for review June 27, 2021 21:19

Merge branch 'develop' into refactor/remove-usage-ThreadManagerSubsystem

11a257a

pollend added a commit that referenced this pull request Jun 28, 2021

refactor: use single with a subject to drive saving

dc557dd

PR: #4799

pollend mentioned this pull request Jun 28, 2021

chore(persistence): replace transaction manager with Reactor #4800

Merged

1 task

chore: replace scheduleDirect with Completable

23b174e

pollend requested review from skaldarnar and DarkWeird July 2, 2021 15:27

skaldarnar reviewed Jul 3, 2021

View reviewed changes

pollend added 2 commits July 3, 2021 22:25

feature: rework with reactor core

f1956e2

Merge branch 'refactor/remove-usage-ThreadManagerSubsystem' of github…

cc44241

….com:MovingBlocks/Terasology into refactor/remove-usage-ThreadManagerSubsystem

update whitelist

7cf9a36

Merge branch 'develop' into refactor/remove-usage-ThreadManagerSubsystem

875b4d3

pollend changed the title ~~refactor: replace usage of ThreadManagerSubsystem with RxJava Scheduler~~ refactor: replace usage of ThreadManagerSubsystem with reactor Scheduler Jul 12, 2021

keturn added 3 commits August 9, 2021 20:29

Revert "chore: add micrometer thread monitor"

75c7e24

This reverts commit 27e8b0e.

chore: whitespace

fa1d7cf

feat(GameScheduler): add scheduleParallel() method compatible with ex…

7f44559

…isting ThreadMonitor

keturn mentioned this pull request Aug 10, 2021

refactor(GameScheduler): provide ThreadMonitor-compatible schedule method #4839

Merged

Merge branch 'develop' into refactor/remove-usage-ThreadManagerSubsystem

c144bdc

Merge branch 'develop' into refactor/remove-usage-ThreadManagerSubsystem

fdbc116

keturn added 4 commits August 24, 2021 10:13

chore(GameScheduler): preemptive initialization of scheduler

271b743

Privileges are only required for initialization; this way we don't need `doPrivileged` in the public method.

Merge remote-tracking branch 'origin/develop' into refactor/remove-us…

4d8a20d

…age-ThreadManagerSubsystem

Merge remote-tracking branch 'origin/develop' into refactor/remove-us…

e8b8808

…age-ThreadManagerSubsystem

chore: checkstyle 🚮

af755f6

doc: some notes on the property permissions

c80f299

keturn force-pushed the refactor/remove-usage-ThreadManagerSubsystem branch from fd32176 to c80f299 Compare August 27, 2021 22:34

keturn mentioned this pull request Aug 27, 2021

Document how and when to add classes/permissions/packages to the engine module #4647

Open

keturn approved these changes Aug 27, 2021

View reviewed changes

keturn merged commit 80d0983 into develop Aug 27, 2021

keturn deleted the refactor/remove-usage-ThreadManagerSubsystem branch August 27, 2021 23:43

keturn added this to the v5.2.0 milestone Sep 5, 2021

keturn mentioned this pull request Nov 14, 2021

feat: micrometer metrics over JMX #4947

Merged

2 tasks

jdrueckert mentioned this pull request Apr 3, 2022

Add Thread Naming Support #5005

Open

@@ @@ -18,14 +23,33 @@ @@
                * </ul>
                *
                */
+              @API

refactor: replace usage of ThreadManagerSubsystem with reactor Scheduler #4799

refactor: replace usage of ThreadManagerSubsystem with reactor Scheduler #4799

Conversation

pollend commented Jun 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Jun 27, 2021

lgtm-com bot commented Jun 27, 2021

lgtm-com bot commented Jun 27, 2021

lgtm-com bot commented Jun 27, 2021

lgtm-com bot commented Jun 29, 2021

skaldarnar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pollend Aug 13, 2021 • edited Loading

Choose a reason for hiding this comment

lgtm-com bot commented Jul 4, 2021

lgtm-com bot commented Jul 4, 2021

lgtm-com bot commented Jul 4, 2021

keturn commented Aug 14, 2021

pollend commented Aug 15, 2021

DarkWeird commented Aug 18, 2021

keturn commented Aug 24, 2021 • edited Loading

DarkWeird commented Aug 24, 2021

keturn commented Aug 24, 2021

DarkWeird commented Aug 24, 2021

lgtm-com bot commented Aug 27, 2021

lgtm-com bot commented Aug 27, 2021

keturn commented Aug 27, 2021

keturn left a comment

Choose a reason for hiding this comment

pollend commented Nov 5, 2021

pollend commented Jun 27, 2021 •

edited

Loading

pollend Aug 13, 2021 •

edited

Loading

keturn commented Aug 24, 2021 •

edited

Loading