Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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. #89

Open
Bond-Addict opened this issue Dec 2, 2024 · 18 comments
Labels
Bug Something isn't working

Comments

@Bond-Addict
Copy link

Bond-Addict commented Dec 2, 2024

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
Copy link
Author

Bond-Addict commented Dec 3, 2024

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 - SignalR/SignalR#1205 (comment)

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.

@chrissainty
Copy link
Member

@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?

@Bond-Addict
Copy link
Author

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);
        }

@chrissainty
Copy link
Member

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.

@Bond-Addict
Copy link
Author

Bond-Addict commented Dec 3, 2024

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.

@chrissainty
Copy link
Member

chrissainty commented Dec 3, 2024

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.

@Bond-Addict
Copy link
Author

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'

@chrissainty
Copy link
Member

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

@Bond-Addict
Copy link
Author

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

using JustDecompile

@chrissainty
Copy link
Member

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.

@Bond-Addict
Copy link
Author

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.

@chrissainty
Copy link
Member

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.

@Bond-Addict
Copy link
Author

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?

@chrissainty
Copy link
Member

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

@Bond-Addict
Copy link
Author

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?

@chrissainty
Copy link
Member

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 chrissainty removed the Triage Issue needs to be triaged label Dec 4, 2024
@Bond-Addict
Copy link
Author

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
Copy link
Author

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 dotnet/aspnetcore#59335 if you're interested

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants