Skip to content

Commit

Permalink
Fix register/unregister issues of WeakReferenceMessenger (#4082)
Browse files Browse the repository at this point in the history
<!-- 🚨 Please Do Not skip any instructions and information mentioned below as they are all required and essential to evaluate and test the PR. By fulfilling all the required information you will be able to reduce the volume of questions and most likely help merge the PR faster 🚨 -->

<!-- 👉 It is imperative to resolve ONE ISSUE PER PR and avoid making multiple changes unless the changes interrelate with each other --> 

<!-- 📝 Please always keep the "☑️ Allow edits by maintainers" button checked in the Pull Request Template as it increases collaboration with the Toolkit maintainers by permitting commits to your PR branch (only) created from your fork. This can let us quickly make fixes for minor typos or forgotten StyleCop issues during review without needing to wait on you doing extra work. Let us help you help us! 🎉 -->


## Fixes #4081
<!-- Add the relevant issue number after the "#" mentioned above (for ex: "## Fixes #1234") which will automatically close the issue once the PR is merged. -->

<!-- Add a brief overview here of the feature/bug & fix. -->

## PR Type
What kind of change does this PR introduce?
<!-- Please uncomment one or more options below that apply to this PR. -->

Bugfix
<!-- - Feature -->
<!-- - Code style update (formatting) -->
<!-- - Refactoring (no functional changes, no api changes) -->
<!-- - Build or CI related changes -->
<!-- - Documentation content changes -->
<!-- - Sample app changes -->
<!-- - Other... Please describe: -->


## What is the current behavior?
<!-- Please describe the current behavior that you are modifying, or link to a relevant issue. -->
The attached message handlers of a WeakReferenceMessenger do not get called anymore correctly for all registered objects after unregistering one of them.

## What is the new behavior?
<!-- Describe how was this issue resolved or changed? -->
The attached message handlers get called for all registered objects.

There was an issue with the enumerator of the ConditionalWeakTable. The linked list nodes were not iterated correctly when a key gets removed while enumerating. In this case the "Next"-property of the node was set to null and the while loop stopped immediately (without checking the remaining nodes). 

As a fix the next node is now kept as a locale variable so that it does not get lost when removing the key.

## PR Checklist

Please check if your PR fulfills the following requirements:

- [x] Tested code with current [supported SDKs](../readme.md#supported)
- [ ] Pull Request has been submitted to the documentation repository [instructions](..\contributing.md#docs). Link: <!-- docs PR link -->
- [ ] Sample in sample app has been added / updated (for bug fixes / features)
    - [ ] Icon has been created (if new sample) following the [Thumbnail Style Guide and templates](https:/windows-toolkit/WindowsCommunityToolkit-design-assets)
- [ ] New major technical changes in the toolkit have or will be added to the [Wiki](https:/windows-toolkit/WindowsCommunityToolkit/wiki) e.g. build changes, source generators, testing infrastructure, sample creation changes, etc...
- [x] Tests for the changes have been added (for bug fixes / features) (if applicable)
- [ ] Header has been added to all new source files (run *build/UpdateHeaders.bat*)
- [x] Contains **NO** breaking changes

<!-- If this PR contains a breaking change, please describe the impact and migration path for existing applications below.
     Please note that breaking changes are likely to be rejected within minor release cycles or held until major versions. -->


## Other information
  • Loading branch information
msftbot[bot] authored Jul 5, 2021
2 parents 28444e9 + 805438c commit 1973504
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 2 deletions.
4 changes: 3 additions & 1 deletion Microsoft.Toolkit.Mvvm/Messaging/WeakReferenceMessenger.cs
Original file line number Diff line number Diff line change
Expand Up @@ -406,6 +406,8 @@ public bool MoveNext()

while (node is not null)
{
LinkedListNode<WeakReference<TKey>>? nextNode = node.Next;

// Get the key and value for the current node
if (node.Value.TryGetTarget(out TKey? target) &&
this.owner.table.TryGetValue(target!, out TValue? value))
Expand All @@ -421,7 +423,7 @@ public bool MoveNext()
this.owner.keys.Remove(node);
}

node = node.Next;
node = nextNode;
}

return false;
Expand Down
28 changes: 27 additions & 1 deletion UnitTests/UnitTests.Shared/Mvvm/Test_Messenger.cs
Original file line number Diff line number Diff line change
Expand Up @@ -511,6 +511,32 @@ void Test()
messenger.Cleanup();
}

// See https:/windows-toolkit/WindowsCommunityToolkit/issues/4081
[TestCategory("Mvvm")]
[TestMethod]
[DataRow(typeof(StrongReferenceMessenger))]
[DataRow(typeof(WeakReferenceMessenger))]
public void Test_Messenger_RegisterMultiple_UnregisterSingle(Type type)
{
var messenger = (IMessenger)Activator.CreateInstance(type);

var recipient1 = new object();
var recipient2 = new object();

int handler1CalledCount = 0;
int handler2CalledCount = 0;

messenger.Register<object, MessageA>(recipient1, (r, m) => { handler1CalledCount++; });
messenger.Register<object, MessageA>(recipient2, (r, m) => { handler2CalledCount++; });

messenger.UnregisterAll(recipient2);

messenger.Send(new MessageA());

Assert.AreEqual(1, handler1CalledCount);
Assert.AreEqual(0, handler2CalledCount);
}

public sealed class RecipientWithNoMessages
{
public int Number { get; set; }
Expand Down Expand Up @@ -550,4 +576,4 @@ public sealed class MessageB
{
}
}
}
}

0 comments on commit 1973504

Please sign in to comment.