From ef185e5e28d2a92944d4e072d70b21df58bab8e1 Mon Sep 17 00:00:00 2001 From: Lauro Moura Date: Thu, 27 Jun 2019 19:04:59 -0300 Subject: [PATCH] efl-mono: Fix value forwarding in promises/async Summary: Values returned from C# Then callbacks must release ownership of the underlying native value, so Eina code can clean it up nicely and avoid the Wrapper flushing it early. The same issue applied to the Async wrappers. In this case the value passed as the Task parameter could be released by an `using` block awaiting the value. Also Future creation was then-ing the wrong handle. Also add better exception messages. Reviewers: vitor.sousa, felipealmeida Reviewed By: vitor.sousa Subscribers: cedric, #reviewers, #committers Tags: #efl Differential Revision: https://phab.enlightenment.org/D9197 --- src/bindings/mono/eina_mono/eina_promises.cs | 12 ++++++++-- src/bindings/mono/eina_mono/eina_value.cs | 24 ++++++++++++++----- src/bindings/mono/eo_mono/iwrapper.cs | 6 +++-- src/tests/efl_mono/Promises.cs | 25 ++++++++++++++++++++ 4 files changed, 57 insertions(+), 10 deletions(-) diff --git a/src/bindings/mono/eina_mono/eina_promises.cs b/src/bindings/mono/eina_mono/eina_promises.cs index 25077a3c97..34561a5b08 100644 --- a/src/bindings/mono/eina_mono/eina_promises.cs +++ b/src/bindings/mono/eina_mono/eina_promises.cs @@ -180,6 +180,8 @@ public class Promise : IDisposable { SanityChecks(); eina_promise_resolve(this.Handle, value); + // Promise will take care of releasing this value correctly. + value.ReleaseOwnership(); this.Handle = IntPtr.Zero; // Resolving a cb does *not* call its cancellation callback, so we have to release the // lambda created in the constructor for cleanup. @@ -224,11 +226,12 @@ public class Future /// public Future(IntPtr handle) { - Handle = ThenRaw(handle, (Eina.Value value) => + handle = ThenRaw(handle, (Eina.Value value) => { Handle = IntPtr.Zero; return value; }); + Handle = handle; } /// @@ -304,7 +307,12 @@ public class Future ResolvedCb cb = handle.Target as ResolvedCb; if (cb != null) { - value = cb(value); + Eina.Value managedValue = cb(value); + // Both `value` and `managedValue` will point to the same internal value data. + // Avoid C# wrapper invalidating the underlying C Eina_Value as the eina_future.c + // code will release it. + value = managedValue.GetNative(); + managedValue.ReleaseOwnership(); } else { diff --git a/src/bindings/mono/eina_mono/eina_value.cs b/src/bindings/mono/eina_mono/eina_value.cs index 99cf09757d..a19d3d449b 100644 --- a/src/bindings/mono/eina_mono/eina_value.cs +++ b/src/bindings/mono/eina_mono/eina_value.cs @@ -46,6 +46,9 @@ static internal class UnsafeNativeMethods [DllImport(efl.Libs.Eina)] internal static extern void eina_value_free(IntPtr type); + [DllImport(efl.Libs.Eina)] + internal static extern IntPtr eina_value_type_name_get(IntPtr type); + [DllImport(efl.Libs.Eina)] [return: MarshalAsAttribute(UnmanagedType.U1)] internal static extern bool eina_value_convert(IntPtr handle, IntPtr convert); @@ -725,7 +728,16 @@ static class ValueTypeBridge LoadTypes(); } - return NativeToManaged[native]; + try + { + return NativeToManaged[native]; + } + catch (KeyNotFoundException ex) + { + var name_ptr = eina_value_type_name_get(native); + var name = Marshal.PtrToStringAnsi(name_ptr); + throw new Efl.EflException($"Unknown native eina value Type: 0x{native.ToInt64():x} with name {name}"); + } } public static IntPtr GetNative(ValueType valueType) @@ -978,8 +990,7 @@ public class Value : IDisposable, IComparable, IEquatable // free(), avoiding a call to eina_value_flush that would wipe the underlying value contents // for pointer types like string. tmp = MemoryNative.Alloc(Marshal.SizeOf(typeof(ValueNative))); - Marshal.StructureToPtr(value, tmp, false); // Can't get the address of a struct directly. - this.Handle = Alloc(); + Marshal.StructureToPtr(value, tmp, false); // Can't get the address of a struct directly. // Copy is used to deep copy the pointed payload (e.g. strings) inside this struct, so we can own this value. if (!eina_value_copy(tmp, this.Handle)) @@ -1482,7 +1493,7 @@ public class Value : IDisposable, IComparable, IEquatable { if (Disposed) { - throw new ObjectDisposedException(base.GetType().Name); + throw new ObjectDisposedException($"Value at 0x{this.Handle.ToInt64():x}"); } return this.Handle; @@ -1494,7 +1505,7 @@ public class Value : IDisposable, IComparable, IEquatable { if (Disposed) { - throw new ObjectDisposedException(base.GetType().Name); + throw new ObjectDisposedException($"Value at 0x{this.Handle.ToInt64():x}"); } // Can't call setup with Empty value type (would give an eina error) @@ -1540,7 +1551,7 @@ public class Value : IDisposable, IComparable, IEquatable { if (Disposed) { - throw new ObjectDisposedException(GetType().Name); + throw new ObjectDisposedException($"Value at 0x{this.Handle.ToInt64():x}"); } } @@ -1591,6 +1602,7 @@ public class Value : IDisposable, IComparable, IEquatable /// Get a ValueNative struct with the *value* pointed by this Eina.Value. public ValueNative GetNative() { + SanityChecks(); ValueNative value = (ValueNative)Marshal.PtrToStructure(this.Handle, typeof(ValueNative)); return value; } diff --git a/src/bindings/mono/eo_mono/iwrapper.cs b/src/bindings/mono/eo_mono/iwrapper.cs index 5fe1daae91..82c08ccf3f 100644 --- a/src/bindings/mono/eo_mono/iwrapper.cs +++ b/src/bindings/mono/eo_mono/iwrapper.cs @@ -530,8 +530,10 @@ public class Globals } else { - // Will mark the returned task below as completed. - tcs.SetResult(received); + // Async receiver methods may consume the value C# wrapper, like when awaiting in the start of an + // using block. In order to continue to forward the value correctly to the next futures + // in the chain, we give the Async wrapper a copy of the received wrapper. + tcs.SetResult(new Eina.Value(received)); } fulfilled = true; diff --git a/src/tests/efl_mono/Promises.cs b/src/tests/efl_mono/Promises.cs index b355a0ee3d..fc0c089e4e 100644 --- a/src/tests/efl_mono/Promises.cs +++ b/src/tests/efl_mono/Promises.cs @@ -42,6 +42,31 @@ class TestPromises Test.AssertEquals(received_value, reference_value); } + public static void test_simple_with_object() + { + bool callbackCalled = false; + Eina.Value receivedValue = null; + + Efl.Loop loop = Efl.App.AppMain; + Eina.Promise promise = new Eina.Promise(); + Eina.Future future = new Eina.Future(promise); + + future = future.Then((Eina.Value value) => { + callbackCalled = true; + receivedValue = new Eina.Value(value); + return value; + }); + + Eina.Value referenceValue = new Eina.Value(Eina.ValueType.Array, Eina.ValueType.Int32); + referenceValue.Append(32); + promise.Resolve(new Eina.Value(referenceValue)); + + loop.Iterate(); + + Test.Assert(callbackCalled, "Future callback should have been called."); + Test.AssertEquals(receivedValue, referenceValue); + } + public static void test_simple_reject() { bool callbackCalled = false;