JnCrMx / discord-game-sdk4j

Java bindings for Discord's Game SDK
MIT License
114 stars 23 forks source link

`new Core(params)` Throwing when Discord is not running (java-impl branch) #76

Open Desoroxxx opened 7 months ago

Desoroxxx commented 7 months ago

When Discord is not running new Core(params) thows a FileNotFoundException. I use this: params.setFlags(CreateParams.Flags.NO_REQUIRE_DISCORD); So I wasn't really excepting this to happen, I expected a debug log saying that it could not find Discord so it was either not present or not running.

java.lang.RuntimeException: java.io.FileNotFoundException: \\?\pipe\discord-ipc-0 (The system cannot find the file specified)
    at de.jcm.discordgamesdk.Core.<init>(Core.java:126)
    at dev.redstudio.hh.handlers.DiscordGameSdkHandler.lambda$init$0(DiscordGameSdkHandler.java:34)
    at java.base/java.lang.VirtualThread.run(VirtualThread.java:309)
Caused by: java.io.FileNotFoundException: \\?\pipe\discord-ipc-0 (The system cannot find the file specified)
    at java.base/java.io.RandomAccessFile.open0(Native Method)
    at java.base/java.io.RandomAccessFile.open(RandomAccessFile.java:356)
    at java.base/java.io.RandomAccessFile.<init>(RandomAccessFile.java:273)
    at java.base/java.io.RandomAccessFile.<init>(RandomAccessFile.java:223)
    at java.base/java.io.RandomAccessFile.<init>(RandomAccessFile.java:137)
    at de.jcm.discordgamesdk.impl.channel.WindowsDiscordChannel.<init>(WindowsDiscordChannel.java:22)
    at de.jcm.discordgamesdk.Core.getDiscordChannel(Core.java:56)
    at de.jcm.discordgamesdk.Core.<init>(Core.java:119)
    ... 2 more
JnCrMx commented 7 months ago

The NO_REQUIRE_DISCORD was originally used in the native version of this library to prevent the application from closing (uncleanly, via a native exit call) and attempting to restart it through Discord in case Discord was not running. Even in the native version, using this flag would still cause exceptions if Discord was not running (but in runCallbacks), because invoking any functions of the SDK fails without Discord being present.

While I personally think that throwing an exception is the most useful thing to do in this case, the documentation is certainly lacking, and the behavior is unexpected.

My idea to address this would be to deprecate CreateParams.Flags.NO_REQUIRE_DISCORD and instead provide a Core.isDiscordRunning method.

The alternatives would be to silently fail if the flag is set and Discord is not running, or to follow the original behavior and fail with a GameSDKException in runCallbacks(). However, none of the two alternative options seem good to me (I'd rather have the user simply catch the exceptions and deal with them as they see fit for their application).

What do you think?

Desoroxxx commented 7 months ago

What do you think?

Maybe if NO_REQUIRE_DISCORD is being used, it could just log a warn or a debug log explaining what happened and saying that it most likely means that Discord is not running.

I like the idea of a Core.isDiscordRunning method if it is static, as it would allow more things that just handling this more cleanly.

letorbi commented 5 months ago

I also stumbled upon this exception when I developed the improved java channel setup (#61), but was not sure how to handle it. Therefore I just let the exception happen, if a pipe or socket cannot be found. The most common case, where this exception happens, is when the app the is started before Discord itself has been started.

The biggest problem is, that an app, which uses discord-game-sdk4j won't try to reconnect to Discord after the channel setup failed once. So the app has to be restarted after Discord has been started in order to update the state etc.

My idea is to enhance the WindowsDiscordChannel and UnixDiscordChannel channel classes so that they try to connect to a socket/pipe before each read/write command, if no connection has been established already.

However, I am not sure whether this is a good idea or not, because it won't prevent the exception in the setup phase and might even trigger more with each command. I think an option would be to just emit a warning if NO_REQUIRE_DISCORD or something similar is set.

What do you think?

Beside that: I don't really understand how Core.isDiscordRunning could replace NO_REQUIRE_DISCORD, since the former would simply show whether Discord is running or not, while the latter indicates whether Discord is required or optional for the app. Or am I getting something wrong here?

JnCrMx commented 5 months ago

Beside that: I don't really understand how Core.isDiscordRunning could replace NO_REQUIRE_DISCORD, since the former would simply show whether Discord is running or not, while the latter indicates whether Discord is required or optional for the app. Or am I getting something wrong?

I think the question ultimately comes down to how much the library wants to handle and how much the application should handle.

Naturally, none of our features are going to work if Discord is not running. The NO_REQUIRE_DISCORD flag originally simply prevents the app from getting terminated if Discord cannot be found. Every runCallbacks would still result in an error, as it should in such a situation, and every other operation simply won't execute.

In that sense NO_REQUIRE_DISCORD and Core.isDiscordRunning would fulfill the same purpose, with the only difference being that with Core.isDiscordRunning the application itself can (and has to) decide how to handle the situation.

The biggest problem is, that an app, which uses discord-game-sdk4j won't try to reconnect to Discord after the channel setup failed once. So the app has to be restarted after Discord has been started in order to update the state etc.

My idea is to enhance the WindowsDiscordChannel and UnixDiscordChannel channel classes so that they try to connect to a socket/pipe before each read/write command, if no connection has been established already.

However, I am not sure whether this is a good idea or not, because it won't prevent the exception in the setup phase and might even trigger more with each command. I think an option would be to just emit a warning if NO_REQUIRE_DISCORD or something similar is set.

In my opinion, it would be best to introduce an AUTO_RECONNECT flag, that can be set in order to cause the library to automatically attempt to reconnect on each command. Some apps might desire it (since it makes stuff easier) while others might prefer avoiding the overhead of opening the socket each time.

I would also change the NO_REQUIRE_DISCORD to simply suppress all exceptions related to Discord not being preset. This would deviate from the behavior of the original SDK for runCallbacks, but it would be more consistent imo (compared to failing sometimes and sometimes now). However, then I would also rename it to something that better reflects its behavior (otherwise people might believe the library can work without Discord). I'd be happy to hear suggestions for a new name :smile:

The behavior I propose would be the following:

letorbi commented 5 months ago

I've tried to implement some kind of auto-reconnect functionality yesterday, but all my efforts were fruitless, because I was not able to resolve all channel related dependencies. For example some managers require an open channel and also some other components don't seem to like it when the channel is closed and then re-opened.

In the end I came to the conclusion that the best way to handle a disconnect is to destroy the old core and create a new core once Discord is available again. Since this needs a way to determine whether Discord is running or not, I've created the pull request #80, which adds a isDiscordRunning() method as suggested by JnCrMx to the core. All details can be found in the description of the pull request.

Desoroxxx commented 3 months ago

So I started using Core#isDiscordRunning which is great, now I can reconnect to Discord if it dies for whatever reason.

Only thing left would be a way to ignore exceptions that are created when Discord doesn't exist and it would be perfect!

JnCrMx commented 3 months ago

That's cool!

Then I'll look into a suppressExceptions flag or something in that direction next.