Virtual threads by GodFuper · Pull Request #65 · FAForever/java-ice-adapter

GodFuper · 2024-11-30T11:14:22Z

Virtualthreads don't like "synchronized". Therefore, I changed it to Lock.
Now all Threads are executed in Executor. And this Executor runs on virtual threads.

Switching to virtual threads requires 21 java.

GodFuper · 2024-11-30T11:35:41Z

                Thread.sleep(20);
            } catch (InterruptedException e) {
                log.error(getLogPrefix() + "Interrupted while waiting for ICE", e);
+                onConnectionLost();


I added it on my own. I think if interrupted, then it is possible to terminate the connection.

GodFuper · 2024-11-30T11:39:08Z

+            return socket;
        } catch (SocketException e) {
            log.error("Could not create socket for peer: {}", getPeerIdentifier(), e);
+            throw e;


If the socket has returned an error, then it doesn't make sense further. Because we are going to get a LocalPort, and the socket will be null.

Brutus5000 · 2024-11-30T20:55:04Z


    public void close() {
        running = false;
-        refreshThread.interrupt();


Why remove? If one of the methods in the while loop are blocking, the thread will keep being blocked.

You are right, it will be blocked until the game ends. I returned it.

It makes me wonder though, if then the running flag even makes sense 🤔

# Conflicts: # ice-adapter/src/main/java/com/faforever/iceadapter/IceAdapter.java # ice-adapter/src/main/java/com/faforever/iceadapter/debug/DebugWindow.java # ice-adapter/src/main/java/com/faforever/iceadapter/debug/DebugWindowController.java # ice-adapter/src/main/java/com/faforever/iceadapter/gpgnet/GPGNetServer.java # ice-adapter/src/main/java/com/faforever/iceadapter/ice/Peer.java # ice-adapter/src/main/java/com/faforever/iceadapter/ice/PeerIceModule.java # ice-adapter/src/main/java/com/faforever/iceadapter/rpc/RPCHandler.java

Brutus5000

I am just wondering if we don't shoot ourselves in the foot here.

From my understanding VirtualThreads were created to simplify the handling of asynchronous code. So using CompletableFuture.async methods handled in a one virtual thread per task-executor is the backwards compatible way to do it, but is there any benefit for new code to do so?

Brutus5000 · 2024-12-01T20:38:59Z

+        CompletableFuture.runAsync(
+                () -> {
+                    Thread.currentThread().setName("sendingLoop");
+                    sendingLoop();
+                },
+                executor);


Feels dangerous. If you don't pass a OneNewVirtualThreadPerTask executor, you might rename and block a shared pool thread.

Since the thread is a loop and not a one time task, why don't just spawn a virtual thread here and leave out the completable future?

Brutus5000 · 2024-12-01T20:40:52Z

-
-            listenerThread = new Thread(this::listenerThread);
-            listenerThread.start();
+            listener = CompletableFuture.runAsync(this::listenerThread, IceAdapter.getExecutor());


Same here. Why not just spawn a virtual thread directly?

You think that if I do this: "Thread.ofVirtual().start(this::listenerThread)". Will something change?

We are setting this up at the executor level.

You have a reference to the thread so you can set the name, exceptionHandler and interrupt it.

Brutus5000 · 2024-12-01T20:44:20Z

+            checker = CompletableFuture.runAsync(
+                    () -> {
+                        Thread.currentThread().setName(getThreadName());
+                        checkerThread();
+                    },
+                    IceAdapter.getExecutor());
+        });


This is a continuous task too, so just spawning a virtual thread seems easer.

executor = Executors.newVirtualThreadPerTaskExecutor();

This means that all tasks where there is an executor will be on virtual threads.

The executor is not defined in this class. So you couple this class to the implementation of another class.
But apart from that e don't make any use of the completablefuture. Since we don't run any follow-up operations on the future, just Thread.ofVirtual().name(...).uncaugtExceptionHandler(...) is easier to understand.

Brutus5000 · 2024-12-01T20:45:35Z


-        listenerThread = new Thread(this::listener);
-        listenerThread.start();
+        listener = CompletableFuture.runAsync(this::listener, IceAdapter.getExecutor());


I would launch a virtual thread here too.

Sheikah45 · 2024-12-01T22:12:57Z

I am just wondering if we don't shoot ourselves in the foot here.

From my understanding VirtualThreads were created to simplify the handling of asynchronous code. So using CompletableFuture.async methods handled in a one virtual thread per task-executor is the backwards compatible way to do it, but is there any benefit for new code to do so?

I think using an executor is still probably the right choice as that way in the future if we find out that in some places virtual threads are not the right choice or additional execution considerations need to be added we can easily do that by swapping out the executor.

Although I do see your point on using the CompletableFuture API for infinite loop tasks and one shot / fire and forget tasks. In those cases I agree it might be best to just use the executor directly with executor.submit etc.

…e application.

GodFuper · 2024-12-03T03:53:09Z

I am just wondering if we don't shoot ourselves in the foot here.
From my understanding VirtualThreads were created to simplify the handling of asynchronous code. So using CompletableFuture.async methods handled in a one virtual thread per task-executor is the backwards compatible way to do it, but is there any benefit for new code to do so?

I think using an executor is still probably the right choice as that way in the future if we find out that in some places virtual threads are not the right choice or additional execution considerations need to be added we can easily do that by swapping out the executor.

Although I do see your point on using the CompletableFuture API for infinite loop tasks and one shot / fire and forget tasks. In those cases I agree it might be best to just use the executor directly with executor.submit etc.

And what is the difference between executor.submit and CompletableFuture.RunAsync(... , executor) ? We get the "Future" there and there.

Sheikah45 · 2024-12-03T07:56:40Z

The main difference is the simplified API. Since only a normal future is returned it doesn't allow you to perform the async callback operations making it clear that the submitted task should be fairly standalone.

Brutus5000 · 2024-12-03T10:29:58Z

The main difference is the simplified API. Since only a normal future is returned it doesn't allow you to perform the async callback operations making it clear that the submitted task should be fairly standalone.

That would not be my interpretation of reading such a signature.
And looking at the overall code quality of this project we are discussing on the wrong level. It just doesn't matter right now which API we use. Everything is better than what we currently have.

GodFuper · 2024-12-03T20:37:21Z

To the places where was "the thread = new Thread(...)" I returned the previous logic. In places where Thread was not controlled from close(), it will now be controlled from Executor.

I suggest 2 options for controlling Threads:

The "parent" class must controlhim Threads and close threads when object.close()
CompletableFuture is controlled using executor and closed using executor.shutdown().

Brutus5000 · 2024-12-03T22:12:09Z

+@UtilityClass
+public class ExecutorHolder {
+    public ExecutorService getExecutor() {
+        return Executors.newVirtualThreadPerTaskExecutor();


This creates a new executor every time. I guess we should keep one instance and return that all the time.

private final ExecutorService executor = ExecutorHolder.getExecutor();
We create it once in the main class, and then distribute it to all other classes via IceAdapter.getExecutor()

Ok, I guess that's not what Sheikah had in mind, but it's fine for now.

Brutus5000 · 2024-12-04T09:28:25Z

+            } catch (InterruptedException e) {
+                log.info("Sending loop interrupted");


Catching the interrupt without re-interrupting will clear out the .isInterrupted() state and block the while loop from ending

https://chatgpt.com/share/67502077-b55c-800e-99d3-f6ede4960126

You would be right if the code had while (true).
But we have the use of while (!Thread.currentThread().isInterrupted()). After that, the 'while' ends and the thread stop doing work.

If you catch the InterruptedException, !Thread.isInterrupted() returns always true.

Yes, you're right, I forgot that this mechanism works like that. I'll fix it in the code now.

GodFuper added 4 commits November 30, 2024 17:05

Upgrading the Java version to version 21

fd3807c

Adding a service for asynchronous tasks

59c7ffd

Moved the implementation of multithreading to AsyncService

a726909

Removed Executor.java

6228d27

GodFuper commented Nov 30, 2024

View reviewed changes

GodFuper added 2 commits November 30, 2024 18:10

A little refactoring

9e8cbc7

spotlessApply

55e81b4

Brutus5000 self-requested a review November 30, 2024 20:27

Sheikah45 reviewed Nov 30, 2024

View reviewed changes

Shutting down the application in one function.

35fd05b

Brutus5000 reviewed Nov 30, 2024

View reviewed changes

GodFuper added 7 commits December 1, 2024 03:14

Transferring Lock functionality to LockUtil

8254713

AsyncService optimization

ce67d04

Return interrupted thread for PeerTurnRefreshModule

c657d04

Return interrupted thread for GPGNetClient

8b976dc

Executor with dependency injection

ac35e89

spotlessApply

79af9d6

Brutus5000 reviewed Dec 1, 2024

View reviewed changes

Comment thread ice-adapter/src/main/java/com/faforever/iceadapter/IceAdapter.java

Brutus5000 reviewed Dec 1, 2024

View reviewed changes

Let all processes in executor terminate correctly, otherwise close th…

495e45b

…e application.

Returned Thread to classes with infinite loops.

0b7801a

Brutus5000 reviewed Dec 3, 2024

View reviewed changes

Error handling

e33eb8c

Fix close Ice Adapter

0e09e79

Brutus5000 reviewed Dec 4, 2024

View reviewed changes

Fixing shutdown after InterruptedException

a9fae2d

Brutus5000 approved these changes Dec 5, 2024

View reviewed changes

Brutus5000 merged commit 51ea8f7 into FAForever:master Dec 5, 2024

juerghegglin mentioned this pull request May 6, 2026

Validate echo length; use virtual thread for ICE listener #85

Open

		} catch (InterruptedException e) {
		log.info("Sending loop interrupted");

Uh oh!

Conversation

GodFuper commented Nov 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Brutus5000 Nov 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Brutus5000 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sheikah45 commented Dec 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GodFuper commented Dec 3, 2024

Uh oh!

Sheikah45 commented Dec 3, 2024

Uh oh!

Brutus5000 commented Dec 3, 2024

Uh oh!

GodFuper commented Dec 3, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Brutus5000 Dec 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GodFuper Dec 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

GodFuper commented Nov 30, 2024 •

edited

Loading

Brutus5000 Nov 30, 2024 •

edited

Loading

Sheikah45 commented Dec 1, 2024 •

edited

Loading

Brutus5000 Dec 4, 2024 •

edited

Loading

GodFuper Dec 4, 2024 •

edited

Loading