SessionStorage icon indicating copy to clipboard operation
SessionStorage copied to clipboard

Consistent issues with Getting or Setting items due to unmanaged SignalR exceptions. Re-opening issue #47 as it is the cause and was never fixed.

Open Bond-Addict opened this issue 1 year ago • 18 comments

Describe the bug #47 was never fixed but was closed because a workaround was discovered. Although increasing the Message size like

services.AddSignalR(hubOptions =>
{
hubOptions.MaximumReceiveMessageSize = 10 * 1024 * 1024; // 10MB
});

might work, this isn't a proper solution.

In our application we store our users entire form data in Session. This should not be an issue as the form data doesn't exceed the limit. What happens though is that when trying to set an item (which should add or replace) or get an item (which I'm a bit puzzled with) it instead throws a System.IO.InvalidDataException from SignalR and if Common Language Runtime Exceptions are enabled, it will throw the exception from HubConnectionHandler.cs at line 254 with throw new InvalidDataException($"The maximum message size of {maxMessageSize}B was exceeded. The message size can be configured in AddHubOptions.");

I've done extensive research and have come to the conclusion of 2 possibilities.

  1. For SetItemAsync, the old item is having the new item appended to it.
  2. The SignalR connection is not being disposed of correctly causing a memory leak (Once I have the tools to debug this, I can report back).

Based on both SetItemAsync and GetItemAsync throwing the same exception, I'm leaning towards something more like option 2.

In either case simply leaving the solution to be increase the message size is treating a symptom and not the cause. I could easily increase my form data in the future, and still be under whatever my threshold is (unless I define no limit or a ridiculously high limit) and run into the same issue.

To Reproduce Create a item that's close to the 32k default SignalR message limit. Save the item. Get the Item.

Or

Save the item, then re-save the item.

Expected behavior In both instances, if the original object is less than the 32k and the new object is less than 32k, there should be no SignalR exceptions thrown and the functions should complete their task successfully.

Hosting Model (is this issue happening with a certain hosting model?):

  • Blazor Server

Bond-Addict avatar Dec 02 '24 21:12 Bond-Addict

Please reference https://github.com/Bond-Addict/BlazoredIssues

Under the logs folder you can see two different logs. One that is 10kb which succeeds when GetItemAsync is called after the item is set and other that is 25kb instead. To test, first let it load with var sessionItem = new byte[10 * 1024]; which will create a 10kb payload. Next refresh the browser using either the normal browser refresh button or clicking the refresh text. When the payload is 10kb, it loads correctly every time.

Next, change the payload to var sessionItem = new byte[25 * 1024]; which will create a 25kb payload which stays below the 32kb default threshold. Perform the same test as before. Once you refresh it will immediately throw System.IO.InvalidDataException: The maximum message size of 32768B was exceeded. The message size can be configured in AddHubOptions..

If the payload was originally 32kb or more then this exception would totally make sense, but this clearly shows that smaller payloads can and will cause the exception. Based on my research it is also ill-advised by the SignalR team to increase the message size unless absolutely necessary, which in most cases it is not.

From the SignalR crew - https://github.com/SignalR/SignalR/issues/1205#issuecomment-11664318

This is a clear demonstration that #47 should have been fixed back in 2021 as it is still plaguing users in 2024.

The provided sample project is also intentionally bare bones and doesn't include any custom code implementations.

One thing to make not of, I believe this is an issue that only affects Blazor server apps as I also created https://github.com/Bond-Addict/BlazoredServerIssueClean using dotnet new blazorserver which creates a new .Net 9 Blazor Server app and it immediately shows the same behavior.

I also created a new Blazor Web Assembly app using dotnet new blazor and was unable to reproduce the issue. Which is why I think this is only a Blazor Server issue.

Bond-Addict avatar Dec 03 '24 04:12 Bond-Addict

@Bond-Addict During your testing, did you look at the message size produced from your payload, or have you just focused on the payload size?

chrissainty avatar Dec 03 '24 08:12 chrissainty

I was unable to find a way to look at the exact message size, so yes I did focus on the payload size. I've not seen a way to see the message size. While running the debugger, the only thing I have access to is the connection but I havent seen a way to access the message size from in there. So far what I've tried is,

  1. Enable trace level logging in the app.
  2. Use the Performance Profiler
  3. Using the Debug Diagnostic tools, take a memory snapshot when the app starts, after the first save happens, and then after the read happens on the refresh.
  4. Tried using fiddler
  5. Tried using the Microsoft Network Monitor
  6. Tried using Wireshark
  7. Tried to get SignalR info to show up in the event viewer.

If there is another method that can be used, please let me know.

One important factor / behavior to mention, this issue has been popping up in several apps I now maintain. Application A was where I first saw issues. The issue that first appeared, the workaround thats been implemented and application flow is as follows:

  1. App A loads the form using data from the db
  2. User makes changes to the form with a minimum of selecting a form type (Just an enum value)
  3. They click submit
  4. App A save their current form data selections and generates a report.

What was happening: When the user would try and generate another report without changing anything else on the form, which would call SetItemAsync the app would crash because the message size was then too big, but the item being saved was the exact same size as the original saved item. This crash I've been able to "fix" by removing the item every time before saving the item again. To me, based on the documentation and or the name of "SetItemAsync" I wouldn't make sense for that to be necessary, as it should be doing some sort of replacement process behind the scenes, but without this extra step it will produce a hang and then crash. This is the extension method I've created and put in place to allow my users to generate the report (and thus save their form selections in Session) as many times as they please without having to stop and relaunch the app. This still doesn't fix the crash when they go back to the search page and select a different report.

        public static async Task SetOrUpdateItemAsyn<T>(this T sessionService, string key, dynamic data) where T : ISessionStorageService
        {
            await sessionService.RemoveItemAsync(key);
            await sessionService.SetItemAsync(key, data);
        }

Bond-Addict avatar Dec 03 '24 12:12 Bond-Addict

The reason I ask about message size vs payload is that there is an overhead to encoding a payload for sending over SignalR. So even though the payload is below the limit, that doesn't mean that the message containing that payload is also below the limit.

The exception thrown by SignalR is pretty reliable and when you see that the official line from MS is to increase the message size as per the original issue you found.

This library doesn't do anything cleaver when saving an item to session storage. It literally passes it directly through to the underlying JavaScript APIs.

It seems like your payloads are just tipping the message size limit once encoded and hence you're seeing the exception.

chrissainty avatar Dec 03 '24 14:12 chrissainty

The part that doesn't add up (at least on the SetItemAsync call) is that if item a is origninally storing a object that is 20kb, and then I try to call SetItemAsync on item a again without changing even a single property on the object being saved to the item again, if I do not first remove item a, it will throw the exact same error. But as soon as I remove the item before setting the item, I can call that function as many times as I please. Based on my research it sounds like its possibly due to the SignalR connection not being closed and then a follow up call being made to that connection. If the object attempting to save to item a was more than 32k when encoded, it should have been to large the first time, but its not.

I'm still trying to figure out the size of the message to verify.

Bond-Addict avatar Dec 03 '24 15:12 Bond-Addict

Based on my research it sounds like its possibly due to the SignalR connection not being closed and then a follow up call being made to that connection.

The SignalR connection isn't closed, that's the point of SignalR, it's a constant connection between the client and server.

It's worth noting that SignalR is opaque to this library, it has no knowledge of whether it's operating over SignalR or not. You may have found a framework bug of some kind, in which case I'd definitely open an issue on the ASP.NET Core repo, I know the team would be keen to look into it.

However, if you can find something that's wrong in this library I'd be happy to take a look.

chrissainty avatar Dec 03 '24 16:12 chrissainty

Could you perhaps help me understand where the SignalR communication comes into play? When I decompile the dll, I'm unable to see any communication back or forth to SignalR and the method calls which seem to end with a JSInterop call to 'session.getItem' or 'session.setItem'

Bond-Addict avatar Dec 03 '24 17:12 Bond-Addict

When you say decompile the DLL, which DLL are you referring to?

chrissainty avatar Dec 03 '24 17:12 chrissainty

.nuget\packages\blazored.sessionstorage\2.4.0\lib\net7.0\Blazored.SessionStorage.dll

using JustDecompile

Bond-Addict avatar Dec 03 '24 17:12 Bond-Addict

But you can view the source code in this repo? I'm not sure what you're looking for by decompiling.

Anyway, as I said before, SignalR is opaque to this library, it's dealt with by Blazor. So depending on how Blazor is running, either server-side or client-side, the framework will direct the call over SignalR or directly to browser APIs respectively.

chrissainty avatar Dec 03 '24 17:12 chrissainty

I guess since I didnt see anything that refers to SignalR here in gitlab, that maybe there was something else inside the nuget package. I could totally just be missing it, but I've been unable to find / figure out was the different get, set or remove methods actually do. Maybe if I knew that, I'd have a good point to start debugging further.

Bond-Addict avatar Dec 03 '24 17:12 Bond-Addict

You can go and look at the code in this repo and see exactly what all the methods do. But honestly I think you're missing how Blazor works fundamentally with SignalR.

This library has literally no knowledge of, or control over, SignalR. It's handled 100% by the framework. If this is any kind of SignalR issue you will need to talk to the .NET team.

chrissainty avatar Dec 03 '24 17:12 chrissainty

So is there something that I'm missing then? Or is this simply all that happening

 public async ValueTask SetItemAsync(string key, string data, CancellationToken cancellationToken = default)
    {
        try
        {
            await _jSRuntime.InvokeVoidAsync("sessionStorage.setItem", cancellationToken, key, data);
        }
        catch (Exception exception)
        {
            if (IsStorageDisabledException(exception))
            {
                throw new BrowserStorageDisabledException(StorageNotAvailableMessage, exception);
            }

            throw;
        }
    }

and

_jSRuntime.InvokeVoidAsync("sessionStorage.setItem", cancellationToken, key, data);

is where this library ends and the blackbox starts?

Bond-Addict avatar Dec 03 '24 18:12 Bond-Addict

That's all this library is doing. It's literally a wrapper around the browser storage APIs. Everything else is the Blazor framework.

chrissainty avatar Dec 03 '24 18:12 chrissainty

Okay, thanks for the clarification/explanation. I will get with the .net crew and see if we can get to the bottom of this. If possible, can you convert this to a discussion instead so others can find it, but I can also provide an update once I know more?

Bond-Addict avatar Dec 04 '24 18:12 Bond-Addict

Sure thing. Good luck with your investigation. If it does turn out there is something that needs changing on this library then let me know and we can get it sorted 👍

chrissainty avatar Dec 04 '24 19:12 chrissainty

Sure thing. Good luck with your investigation. If it does turn out there is something that needs changing on this library then let me know and we can get it sorted 👍

While I wait for responses, I'm going to try and eliminate this library and make jsinterop calls myself and see the behavior. I'm also going to try and figure out a way to inspect the actual message size that is saved and then attempted to be accessed. Being a Microsoft peep, is there anyway you're aware of to accomplish this?

Bond-Addict avatar Dec 04 '24 20:12 Bond-Addict

Sure thing. Good luck with your investigation. If it does turn out there is something that needs changing on this library then let me know and we can get it sorted 👍

I went ahead and opened up https://github.com/dotnet/aspnetcore/issues/59335 if you're interested

Bond-Addict avatar Dec 05 '24 15:12 Bond-Addict