jsakamoto / Toolbelt.Blazor.HotKeys2

This is a class library that provides configuration-centric keyboard shortcuts for your Blazor apps.
https://jsakamoto.github.io/Toolbelt.Blazor.HotKeys2/
Mozilla Public License 2.0
87 stars 6 forks source link

Getting "There is no tracked object with id". console errors. #23

Open accessguru opened 1 month ago

accessguru commented 1 month ago

Hello. A couple of years ago, my company incorporated the original HotKeys into our solution, which has worked well over the years. The implementation uses an adapter pattern to track hotkeys and context. Recently, when moving to HotKeys2, I noticed that "There is no tracked object with id " console errors. are appearing. It appears that when the HotKeysContext is disposed, it is not removing the keypress event listener. I have a repo that demonstrates this issue with our implementation.

We would greatly appreciate any insight that you could provide for this issue. Please reach out to me (lawrence.mantin@sprbrk.com) if you have any questions. Thank you.

jsakamoto commented 1 month ago

Hi @accessguru, Thank you for your contributions. I'm investigating this problem, but it is taking longer than I estimated. This problem must be due to a lack of my concerns about race conditions in asynchronous operations. Please wait for a while. Thank you for your patience.

jsakamoto commented 1 month ago

Hi @accessguru,

I published the v.5.0.0 Preview today.

I believe that the latest preview version will work correctly. But please make sure to avoid disposing of hotkey entries yourself. This means you have to delete line 116 of the "HotKeys2Test.Client/ShortcutKeys/SprbrkHotKeysRootContext.cs"

private void RecreateCurrentContext()
{
    ...
    //DON'T DO THIS:  keyEntriesToDispose.ForEach(k => k.Dispose());
    ...

Could you try it out? Thanks!

accessguru commented 1 month ago

Thanks for the quick turnaround on this. I have updated my "test" project with your most recent v5 preview package and changed everything required to support async/await. I no longer see the "no object track id" issues, but I am seeing this console error only when the page initially loads. If I have the time, I'll bring down this repro and see if I can find the issue.

invoke-js.ts:176 fail: Toolbelt.Blazor.HotKeys2.HotKeys[0] Cannot access a disposed object. Object name: 'Microsoft.JSInterop.WebAssembly.WebAssemblyJSObjectReference'. System.ObjectDisposedException: Cannot access a disposed object. Object name: 'Microsoft.JSInterop.WebAssembly.WebAssemblyJSObjectReference'. at Microsoft.JSInterop.Implementation.JSObjectReference.ThrowIfDisposed() at Microsoft.JSInterop.Implementation.JSObjectReference.InvokeAsync[IJSVoidResult](String identifier, Object[] args) at Microsoft.JSInterop.JSObjectReferenceExtensions.InvokeVoidAsync(IJSObjectReference jsObjectReference, String identifier, Object[] args) at Toolbelt.Blazor.HotKeys2.HotKeysContext.<>c__DisplayClass73_0.<b__1>d.MoveNext() --- End of stack trace from previous location --- at Toolbelt.Blazor.HotKeys2.Extensions.JS.InvokeSafeAsync(Func`1 action, ILogger logger)

accessguru commented 1 month ago

I believe this is the fix needed in HotKeysContext.cs. There was no check to see if dispose was already called.

public async ValueTask DisposeAsync()
    {
        GC.SuppressFinalize(this);
        await this._Syncer.InvokeAsync(async () =>
        {           
            if (this._IsDisposed) return false; // <<< Dispose Check

            var context = await this._JSContextTask;
            this._IsDisposed = true;
            foreach (var entry in this._Keys)
            {
                await this.UnregisterAsync(entry);
                entry._NotifyStateChanged = null;
            }
            lock (this._Keys) this._Keys.Clear();
            await JS.InvokeSafeAsync(async () =>
            {
                await context.InvokeVoidAsync("dispose");
                await context.DisposeAsync();
            }, this._Logger);
            return true;
        });
    }
jsakamoto commented 1 month ago

Hi @accessguru, Thank you for your feedback! Your contributions must be helpful not only to me but also to every developer who utilizes this library.

By the way, I changed my mind that we should not allow the second or more invoking the Dispose method without any errors, as well as the dotNET runtime is doing, because such an invoking may be a signal of bugs.

For instance, the HotKeys2Test project has code that exchanges the "HotKeyCpontext" instance in the "_currentContext" field variable (The "RecreateCurrentContextAsync" method of the "SprbrkHotKeysRootContext" class), but that code may be causing unexpected behavior because the asynchronous process is not atomic.

private async Task RecreateCurrentContextAsync()
{
    // Before the disposing task below is completed (during the asyncronouse process), 
    // the second invoking was happened!
    // This is the reason for the DisposeAsync was invoked twice for the same instance.
    await this._currentContext.DisposeAsync();

    // And if the process could continue witout errors even though the invoking DisposingAsync happend twice,
    // the code below line will overwrite the previouse instance,
    // and that instance's dispoing process will never invoked!
    this._currentContext = this.HotKeys.CreateContext();
    ...

That is the reason why the "DisposeAsync" method of the same "HotKeysContext" instance was invoked twice. And if I ignored the second invoking of the "DisposeAsync" as you suggested, one of the "HotKeysContext" instances would never be disposed, and obviously, it would cause another bug.

In this case, you have to protect the "RecreateCurrentContextAsync" method from unexpected race conditions, such as by using a Semaphore.

private readonly SemaphoreSlim _Syncer = new(1);

// You should protect the RecreateCurrentContextAsync method
// to be atomic, in this case.
private async Task RecreateCurrentContextAsync()
{
    await this._Syncer.WaitAsync();
    try {
        await this._currentContext.DisposeAsync();
        this._currentContext = this.HotKeys.CreateContext();
        ...
    }
    finally { this._Syncer.Release(); }
}

So, I will implement that to throw an exception when the "DisposeAsync" method in the HotKeys2 library is invoked twice or more. That is what I'm considering right now.

accessguru commented 1 month ago

That seems to work well! I will implement this pattern in our project and let you know if I run into any further issues. Will you be releasing v5 soon? Thank you.

jsakamoto commented 1 month ago

@accessguru I'm retracting my decision. According to the Microsoft Learn document, we should allow the Dispose method to be invoked multiple times.

https://learn.microsoft.com/en-us/dotnet/standard/garbage-collection/implementing-dispose

So, I'll follow the guidelines above and implement the disposal check as you suggested before.

Will you be releasing v5 soon?

I want to do so, but I might have to wait for feedback from other developers.

accessguru commented 1 month ago

Please consider these changes in your DisposeAsync() method...

    public async ValueTask DisposeAsync()
    {
        GC.SuppressFinalize(this);
        await this._Syncer.WaitAsync();
        try
        {
            if (this._Disposed) return;

            var context = await this._JSContextTask;
            this._Disposed = true;
            foreach (var entry in this._Keys)
            {
                await this.UnregisterAsync(entry);
                entry._NotifyStateChanged = null;
            }
            this._Keys.Clear();
            await JS.InvokeSafeAsync(async () =>
            {
                await context.InvokeVoidAsync("dispose");
                await context.DisposeAsync();
            }, this._Logger);
        }
        finally
        {
            this._Syncer.Release();
        }
    }
  1. Added await this._Syncer.WaitAsync() to ensure exclusive access to the _Keys collection while disposing.
  2. Replaced lock (this._Keys) this._Keys.Clear() with this._Keys.Clear() since we are already inside the exclusive lock.
  3. Added finally block to release the SemaphoreSlim in case of any exceptions during disposal.
  4. Removed the unnecessary return true statement since the method has a void return type.
jsakamoto commented 1 month ago

Hello @accessguru, Sorry for the late reply.

Please consider these changes in your DisposeAsync() method...

Yeah, actually, you don't need to worry about it because the latest implementation of Blazor HotKeys2 already matches your request. The source code doesn't match your suggestion exactly, but that's because some of the code is encapsulated into an extension method. Essentially, the meaning of the final code is the same as your suggestion.

Anyway, thank you for your suggestions and contributions!

I'll be going to release the Blazor HotKey2 v5 as an official soon.