GC.KeepAlive doesn't keep local delegate alive in mono (Xamarin.Android)

[trampster]

EDITED by @stephentoub on 1/20/2023: The issue at this point tracks an analyzer that would flag use of GC.KeepAlive(local) usage in the context of C# iterators, async methods, and anywhere else the C# compiler does a significant method rewrite such that a "local" isn't actually local, e.g. where that local is lifted to a state machine and the use of GC.KeepAlive has little to no impact, e.g.

object o = new object();
yield return o;
GC.KeepAlive(o);

---

Description

According to Eric Lippert there is no need to pin a local delegate or keep a GCHandle when passing a delegate via PInvoke as long as you use GC.KeepAlive to keep the delegate alive for as long as the native code could call it. https://stackoverflow.com/a/5465380/78561

However while this seems to be true for the dotnet runtime in mono this does not work and we are getting crashes when the callback happens.

We have found that in mono we need to keep a Normal GCHandle to the delegate GC.KeepAlive does not seem to be enough.

This behaviour needs to be consistent across the mono/dotnet runtimes.

This issue was found when using Xamarin.Android

Reproduction Steps

var taskCompletionSource = new TaskCompletionSource();

MyDelegateType myDelegate = () => taskCompletionSource.SetResult(true);

NativeMethods.MyNativeMethod(myDelegate);

await taskCompletionSource.Task;

GC.KeepAlive(myDelegate);

Expected behavior

GC.KeepAlive should prevent the delegate from being collected until the delegate callback has happened.

Actual behavior

Crash, usually with NullReferenceException

Regression?

No response

Known Workarounds

Get a normal GCHandle to the delegate instead of using GC.KeepAlive

var taskCompletionSource = new TaskCompletionSource();

MyDelegateType myDelegate = () => taskCompletionSource.SetResult(true);

var gcHandle = GCHandle.Alloc(myDelegate, GCHandleType.Normal);

NativeMethods.MyNativeMethod(myDelegate);

await taskCompletionSource.Task;

gcHandle.Free();

Configuration

No response

Other information

No response

[ghost]

Tagging subscribers to this area: @dotnet/area-system-threading-tasks See info in area-owners.md if you want to be subscribed.

Issue Details

---

### Description According to Eric Lippert there is no need to pin a local delegate or keep a GCHandle when passing a delegate via PInvoke as long as you use GC.KeepAlive to keep the delegate alive for as long as the native code could call it. https://stackoverflow.com/a/5465380/78561 However while this seems to be true for the dotnet runtime in mono this does not work and we are getting crashes when the callback happens. We have found that in mono we need to keep a Normal GCHandle to the delegate GC.KeepAlive does not seem to be enough. This behaviour need to be consistent across the mono/dotnet runtimes. This issue was found when using Xamarin.Android ### Reproduction Steps ``` var taskCompletionSource = new TaskCompletionSource(); var myDelegate = () => taskCompletionSource.SetResult(true); var taskCompletionSource = new TaskCompletionSource(); var myDelegate = () => taskCompletionSource.SetResult(true); NativeMethods.MyNativeMethod(myDelegate); await taskCompletionSource.Task; NativeMethods.MyNativeMethod(myDelegate); await taskCompletionSource.Task; GC.KeepAlive(myDelegate); ``` ### Expected behavior GC.KeepAlive should prevent the delegate from being collected until the delegate callback has happened. ### Actual behavior Crash, usually with NullReferenceException ### Regression? _No response_ ### Known Workarounds Get a normal GCHandle to the delegate instead of using GC.KeepAlive ``` var taskCompletionSource = new TaskCompletionSource(); MyDelegateType myDelegate = () => taskCompletionSource.SetResult(true); var gcHandle = GCHandle.Alloc(myDelegate, GCHandleType.Normal); var taskCompletionSource = new TaskCompletionSource(); var myDelegate = () => taskCompletionSource.SetResult(true); NativeMethods.MyNativeMethod(myDelegate); await taskCompletionSource.Task; NativeMethods.MyNativeMethod(myDelegate); await taskCompletionSource.Task; gcHandle.Free(); ``` ### Configuration _No response_ ### Other information _No response_

Author:	trampster
Assignees:	-
Labels:	`area-System.Threading.Tasks`, `untriaged`
Milestone:	-

[ghost]

Tagging subscribers to 'arch-android': @steveisok, @akoeplinger See info in area-owners.md if you want to be subscribed.

Issue Details

---

### Description According to Eric Lippert there is no need to pin a local delegate or keep a GCHandle when passing a delegate via PInvoke as long as you use GC.KeepAlive to keep the delegate alive for as long as the native code could call it. https://stackoverflow.com/a/5465380/78561 However while this seems to be true for the dotnet runtime in mono this does not work and we are getting crashes when the callback happens. We have found that in mono we need to keep a Normal GCHandle to the delegate GC.KeepAlive does not seem to be enough. This behaviour needs to be consistent across the mono/dotnet runtimes. This issue was found when using Xamarin.Android ### Reproduction Steps ``` var taskCompletionSource = new TaskCompletionSource(); var myDelegate = () => taskCompletionSource.SetResult(true); var taskCompletionSource = new TaskCompletionSource(); var myDelegate = () => taskCompletionSource.SetResult(true); NativeMethods.MyNativeMethod(myDelegate); await taskCompletionSource.Task; NativeMethods.MyNativeMethod(myDelegate); await taskCompletionSource.Task; GC.KeepAlive(myDelegate); ``` ### Expected behavior GC.KeepAlive should prevent the delegate from being collected until the delegate callback has happened. ### Actual behavior Crash, usually with NullReferenceException ### Regression? _No response_ ### Known Workarounds Get a normal GCHandle to the delegate instead of using GC.KeepAlive ``` var taskCompletionSource = new TaskCompletionSource(); MyDelegateType myDelegate = () => taskCompletionSource.SetResult(true); var gcHandle = GCHandle.Alloc(myDelegate, GCHandleType.Normal); var taskCompletionSource = new TaskCompletionSource(); var myDelegate = () => taskCompletionSource.SetResult(true); NativeMethods.MyNativeMethod(myDelegate); await taskCompletionSource.Task; NativeMethods.MyNativeMethod(myDelegate); await taskCompletionSource.Task; gcHandle.Free(); ``` ### Configuration _No response_ ### Other information _No response_

Author:	trampster
Assignees:	-
Labels:	`area-System.Threading.Tasks`, `os-android`, `untriaged`, `runtime-mono`
Milestone:	-

[steveisok]

/cc @SamMonoRT @BrzVlad

[BrzVlad]

@trampster I'm skeptical that this is a real issue. What makes you so sure that coreclr has different behavior ? In the post that you linked there is no mention of async methods, which completely change the apparent storage of locals so I expect GC.KeepAlive doesn't work because of this.

@trampster Also what happens if you put the keep alive before the await operation. I suspect the await is breaking the method so the keep alive probably no longer has any effect.

[stephentoub]

which completely change the apparent storage of locals so I expect GC.KeepAlive doesn't work because of this.

Right. The C# compiler rewrites iterators, async methods, etc., such that locals whose use spans a yield/await boundary are lifted to be fields, such that they're not actually locals. GC.KeepAlive will be a nop on such usage. It did not create a GC handle, just keeps an actual local referenced and alive until that point.

[trampster]

Thanks for your reply.

We run our integrations tests with dotnet 6 and GC.KeepAlive fixed a crash we where having there. The same code when run on Xamarin.Android crashes without a normal GCHandle. That particular example may not have had async in it, I can check when I'm back at work on monday.

If the locals are lifted to be fields then the field should keep the value alive (instance variables do no need GC.KeepAlive or GCHandles, again this is from Eric Lippert's post and therefore no Handle or keep alive should be required, However we know that with a normal GCHandle the code does not crash and without it it does.

Do guys know why a GCHandle would be required to keep the delegate alive with async in this situation on mono?

[stephentoub]

Async state machines are kept alive by the thing being awaited, e.g. the thing that will complete the awaited task. But in your case, that's a native call, which upon returning won't keep anything managed rooted, so as with most interop, you need to create a GC handle to keep that call's state alive, and that will in turn keep the state machine alive.

[trampster]

I just read the docs for GC.KeepAlive https://docs.microsoft.com/en-us/dotnet/api/system.gc.keepalive?view=net-6.0

And I quote "The purpose of the KeepAlive method is to ensure the existence of a reference to an object that is at risk of being prematurely reclaimed by the garbage collector."

There is nothing on that page that says it doesn't work with async code.

While I understand your explanation about why it didn't work in this case, I still consider it a bug. Yes your explanation explains why the internal implementation is broken, but this in no way makes it not be a bug.

The fact that the reference is promoted to a field and then turned back into a local is an internal implementation detail. I told GC.KeepAlive to keep my thing alive and the async await implementation broke that contract such that it wasn't kept alive.

Either update the documenation for GC.KeepAlive to say that it doesn't work past an await. Or fix the implementation so that it can keep the reference alive through the state changes required to resume after an await.

[stephentoub]

From the runtime's perspective, it's doing exactly what the docs say it does: it extends the lifetime from the start of the current routine. It's just that the C# compiler is rewriting the method, so what looks like a single method and single method call isn't.

The docs are open source and welcoming of contributions. They could certainly be expanded upon.

[trampster]

Maybe the c# compiler needs to create a GCHandle for the reference in this case? So the behaviour one would expect as a c# programmer is preserved.

[stephentoub]

I don't think that's particularly feasible. In addition to the performance implications, the generated code would need to ensure the handle was always freed at and prior to that point, even if control flow exited at arbitrary points (returns, throws, etc), which would yield non-trivial complication to the generated code. And more importantly, it would lead to leaks if control flow never returned to the method, e.g. imagine an iterator where the consuming loop called MoveNext and then didn't finish iterating and just ignored the enumerator.

I think it's fine to improve the docs. I think it'd be fine to consider an analyzer that flagged GC.KeepAlive usage in iterators, in async methods, in local functions, and anywhere else there's material rewriting happening as part of lowering where locals are lifted to fields. But it's not a bug and it is behaving as designed.

[trampster]

In that case, I think improving the docs and adding an Analyzer would be good additions, it would definitely have saved me a lot of time and also would have resulted in me not needing to bug you about it and also waste your time.

[ghost]

Tagging subscribers to this area: @dotnet/gc See info in area-owners.md if you want to be subscribed.

Issue Details

---

### Description According to Eric Lippert there is no need to pin a local delegate or keep a GCHandle when passing a delegate via PInvoke as long as you use GC.KeepAlive to keep the delegate alive for as long as the native code could call it. https://stackoverflow.com/a/5465380/78561 However while this seems to be true for the dotnet runtime in mono this does not work and we are getting crashes when the callback happens. We have found that in mono we need to keep a Normal GCHandle to the delegate GC.KeepAlive does not seem to be enough. This behaviour needs to be consistent across the mono/dotnet runtimes. This issue was found when using Xamarin.Android ### Reproduction Steps ``` var taskCompletionSource = new TaskCompletionSource(); MyDelegateType myDelegate = () => taskCompletionSource.SetResult(true); NativeMethods.MyNativeMethod(myDelegate); await taskCompletionSource.Task; GC.KeepAlive(myDelegate); ``` ### Expected behavior GC.KeepAlive should prevent the delegate from being collected until the delegate callback has happened. ### Actual behavior Crash, usually with NullReferenceException ### Regression? _No response_ ### Known Workarounds Get a normal GCHandle to the delegate instead of using GC.KeepAlive ``` var taskCompletionSource = new TaskCompletionSource(); MyDelegateType myDelegate = () => taskCompletionSource.SetResult(true); var gcHandle = GCHandle.Alloc(myDelegate, GCHandleType.Normal); NativeMethods.MyNativeMethod(myDelegate); await taskCompletionSource.Task; gcHandle.Free(); ``` ### Configuration _No response_ ### Other information _No response_

Author:	trampster
Assignees:	BrzVlad
Labels:	`area-GC-coreclr`, `code-analyzer`
Milestone:	8.0.0

[jkotas]

We run our integrations tests with dotnet 6 and GC.KeepAlive fixed a crash we where having there. The same code when run on Xamarin.Android crashes without a normal GCHandle

That sounds like a bug in Mono.

locals whose use spans a yield/await boundary are lifted to be fields, such that they're not actually locals. GC.KeepAlive will be a nop on such usage.

I do not think that GC.KeepAlive is guaranteed to be nop in this case. The last reference of the state machine can occur before the GC.KeepAlive call, and so the state machine may not be keeping the reference alive.

[stephentoub]

The last reference of the state machine can occur before the GC.KeepAlive call, and so the state machine may not be keeping the reference alive.

Sure. My point was it does nothing to keep anything alive across a yield/await that returns out of the method that contains it. It might successfully keep a reference alive until that await, but if that await yields, at that point the KeepAlive is meaningless.

[jkotas]

I am not following why it is it meaningless.

In this example, GC.KeepAlive should keep the myDelegate alive until after the await completes and execution of the async method continues.

MyDelegateType myDelegate = () => taskCompletionSource.SetResult(true);

NativeMethods.MyNativeMethod(myDelegate);

await taskCompletionSource.Task;

GC.KeepAlive(myDelegate);

If GC.KeepAlive is deleted in this example, myDelegate can be collected right after returning from NativeMethods.MyNativeMethod method before the await yields.

[jkotas]

Ok, I see. The problem in this example is that this is calling into native code that should have used GCHandle instead.

If the method being called was a managed method, the GC.KeepAlive can be meaningful.

[jkotas]

I think the actual problem are marshalled delegates. Marshalled delegates are a bug farm. Should we rather build an analyzer that tells people to not use marshalled delegates and use function pointers + GCHandles instead?

[stephentoub]

The problem in this example is that this is calling into native code that should have used GCHandle instead.

Right.

If the method being called was a managed method, the GC.KeepAlive can be meaningful.

Yes, although even then if the goal is to keep the delegate alive during that call, it should be closer to the call and not spanning the await. Having it after the await suggests it's somehow going to keep myDelegate alive until that point, which isn't necessarily the case, even without native interop. In an extreme case, consider:

var o = new MyFinalizableObject();
await new TaskCompletionSource().Task;
GC.KeepAlive(o);

If someone thinks this KeepAlive is going to somehow keep that MyFinalizableObject from being collected, they'll be sorely mistaken: the object will be allocated, the method will await a task that isn't rooted by anyone, and the state machine will eventually become available for collection, along with the MyFinalizableObject instance.

Should we rather build an analyzer that tells people to not use marshalled delegates and use function pointers + GCHandles instead?

That sounds like a valuable thing to do in addition.

[jkotas]

Agree. On the other hand, this is an example where KeepAlive should works as expected:

var taskCompletionSource = new TaskCompletionSource();

MyDelegateType myDelegate = () => taskCompletionSource.SetResult(true);

var gcHandle = GCHandle.Alloc(myDelegate, GCHandleType.Normal);

NativeMethods.MyNativeMethod(myDelegate, finalizableResource.Handle);

await taskCompletionSource.Task;

GC.KeepAlive(finalizableResource); // make sure that finalizableResource is not finalized while the async operation is in progress.

gcHandle.Free();

[stephentoub]

Agree. On the other hand, this is an example where KeepAlive should works as expected:

Yup, because it forces finalizeResource to be lifted to the state machine. The state machine gets hooked up as a continuation onto taskCompletionSource.Task such that GCHandle to the delegate references the taskCompletionSource which references the Task which references the state machine which references finalizableResource, keeping it rooted.

[bartonjs]

It's unclear from the top post what the analyzer is intended to do. Please update the top post before the next review session. (Not marking as needs-work right now in case it breaks our new sorting algorithm)

[terrajobst]

Video

• Looks good as proposed
  • There is some objection to using GCHandle as the recommended mitigation over rooted the delegate in a static, but that feels orthogonal to the analyzer.
  • We think there is a high chance this code pattern indicates as bug, justifying it be a warning
  • When a reverse P/Invoke is called, is the delegate rooted while its executed? If it doesn't, is a problem?

Category: Reliability Severity: Warning

[ghost]

Tagging subscribers to this area: @dotnet/area-system-threading-tasks See info in area-owners.md if you want to be subscribed.

Issue Details

---

EDITED by @stephentoub on 1/20/2023: The issue at this point tracks an analyzer that would flag use of GC.KeepAlive(local) usage in the context of C# iterators, async methods, and anywhere else the C# compiler does a significant method rewrite such that a "local" isn't actually local, e.g. where that local is lifted to a state machine and the use of GC.KeepAlive has little to no impact, e.g. ```C# object o = new object(); yield return o; GC.KeepAlive(o); ``` --- ### Description According to Eric Lippert there is no need to pin a local delegate or keep a GCHandle when passing a delegate via PInvoke as long as you use GC.KeepAlive to keep the delegate alive for as long as the native code could call it. https://stackoverflow.com/a/5465380/78561 However while this seems to be true for the dotnet runtime in mono this does not work and we are getting crashes when the callback happens. We have found that in mono we need to keep a Normal GCHandle to the delegate GC.KeepAlive does not seem to be enough. This behaviour needs to be consistent across the mono/dotnet runtimes. This issue was found when using Xamarin.Android ### Reproduction Steps ``` var taskCompletionSource = new TaskCompletionSource(); MyDelegateType myDelegate = () => taskCompletionSource.SetResult(true); NativeMethods.MyNativeMethod(myDelegate); await taskCompletionSource.Task; GC.KeepAlive(myDelegate); ``` ### Expected behavior GC.KeepAlive should prevent the delegate from being collected until the delegate callback has happened. ### Actual behavior Crash, usually with NullReferenceException ### Regression? _No response_ ### Known Workarounds Get a normal GCHandle to the delegate instead of using GC.KeepAlive ``` var taskCompletionSource = new TaskCompletionSource(); MyDelegateType myDelegate = () => taskCompletionSource.SetResult(true); var gcHandle = GCHandle.Alloc(myDelegate, GCHandleType.Normal); NativeMethods.MyNativeMethod(myDelegate); await taskCompletionSource.Task; gcHandle.Free(); ``` ### Configuration _No response_ ### Other information _No response_

Author:	trampster
Assignees:	BrzVlad
Labels:	`api-approved`, `area-System.Threading.Tasks`, `code-analyzer`
Milestone:	8.0.0