Skip to content

Commit 1973504

Browse files
author
msftbot[bot]
authored
Fix register/unregister issues of WeakReferenceMessenger (#4082)
<!-- 🚨 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://github.com/windows-toolkit/WindowsCommunityToolkit-design-assets) - [ ] New major technical changes in the toolkit have or will be added to the [Wiki](https://github.com/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
2 parents 28444e9 + 805438c commit 1973504

File tree

2 files changed

+30
-2
lines changed

2 files changed

+30
-2
lines changed

Microsoft.Toolkit.Mvvm/Messaging/WeakReferenceMessenger.cs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -406,6 +406,8 @@ public bool MoveNext()
406406

407407
while (node is not null)
408408
{
409+
LinkedListNode<WeakReference<TKey>>? nextNode = node.Next;
410+
409411
// Get the key and value for the current node
410412
if (node.Value.TryGetTarget(out TKey? target) &&
411413
this.owner.table.TryGetValue(target!, out TValue? value))
@@ -421,7 +423,7 @@ public bool MoveNext()
421423
this.owner.keys.Remove(node);
422424
}
423425

424-
node = node.Next;
426+
node = nextNode;
425427
}
426428

427429
return false;

UnitTests/UnitTests.Shared/Mvvm/Test_Messenger.cs

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -511,6 +511,32 @@ void Test()
511511
messenger.Cleanup();
512512
}
513513

514+
// See https://github.com/windows-toolkit/WindowsCommunityToolkit/issues/4081
515+
[TestCategory("Mvvm")]
516+
[TestMethod]
517+
[DataRow(typeof(StrongReferenceMessenger))]
518+
[DataRow(typeof(WeakReferenceMessenger))]
519+
public void Test_Messenger_RegisterMultiple_UnregisterSingle(Type type)
520+
{
521+
var messenger = (IMessenger)Activator.CreateInstance(type);
522+
523+
var recipient1 = new object();
524+
var recipient2 = new object();
525+
526+
int handler1CalledCount = 0;
527+
int handler2CalledCount = 0;
528+
529+
messenger.Register<object, MessageA>(recipient1, (r, m) => { handler1CalledCount++; });
530+
messenger.Register<object, MessageA>(recipient2, (r, m) => { handler2CalledCount++; });
531+
532+
messenger.UnregisterAll(recipient2);
533+
534+
messenger.Send(new MessageA());
535+
536+
Assert.AreEqual(1, handler1CalledCount);
537+
Assert.AreEqual(0, handler2CalledCount);
538+
}
539+
514540
public sealed class RecipientWithNoMessages
515541
{
516542
public int Number { get; set; }
@@ -550,4 +576,4 @@ public sealed class MessageB
550576
{
551577
}
552578
}
553-
}
579+
}

0 commit comments

Comments
 (0)