[tests] Fix GestureRecognizerTest.NoStrongCycles - #5462
Conversation
- Fixes issue dotnet#1345: [FAIL] GestureRecognizerTest.NoStrongCycles : Any finalized (xamarin/maccore#1345) - Because `UIGestureRecognizer` has an `OnDispose ()` method called by `Dispose ()` it also means we're at some point running `GC.SuppressFinalize()`. Therefore, fix the test to also call the delegate when `Dispose ()` is called and not the finalizer only. As I understand, the goal of this test is to make sure that the `UIGestureRecognizer` does not hold `FinalizerNotifier`, a.k.a that its finalizer / dispose is called.
stephen-hawley
left a comment
stephen-hawley
left a comment
There was a problem hiding this comment.
I don't like all three cases being run in one test, but that's what it was before.
|
Build failure Test results1 tests failed, 0 tests skipped, 79 tests passed.Failed tests
|
|
The initial fix is fixing a strong cycle, so that the GC can collect all the objects in the cycle. The corresponding test creates such a cycle, and asserts that one of the objects in the cycle is collected. The asserted object ( The fact that it only seem to fail on 32-bit is strange, however. It means the test works fine in 64-bit... Maybe one place to start investigating is to check if the RemoveTarget code from the BeginInvokeOnMainThread block is actually called or not: https://github.com/xamarin/xamarin-macios/pull/5402/files#diff-71c27806e4190990d0e094755a9d2b43R49 |
| public void Dispose () | ||
| { | ||
| if (Action != null) | ||
| Action (); |
There was a problem hiding this comment.
This might be a race condition, not in this case AFAIK. But I find it safer to do Action?.Invoke() since from the check agains null until the execution, Action can be set to be null. Thread stops after the check, other thread changes action, first thread continues and boom! NRE.
There was a problem hiding this comment.
yup (ToCToU). .net pattern for events suggest a local
var a = Action;
if (a != null)
a ();
so the check and the call are done on the same value.
spouliot
left a comment
spouliot
left a comment
There was a problem hiding this comment.
+1 on @rolfbjarne comment
Since this test prevent us from having green builds (and we already have an open issue) this test should be disabled (separate PR) and back ported to d16-0 (while we can)
- WIP PR dotnet#5462 more investigation needed so ignore test which is preventing green builds
- WIP PR dotnet#5462 more investigation needed so ignore test which is preventing green builds
- WIP PR #5462 more investigation needed so ignore test which is preventing green builds
|
Note: when fixed, don't forget to un-ignore the test: #5486 |
- WIP PR #5462 more investigation needed so ignore test which is preventing green builds
6 participants
(https://github.com/xamarin/maccore/issues/1345)
UIGestureRecognizerhas anOnDispose ()method called byDispose ()it also means we're at some point runningGC.SuppressFinalize().Therefore, fix the test to also call the delegate when
Dispose ()is called and not the finalizer only.As I understand, the goal of this test is to make sure that the
UIGestureRecognizerdoes not holdFinalizerNotifier, a.k.a that its finalizer / dispose is called.