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

refactor: Remove reflection usage in Brush #16741

Closed
wants to merge 9 commits into from
Closed

Conversation

dr1rrb
Copy link
Member

@dr1rrb dr1rrb commented May 15, 2024

Refactoring (no functional changes, no api changes)

Remove reflection usage in Brush

What is the current behavior?

We use reflection to invoke event handler

What is the new behavior?

No reflection!

PR Checklist

Please check if your PR fulfills the following requirements:

@dr1rrb dr1rrb enabled auto-merge May 15, 2024 15:53
@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-16741/index.html

Copy link
Member

@Youssef1313 Youssef1313 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quite scary class to touch, but I don't see leak concerns with the new impl. Just few comments.

I'd also love us to have this well-tested.

e.g (either runtime test or unit test should be fine, though my preference is always runtime test if no platform-specifics are involved):

using System;
using System.Collections.Generic;
using Uno.UI.Helpers;

namespace Uno.UI.RuntimeTests.Tests.Uno_UI;

[TestClass]
public class Given_WeakEventManager
{
	private sealed class EventPublisher
	{
		private readonly WeakEventManager _manager = new();

		internal event Action Event
		{
			add => _manager.AddEventHandler(value);
			remove => _manager.RemoveEventHandler(value);
		}
	}

	private sealed class EventSubscriber
	{
		public void M() { }
	}

	[TestMethod]
	public void When_Many_Subscriptions_Should_Not_Leak()
	{
		var publisher = new EventPublisher();
		var weakRefs = new List<WeakReference<EventSubscriber>>();
		Subscribe(publisher, weakRefs);

		for (int i = 0; i < 10; i++)
		{
			GC.Collect();
			GC.WaitForPendingFinalizers();
		}

		foreach (var x in weakRefs)
		{
			Assert.IsFalse(x.TryGetTarget(out _));
		}
	}

	private void Subscribe(EventPublisher publisher, List<WeakReference<EventSubscriber>> weakRefs)
	{
		for (int i = 0; i < 1000; i++)
		{
			var subscriber = new EventSubscriber();
			publisher.Event += subscriber.M;
			weakRefs.Add(new WeakReference<EventSubscriber>(subscriber));
		}
	}
}

Maybe also have additional tests around a handler that tries to subscribe/unsubscribe, re-entrancy, etc

Copy link
Member

@Youssef1313 Youssef1313 May 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI there are some tests from MAUI here. I haven't looked at them, but just in case there is any interesting test scenario there. This comment isn't necessarily to port them as-is, but just looking if there is anything interesting :)

@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-16741/index.html

@dr1rrb dr1rrb force-pushed the dev/dr/brushReflection branch from bc94246 to 153a52c Compare May 22, 2024 21:56
Co-authored-by: Agnès ZITTE <16295702+agneszitte@users.noreply.github.com>
@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-16741/index.html

@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-16741/index.html

1 similar comment
@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-16741/index.html

@dr1rrb dr1rrb force-pushed the dev/dr/brushReflection branch from ca06783 to 3c192bc Compare May 30, 2024 13:39
@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-16741/index.html

@jeromelaban
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-16741/index.html

@jeromelaban
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-16741/index.html

@jeromelaban
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-16741/index.html

@Youssef1313
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@unodevops
Copy link
Contributor

🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-16741/index.html

@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-16741/index.html

@MartinZikmund
Copy link
Member

The failures here don't seem related to the changes, maybe just CI being funky

}

[TestMethod]
public void When_ManySubscriptions_Then_DoesNotLeak()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is failing on iOS @dr1rrb

@jeromelaban
Copy link
Member

Superseeded by #18899

auto-merge was automatically disabled November 22, 2024 21:19

Pull request was closed

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

Successfully merging this pull request may close these issues.

8 participants