SteamNetworkingMessage_t crashes on Release() / SteamNetworkingMessage_t usage

[ghost]

I am fully aware that there is an issue about this problem here, and I am fully aware that it regards the development of the library rather than the usage, which is what I need. After reading it multiple times I still can't understand exactly how am I supposed to clear messages, since Release() method still crashes Unity on version 14.0.0. My code looks like this:

MessageCount = SteamNetworkingSockets.ReceiveMessagesOnPollGroup(PollGroup, PointerBuffer, 32);
for (int i = 0; i < MessageCount; ++i)
{
SteamNetworkingMessage_t NewMessage = Marshal.PtrToStructure<SteamNetworkingMessage_t>(PointerBuffer[i]);
Marshal.Copy(NewMessage.m_pData, Buffer, 0, NewMessage.m_cbSize);
FillSize = NewMessage.m_cbSize;

// Something happens here like parsing/serialization etc

NewMessage.Release();
}

[ghost]

After looking at Facepunch library while trying to understand how they handled this scenario, it seems that they are actually passing a pointer to the SteamNetworkingMessage_t itself to the release function, and not the address of the release function to the release function (what?).

Replacing

NewMessage.Release();

with

NativeMethods.SteamAPI_SteamNetworkingMessage_t_Release(PointerBuffer[i]);

Helped me to stop crashes and now everything works as intended, which seems to me that either I don't fully understand how this library wants me to manage my memory/message buffer, or there is something wrong with the implementation.

[rlabrecque]

I think what we may want to be doing here is:

MessageCount = SteamNetworkingSockets.ReceiveMessagesOnPollGroup(PollGroup, PointerBuffer, 32);
for (int i = 0; i < MessageCount; ++i)
{
    SteamNetworkingMessage_t NewMessage = Marshal.PtrToStructure<SteamNetworkingMessage_t>(PointerBuffer[i]);
    NewMessage.m_pfnFreeData = PointerBuffer[i]; // This line ?
    Marshal.Copy(NewMessage.m_pData, Buffer, 0, NewMessage.m_cbSize);
    FillSize = NewMessage.m_cbSize;

    // Something happens here like parsing/serialization etc

    NewMessage.Release();
}

That's what they do over here:
https://github.com/nxrighthere/ValveSockets-CSharp/blob/656d236e381cb9b0c20c12795f1c9907e25cc62a/ValveSockets.cs#L648

One thing we could possibly do relatively easy is have a constructor that takes an IntPtr, and handles this stuff like so:

MessageCount = SteamNetworkingSockets.ReceiveMessagesOnPollGroup(PollGroup, PointerBuffer, 32);
for (int i = 0; i < MessageCount; ++i)
{
    SteamNetworkingMessage_t NewMessage = SteamNetworkingMessage_t(PointerBuffer[i]);
    NewMessage.SetData(Buffer); // Like: Marshal.Copy(NewMessage.m_pData, Buffer, 0, NewMessage.m_cbSize);

    // Something happens here like parsing/serialization etc

    NewMessage.Release();
}

We possibly want to make SteamNetworkingMessage_t IDisposable too?

I haven't written "client" code using these myself yet, so let me know what you would like and we can make that happen. 👍

The ValveSockets-CSharp library does some clever performance stuff with pooling, and I'd like that to continue to be possible with Steamworks.NET as well.

[ghost]

Thanks for the hint with that pooling mechanism, I already implemented something simular to Facepunch's approach with pooled pointer buffer on preallocated umashalled memory, but I will think what else there could be done.

Talking about code conventions and stuff, I believe naming convention of the method should reflect that setter of the "data" needs to be mandatory, and is bound to the release mechanism of the message so that there would be no need to read documentation, so naming like it something along "SetMessagePtr" would make sense, considering that other methods are operating exactly on top of pointers.

[AG4W]

Would it be possible to expose NativeMethods.SteamAPI_SteamNetworkingMessage_t_Release to make this easier to workaround? So we don't need to manually wrangle the NativeMethods-class - it was pretty confusing for a noob like me.

On a tangent, from a usability/client-code perspective, a signature that delivers the messages instead of the IntPtr[] would be much easier to work with - imo.

[SingleAccretion]

@rlabrecque So, as you've correctly identified, the crash is the direct consequence of the fact that the wrong pointer is being passed to the native function. SteamAPI_SteamNetworkingMessage_t_Release(IntPtr self) expects the argument to be a pointer to this, &message (it is an instance method in C++). Instead, the current implementation of SteamNetworkingMessage_t.Release passes m_pfnFreeData, which is presumably a copy-paste bug as I've found at least one other library doing it. m_pfnFreeData is actually meant for the reverse scenario: you would construct a message with a custom allocator (say, GC, new and pinned GCHandle in C#) and assign this field to a custom deallocator function (something like GCHandle.Release for that managed array), then pass that message back to native which would take care of calling that deallocation function as appropriate.

The fix might be as simple as just doing something like this in Release:

public void Release()
{
    fixed (SteamNetworkingMessage_t* thisPtr = &this)
    {
        NativeMethods.SteamAPI_SteamNetworkingMessage_t_Release((IntPtr)thisPtr);
    }
}

However, it is not 100% bulletproof as the native side may be expecting the exact same pointer that it passed to you via, e. g., ReceiveMessagesOnPollGroup or similar methods (full disclosure: I am commenting here after helping @AG4W figure this out, so I am not familiar with the library or the Steam API). You also cannot exactly "carry" this pointer in SteamNetworkingMessage_t, as it must be field-for-field compatible with the native version, as your users using Marshal.PtrToStructure are already relying on this (you might be able to play tricks with appending fields to the back or assigning the pointer to m_pfnFreeData, but I would not recommend this approach due its questionable reliability and maintainability characteristics). In my opinion, your best course of action to unblock users right now would be to:

1. Expose the API releasing the pointer to the message. Something like this:

public class SteamNetworkingMessage_t
{
+    public static void ReleaseMessage(IntPtr pointerToMessage);
}

2. Obsolete SteamNetworkingMessage_t.Release and point users to the new API in the obsoletion message.
3. Hide SteamNetworkingMessage_t.Release via the use of [EditorBrowsable(EditorBrowsableState.Never)] attribute (optional, might not be worth it).

Going on a bit of a tangent here: the library seems to provide a managed wrapper over the Steam API, but it appears to be inconsistent it its design. In places, it exposes very low-level C-like primitives, as is the case here, where users have to manually deduce that they need to treat IntPtrs as SteamNetworkingMessage_t* and dereference them accordingly, while at the same time hiding the raw imports. You might consider either exposing the raw imports (preferably with blittable signatures instead of using expensive marshalling features like arrays with [Out, In] attributes) so that users who are already operating on that low level can consume them while not dealing with the complexities of packaging, platform and bitness differences, and/or exposing a higher level API that would use SafeHandles and such for messages, friendly names and signatures, etc.

[rlabrecque]

ValveSoftware/GameNetworkingSockets#169 (comment)

[fghl]

ValveSoftware/GameNetworkingSockets#169 (comment)

This only helps those using GameNetworkingSockets right? What about for those of us using the Steamworks API?

Can we settle on a way forward? I'm spending all of my time on this currently and am blocked. I'm happy to help but just need a direction. Are we adding the Release method changes @SingleAccretion mentioned as a short-term solution?