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

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

Open
accessguru opened this issue May 9, 2024 · 9 comments
Open

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

accessguru opened this issue May 9, 2024 · 9 comments

Comments

@accessguru
Copy link

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

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

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

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

accessguru commented May 13, 2024

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

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

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

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

accessguru commented May 15, 2024

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants