Partial fix for AVAudioEngine.Connect(input, sink, format) crash when format == null

[whitneyschmidt]

This fixes the crash which occurs when calling AVAudioEngine.Connect(input, sink, null) with null as the third parameter.

The crash occurred when we tried to check for equality using .Equals on null for the null check in this generated code:

global::ObjCRuntime.Messaging.void_objc_msgSend_IntPtr_IntPtr_nuint_IntPtr (this.Handle, Selector.GetHandle ("connect:toConnectionPoints:fromBus:format:"), sourceNode.Handle, nsa_destNodes.Handle, sourceBus, format == null ? IntPtr.Zero : format.Handle);

[spouliot]

Manual code should come with unit tests. That would likely have prevented the issue in the original commit.

Note: some identical already exists for other types, they just have to be copied for AVAudioFormat

[spouliot]

might be smaller to return !(a == b);

[whitneyschmidt]

🤦‍♀️ Good catch! That's much better 😄

[mandel-macaque]

Please add tests.

[monojenkins]

Build success
✅ Build succeeded
✅ API Diff (from stable)
✅ API Diff (from PR only) (no change)
✅ Generator Diff (no change)
✅ Test run succeeded

[monojenkins]

Build success
✅ Build succeeded
✅ API Diff (from stable)
✅ API Diff (from PR only) (no change)
✅ Generator Diff (no change)
✅ Test run succeeded

[whitneyschmidt]

Not sure whether these using are needed - could format be a field like nullformat?

[spouliot]

It's not very important (in tests) but it's good to dispose asap
if there's a disposal issue you'll know it sooner, likely with a reference to the test (stack trace)
otherwise it can happen at GC time and tracking those are much less fun :)

[spouliot]

It's not very important (in tests) but it's good to dispose asap
if there's a disposal issue you'll know it sooner, likely with a reference to the test (stack trace)
otherwise it can happen at GC time and tracking those are much less fun :)

[tj-devel709]

Is it a standard practice to test using a field from the class and a variable inside the method or was this specific to this case and why did you decide to do that?

[spouliot]

no, but if there's one place where standard is not very useful is the tests ;-) Having diversity, like different code (not source) patterns, can some time help us find interesting bugs

[whitneyschmidt]

This is a good question 👍

This test code was mostly copied/modified from here: https://github.com/whitneyschmidt/xamarin-macios/blob/d3f0916cdb5a43ad1a18d7271cbe19603e004a07/tests/monotouch-test/Foundation/UrlTest.cs

The using ensures that the variable inside the method is disposed as soon as possible.

From my understanding (@spouliot + @mandel-macaque please correct as needed 😆 ) if we didn't wrap the variable in using, it would be picked up for disposal by the garbage collector, which we have no control over.

I think we don't need to worry about nullFormat because there's no object created in that variable's assignment and the value is never modified so there's no potential for weirdness with memory management.

The choice to make format a variable inside of individual test methods comes from the other test, and fwict we try to stick to the policy of making sure things are disposed asap in testing code (even if it is not strictly necessary).

Maybe someone else can give an example of a bug that might occur w/o using to handle disposal?

[mandel-macaque]

@tj-devel709 @whitneyschmidt Usually, it is good to ensure that the variables in the test are disposed ASAP as in this example. There are some cases, in which the variables that we are going to be using do need certain setup before they are executed and that the setup is expensive or error prone. One example I can think of are the networking tests, in which we need to create a connection and get the available interfaces. That setup can be shared for the full TestCase since is complicated.

In https://github.com/whitneyschmidt/xamarin-macios/blob/d3f0916cdb5a43ad1a18d7271cbe19603e004a07/tests/monotouch-test/Network/NWConnectionTest.cs we do such a thing, where as you can see SetUp does a lot:

		[SetUp]
		public void SetUp ()
		{
			// connect and once the connection is done, deal with the diff tests
			connectedEvent = new AutoResetEvent(false);
			host = NetworkResources.MicrosoftUri.Host;
			// we create a connection which we are going to use to get the availabe
			// interfaces, that way we can later test protperties of the NWParameters class.
			using (var parameters = NWParameters.CreateUdp ())
			using (var endpoint = NWEndpoint.Create (host, "80"))
			{
				using (var protocolStack = parameters.ProtocolStack) {
					var ipOptions = protocolStack.InternetProtocol;
					ipOptions.IPSetVersion (NWIPVersion.Version4);
				}
				connection = new NWConnection (endpoint, parameters);
				connection.SetQueue (DispatchQueue.DefaultGlobalQueue); // important, else we will get blocked
				connection.SetStateChangeHandler (ConnectionStateHandler);
				connection.Start (); 
				Assert.True (connectedEvent.WaitOne (20000), "Connection timed out.");
			}

		}

Because we use a TestCase instance variable, the life of the variable cannot be managed with a using clause, so we use the TearDown method, to call Dsipose from the IDisposable interface:

		[TearDown]
		public void TearDown () => connection.Dispose ();

Ideally, when unit testing we want to start with a clean canvas, like what @whitneyschmidt does here.

Question: @whitneyschmidt @tj-devel709 what will happen with this:

using (var format = new AVAudioFormat ())
using (IDisposable myMemoryHungryVar = null) {
   // I test, I don't care about my hunger! 🍔 
}

[mandel-macaque]

Another nice thing to read, there is a diff between SetUp and SetUpFixture.

xUnit follows a slightly diff pattern.

[monojenkins]

Build success
✅ Build succeeded
✅ API Diff (from stable)
✅ API Diff (from PR only) (no change)
✅ Generator Diff (no change)
✅ Test run succeeded

[whitneyschmidt]

@monojenkins backport d16-8

[whitneyschmidt]

@monojenkins backport main

[spouliot]

That was not really needed, but it's not harmful (keep it).

Adding the using is the same as

AVAudioFormat nullFormat;
try {
    nullFormat = null;
    ...
} finally {
   nullFormat?.Dispose ();
}

and we know that Dispose won't be called, so just more core is generated in case an exception occurs.

Note that it's possible recent C# compiler versions (or the JIT/AOT compiler) simplify this even since it's obviously always null. Once the Dispose is optimized away it's also possible to remove the exception blocks (since finally is empty). YMMV

[monojenkins]

Build success
✅ Build succeeded
✅ API Diff (from stable)
✅ API Diff (from PR only) (no change)
✅ Generator Diff (no change)
✅ Test run succeeded