C# Example app crashes after Garbage Collecting

[gosucherry]

Hi. Im struggling with using Discord Game SDK for C# outside Unity Engine. Your example code works fine, but if Garbage Collector walk in, the app crashes! After trying to attach this example to my larger project (where GC collects every 10-30 seconds) it makes SDK unusable!

Steps to reproduce:

1. Download your sdk package (https://dl-game-sdk.discordapp.net/latest/discord_game_sdk.zip).
2. Make a new .Net Framework Console App (i've done mine in Visual Studio 2019)
3. Force garbage collection just before "lobbyManager.Search()" call

for example:

GC.Collect();
lobbyManager.Search(query, (_) =>
{
   Console.WriteLine("search returned {0} lobbies", lobbyManager.LobbyCount());
   if (lobbyManager.LobbyCount() == 1)
   {
      Console.WriteLine("first lobby secret: {0}", 
      lobbyManager.GetLobby(lobbyManager.GetLobbyId(0)).Secret);
   }
});

4. Simply run the app

It will throw "CallbackOnCollectedDelegate" on usage of "Discord.LobbyManager+FFIMethods+SearchCallback::Invoke".

In my application im not forcing manually GC.Collect() since GC runs pretty often by itself.

Implementation specifics : C# + Visual Studio 2019.

When using the very same code in Unity Engine everything works fine and no crash occurs. Im not sure if im not missing anything here, but I've spent over two days already trying to fix this issue, yet still no success...

Update: My app works fine, when run on Mono. The issue can be related to "MonoPInvokeCallback" attribute.

[gosucherry]

Fixed issue by defining and initializing Callbacks delegate as statics in the class body.

[PizzaConsole]

This is currently what the SDK is experiencing on non-Mono projects due to JIT (just in time) Compilation: https://docs.microsoft.com/en-us/dotnet/framework/debug-trace-profile/callbackoncollecteddelegate-mda

[gosucherry]

Maybe it would be reasonable to include this solution in future releases of the SDK.

Original code (Core.cs):

[MonoPInvokeCallback]
private static void ConnectLobbyCallbackImpl(IntPtr ptr, Result result, ref Lobby lobby)
{
GCHandle h = GCHandle.FromIntPtr(ptr);
    ConnectLobbyHandler callback = (ConnectLobbyHandler)h.Target;
    h.Free();
    callback(result, ref lobby);
}

public void ConnectLobby(Int64 lobbyId, string secret, ConnectLobbyHandler callback)
{
    GCHandle wrapped = GCHandle.Alloc(callback);
    Methods.ConnectLobby(MethodsPtr, lobbyId, secret, GCHandle.ToIntPtr(wrapped), ConnectLobbyCallbackImpl);
}

Working code:

[MonoPInvokeCallback]
private static void ConnectLobbyCallbackImpl(IntPtr ptr, Result result, ref Lobby lobby)
{
    GCHandle h = GCHandle.FromIntPtr(ptr);
    ConnectLobbyHandler callback = (ConnectLobbyHandler)h.Target;
    h.Free();
    callback(result, ref lobby);
}

private readonly static FFIMethods.ConnectLobbyCallback connectLobbyCallback = new FFIMethods.ConnectLobbyCallback(ConnectLobbyCallbackImpl);

public void ConnectLobby(Int64 lobbyId, string secret, ConnectLobbyHandler callback)
{
    GCHandle wrapped = GCHandle.Alloc(callback);
    Methods.ConnectLobby(MethodsPtr, lobbyId, secret, GCHandle.ToIntPtr(wrapped), connectLobbyCallback);
}

[adepierre]

Got the same issue here. Thanks for the working code, it's perfect! However it's a bit tedious to change every function used. Would be great if this fix was integrated in the SDK directly.

[PizzaConsole]

@adepierre We messaged @msciotti and they were supposed to fix the SDK with this change months ago...

[x-gaming]

Thank you so much for that solution, I had the same problem but with the UpdateActivityCallback and I spent hours of trying to find the problem, now it works ❤️

[gosucherry]

Thank you so much for that solution, I had the same problem but with the UpdateActivityCallback and I spent hours of trying to find the problem, now it works ❤️

Glad to hear :) I've spent almost 3 days figuring this out haha :D I would love to make some fork repo, with this already implemented, but it's sadly not allowed by terms of use of discord...

[PizzaConsole]

I am really disappointed that discord has not applied the change yet...

[gosucherry]

I am really disappointed that discord has not applied the change yet...

And im really dissappointed with the example code I've provided in this issue... haha :) I've mixed "Search" and "ConnectLobby" methods, which probably made the code even more illegible... But it's fixed now (my comment from 26 March was edited).

[dgibso29]

Also experiencing this issue & can confirm @gosucherry 's fix -- Thanks!

Hoping the SDK is updated presently.

[dgibso29]

102 is related.

[vkuhlmann]

I wrote a Regex to do this edit:

1. Open Core.cs in Notepad++
2. Open 'Replace' (Ctrl+H)
3. Set Search Mode to 'Regular expression' and uncheck '. matches newline'
4. In 'Find what:' enter (?<aligner>\n(?<methodIndent>\s*)Methods.*?, )(?<target>[A-Za-z0-9]*)Impl(?<term>\);\n(\k<methodIndent>[^\n]*\n)*(?<indent>\s*)})
5. In 'Replace with:' enter $+{aligner}$+{target}Delegate$+{term}\n\n$+{indent}private readonly static FFIMethods.$+{target} $+{target}Delegate = new FFIMethods.$+{target}\($+{target}Impl\);
6. Use 'Replace' or 'Replace all' to convert the whole file.
7. Add a note at the start of the file indicating you performed this fix, like

   // This file was modified with the following fix:
   // https://github.com/discord/gamesdk-and-dispatch/issues/36

8. Yeey! Back to actual coding!

PS: Thanks so much to everyone discussing the problem here, it helped me a lot!

[PizzaConsole]

Just as a cool note. When we first found this fix it was technically illegal under the ToS from discord

2.2 Restrictions. You shall not, and shall not permit any person, directly or indirectly, to (i) reverse engineer, disassemble, reconstruct, decompile, translate, modify or copy the API or SDK

That is not longer the case as they have removed the restrictions from their new ToS

[vkuhlmann]

Ouch. Good thing they removed that. One could of course argue that they meant the .dll's, with the code provided for the languages being merely bindings, not the real API or SDK. I mean, providing plain code is just asking for people to modify it. Especially if it is broken.

[PizzaConsole]

I mean, providing plain code is just asking for people to modify it.

Unfortunately that's not the way legal works... You have to abide by the license.

[vkuhlmann]

Haha, don't you worry. But to be honest, I missed the license you mentioned. Why didn't they include it in the SDK zip?

[PizzaConsole]

@vkuhlmann That's because you automatically accept it here when you make your account. https://discord.com/developers/docs/legal

[vkuhlmann]

Yes, I figured that. I wasn't thinking when I accepted the license I would do anything out of the ordinary. So, to just get started quickly with my project, I only briefly scanned through it... And I would read it better once I was to actually publish anything.

[PizzaConsole]

I ALWAYS read every ToS nowadays, just my nature I guess. But you can find some interesting stuff when you do!

[vkuhlmann]

Haha. I always feel like it slows you down so much, better to assure everything works in broad strokes, and then look into every detail. But I agree, you are doing things the proper way.

[Vercidium]

Bumping this issue as it is still occuring in the latest Discord SDK.

I am using a similar solution to what was posted above:

[MonoPInvokeCallback]
private static void UpdateActivityCallbackImpl(IntPtr ptr, Result result)
{
    GCHandle h = GCHandle.FromIntPtr(ptr);
    UpdateActivityHandler callback = (UpdateActivityHandler)h.Target;
    h.Free();
    callback(result);
}

// This line ensures that UpdateActivityCallbackImpl is not collected by GC
static FFIMethods.UpdateActivityCallback staticCallback = UpdateActivityCallbackImpl;

public void UpdateActivity(Activity activity, UpdateActivityHandler callback)
{
    GCHandle wrapped = GCHandle.Alloc(callback);

    // This line now uses staticCallback instead of UpdateActivityCallbackImpl 
    Methods.UpdateActivity(MethodsPtr, ref activity, GCHandle.ToIntPtr(wrapped), staticCallback);
}

[PizzaConsole]

I gave up on them merging this fix :/

[samspillers]

Issue still occuring on newest version. Used the following to fix UpdateActivity in ActivityManager.

        private FFIMethods.UpdateActivityCallback MyUpdateActivityCallback = UpdateActivityCallbackImpl;

        public void UpdateActivity(Activity activity, UpdateActivityHandler callback)
        {
            GCHandle wrapped = GCHandle.Alloc(callback);
            Methods.UpdateActivity(MethodsPtr, ref activity, GCHandle.ToIntPtr(wrapped), MyUpdateActivityCallback);
        }