API review: During shutdown, revisit finalization and provide a way to clean up resources

[kouvel]

Running finalizers on reachable objects during shutdown is currently unreliable. This is a proposal to fix that and provide a way to clean up resources on shutdown in a reliable way.

Issues observed on shutdown

Currently, a best-effort attempt is made to run finalizers for all finalizable objects during shutdown, including reachable objects. Running finalizers for reachable objects is not reliable, as the objects are in an undefined state.

• In order to finalize reachable objects, threads must be blocked, since objects that are still reachable cannot be used during or after finalization. Later, threads are terminated without running any more user code.
• Running user code in finalizers after blocking other threads is unreliable, as those threads may be blocked at an inopportune point, and may cause finalizers to block indefinitely or result in undefined behavior due to the undefined state of the object
  • Example from a user code perspective. Consider an object that writes to a network stream using some stateful communication protocol. The finalizer would write a termination value to the stream and close the stream. Suppose that the termination value indicates to the receiving end that all data has been written, while abruptly closing the stream without writing the termination value would indicate incomplete transmission due to disconnection or some other reason. Writing the termination value in the finalizer assumes that there are no more references to the object, indicating that all data has been written. Suppose that a background thread is using the object, writing data to the pipe. During shutdown, the background thread is blocked at some arbitrary point, and the object is still referenced. Writing the termination value to the pipe in the finalizer at that point may be invalid according to the protocol.
  • Example from a runtime perspective. If a thread is blocked during GC, and a finalizer tries to allocate something, it may block waiting for GC to complete, which will never happen. The finalizer itself may not even allocate anything, but even just jitting the finalizer method will trigger allocation. While this particular issue can be fixed separately, it demonstrates the unreliability of the current design not just from a user code perspective but from a runtime perspective.
• Effectively, the best-effort attempt to run finalizers for reachable finalizable objects is not reliable.

Proposal

• Don't run finalizers on shutdown (for reachable or unreachable objects)
• Don't block threads on shutdown
• Don't do a GC on shutdown (no change from current behavior)
  • Under this proposal, it is not guaranteed that all finalizable objects will be finalized before shutdown.
  • Doing a GC on shutdown and running finalizers for unreachable objects can guarantee that objects that are deterministically unreachable by the time of shutdown will be finalized. However, such objects should also be deterministically disposed before shutdown. For cases that require this, the user can trigger a GC explicitly and wait for finalizers before shutdown.
  • When there are background threads that are still running, there would be no guarantee on how many objects will be finalized anyway
• Provide a public AssemblyLoadContext.Unloading event
  • An AssemblyLoadContext manages the lifetime of assemblies loaded under that context
  • Code should register for the event in the AssemblyLoadContext instance associated with the assembly
  • The event is raised when the GC determines that the AssemblyLoadContext instance is no longer referenced. For the default AssemblyLoadContext instance and for a custom instance installed as the default load context, the event will be raised before normal shutdown.
    • Abrupt shutdown due to unhandled exception, process kill, etc., will not raise any further Unloading events
    • As unloading an AssemblyLoadContext is not yet implemented, instances that have been used to load assemblies will currently live until the end of the process
  • No timeout. The timeout on waiting for finalizers to complete on shutdown was removed in CoreCLR some time back. In favor of treating blocking issues as program errors, no timeout will be used for this event either.
  • Event handler exceptions will crash the process. Any exception propagating out of an event handler will be treated as an unhandled exception.
  • Since other threads are not blocked before this, and may continue to run for a short period after the event is raised, event handlers may need to handle concurrency, and safeguard from using cleaned up resources from other threads

Behavioral change

public static void Main()
{
    var obj = new MyFinalizable();
}

private class MyFinalizable
{
    ~MyFinalizable()
    {
        Console.WriteLine("~MyFinalizable");
    }
}

Previous output:
~MyFinalizable

Typical output with the proposal above (running the finalizer is not guaranteed, but may run if a GC is triggered):
(empty)

Proposed API

namespace System.Runtime.Loader
{
    public abstract class AssemblyLoadContext
    {
        public event Action<AssemblyLoadContext> Unloading;
    }
}

Example

public class Logger
{
    private static readonly object s_lock = new object();
    private static bool s_isClosed = false;

    static Logger()
    {
        var currentAssembly = typeof(Loader).GetTypeInfo().Assembly;
        AssemblyLoadContext.GetLoadContext(currentAssembly).Unloading += OnAssemblyLoadContextUnloading;

        // Create log file based on configuration
    }

    private static void OnAssemblyLoadContextUnloading(AssemblyLoadContext sender)
    {
        // This may be called concurrently with WriteLine
        Close();
    }

    private static void Close()
    {
        lock (s_lock)
        {
            if (s_isClosed)
                return;
            s_isClosed = true;

            // Write remaining in-memory log messages to log file and close log file
        }
    }

    public static void WriteLine(string message)
    {
        lock (s_lock)
        {
            if (s_isClosed)
                return;

            // Save log message in memory, if buffer is full, write messages to log file
        }
    }
}

[kouvel]

CC @jkotas @stephentoub @terrajobst

[jkotas]

The proposed AssemblyLoadContext.ProcessExit API is essentially exposing AppDomain.ProcessExit from full .NET Framework under different name.

It may be worth noting that the fundamental problem with finalization during shutdown came from inspiration by Java designs in the early days of .NET. Java have realized and fixed the fundamental problem many years ago by deprecating (disabling by default) finalization during shutdown. It is time to fix it for .NET Core as well.

Uncommenting the call to GC.Collect() would produce the previous output:

This is actually not guaranteed. The JIT is allowed to extend the lifetime of the local variable till the end of the method, and so the call to GC.Collect may still see MyFinalizable object as live.

[kouvel]

This is actually not guaranteed. The JIT is allowed to extend the lifetime of the local variable till the end of the method, and so the call to GC.Collect may still see MyFinalizable object as live.

Oh right, I had meant to null it out, but I guess there might still be a reference somewhere. I'll just remove that part.

[clrjunkie]

For instance, if a thread is blocked during GC, and a finalizer tries to allocate something, it may block waiting for GC to complete, which will never happen. The finalizer itself may not even allocate anything, but even just jitting the finalizer method will trigger allocation.

"if a thread is blocked during GC", are you referring to the finalizer thread? Is that because the GC does not reclaim any memory during shutdown and if the finalizer thread attempts to make an allocation that would trigger a GC it would block waiting to be notified that the GC completed its work - a notification that would never occur since at this point GC requests would simply be ignored?

Provide a public ProcessExit event that is raised during shutdown.

What is the execution order of this event with respect to finalize methods execution?

The event would be raised on the finalizer thread

Will the ProcessExit event retain the same timeout semantics as “regular” finalization
2 sec timeout per finalize method (kill process on first timeout) 40 sec total timeout

Looking at AssemblyLoadContext, it appears that it's sole responsibility is well loading assemblies. Why not than simply call it AssemblyLoader (“Context” in .NET is usually used to model temporary state, especially during Async operations)

Why not put this event in System.Environment along HashShutdownStarted like in the full .NET?

[kouvel]

"if a thread is blocked during GC", are you referring to the finalizer thread?

Finalizers are run in a separate finalizer thread. Here I'm referring to other threads running managed code. Other threads are blocked, then finalizers run on the finalizer thread.

Is that because the GC does not reclaim any memory during shutdown and if the finalizer thread attempts to make an allocation that would trigger a GC it would block waiting to be notified that the GC completed its work - a notification that would never occur since at this point GC requests would simply be ignored?

When there are background threads running managed code, and those threads are blocked, one of them may be blocked while doing a GC. When finalizers run on the finalizer thread, an allocation may get blocked until GC is complete but the thread doing the GC was blocked so it would never complete.

What is the execution order of this event with respect to finalize methods execution?

The event is raised before the final round of running finalizers.

Will the ProcessExit event retain the same timeout semantics as “regular” finalization
2 sec timeout per finalize method (kill process on first timeout) 40 sec total timeout

The timeout for waiting for finalizers had been disabled some time back in CoreCLR. If a finalizer blocks indefinitely, it should not be due to a CLR issue, so a timeout should not be necessary. Similarly, I'm proposing that a timeout is not necessary for the ProcessExit event. If it blocks indefinitely, then it should be due to a program error.

Looking at AssemblyLoadContext, it appears that it's sole responsibility is well loading assemblies. Why not than simply call it AssemblyLoader (“Context” in .NET is usually used to model temporary state, especially during Async operations)

There can be multiple contexts under which assemblies are loaded, each with custom load behavior. Loaded assemblies are associated with a context so that they can be unloaded (based on ref count) in batch for a particular context. I suppose "context" here may refer to the behavior with which assemblies are loaded.

Why not put this event in System.Environment along HashShutdownStarted like in the full .NET?

Environment may also be a good place to put it. AssemblyLoadContext was chosen because the ProcessExit event is the equivalent counterpart of AppDomain.ProcessExit in the desktop CLR, and AssemblyLoadContext takes some of the responsibilities of AppDomain in CoreCLR. Thoughts?

[clrjunkie]

@kouvel

Finalizers are run in a separate finalizer thread. Here I'm referring to other threads running managed code.

I know this. I'm just trying to understand your example, specifically in the context of shutdown.

When there are background threads running managed code, and those threads are blocked, one of them may be blocked while doing a GC. When finalizers run on the finalizer thread, an allocation may get blocked until GC is complete but the thread doing the GC was blocked so it would never complete.

Please help me understand the exact chain of events:

Let's assume we have three "regular" user code threads T1, T2, T3 and the Finalizer thread FT.

So:

1. T1 executes some long running user code.
2. T3 calls either Environment.Exit(0) or Process.Kill followed by WaitForExit();
3. T2 executes user code that calls newobj which in turn calls GC code.
4. GC Code executed by T2 suspends execution of T1 and possibly also T3 (if it is still executing a little bit after calling either shutdown methods).
5. T1 and optionally T3 are now blocked at some arbitrary point, each in their respective code paths.
6. GC Code executed by T2 completes and releases/starts FT which will 1) Append object references listed in the Finalization List on to the F-reachable Queue 2) Dequeue each object from F-reachable Queue and call it's Finalize method.
7. GC Code executed by T2 resumes execution of T1 and T3, unwinds and continues executing it’s user code. (T1,T2,T3,FT are all running)

Questions:

a) What happens next? I assume the CLR will simply wait for T1, T2, T3 and FT to complete and exit the process.

b) What blocking issues can arise here? I mean even if FT executes a Finalize method that triggers a GC, I would expect that the FT Thread will continue and execute the GC code which will ultimately augment the F-reachable Queue, unwind and continue executing the current Finalize method.

Point being I don't expect FT and T2 to be dependent one on another.

c) How all this play out when GC Code is invoked asynchronously (e.g concurrent GC)

The event is raised before the final round of running finalizers.

What do you mean by "final round”? This should be documented in the spec.

The timeout for waiting for finalizers had been disabled some time back in CoreCLR.

This should be documented as well.

Similarly, I'm proposing that a timeout is not necessary for the ProcessExit event. If it blocks indefinitely, then it should be due to a program error.

I agree, and in a way it's better because on the full .NET there is no alert if the finalizer thread gets killed, which makes debugging very hard.

Nothing is written to event log when this happens:

class Program
{
    static WorkItem WorkItem = new WorkItem();
    static void Main(string[] args)
    {
        WorkItem.DoWork();
    }
}

class WorkItem
{
    public void DoWork() 
    {
        Console.WriteLine("Done");            
    }

    ~WorkItem()
    {            
        Console.WriteLine("Finalizing");
        Thread.Sleep(5000);
        Console.WriteLine("Finalized");
    }
}

Therefore, I suggest writing an error to the event log or stderr when such a situation occurs that includes some context on the blocking finalize method.

Writing robust graceful shutdown code is a hard problem. Debugging it in production is even harder.

Loaded assemblies are associated with a context so that they can be unloaded (based on ref count) in batch for a particular context.

Is this something new? As I recall once an Assembly is loaded into an AppDomain it can't be unloaded, you can only discard the AppDomain.. Any written references?

I suppose "context" here may refer to the behavior with which assemblies are loaded.

Then "context" is more of a loading parameter per "type of load" in any case I don't see how appending the word "Context" to the class helps to clarify this.

AppDomain.ProcessExit in the desktop CLR, and AssemblyLoadContext takes some of the responsibilities of AppDomain in CoreCLR. Thoughts?

I think "AppDomain" and "Assembly" are two different concepts that should be encapsulated separately.

[kouvel]

@clrjunkie,

6. GC Code executed by T2 completes and releases/starts FT which will 1) Append object references listed in the Finalization List on to the F-reachable Queue 2) Dequeue each object from F-reachable Queue and call it's Finalize method.

a) What happens next? I assume the CLR will simply wait for T1, T2, T3 and FT to complete and exit the process.

b) What blocking issues can arise here? I mean even if FT executes a Finalize method that triggers a GC, I would expect that the FT Thread will continue and execute the GC code which will ultimately augment the F-reachable Queue, unwind and continue executing the current Finalize method.

After FT enters its shutdown sequence, it tries to block T1, T2, and T3 before running finalizers. Suppose T2 has just started a GC (see gc.cpp, GarbageCollectGeneration, GC is now flagged as started), then as part of trying to disable preemptive GC, it detects the shutdown case and blocks for shutdown (see https://github.com/dotnet/coreclr/blob/a3a2d323653e08b7592ed70070f13833b2994828/src/vm/threadsuspend.cpp#L3423).

A finalizer that does allocation and needs to allocate more space will go into gc.cpp, try_allocate_more_space, where it will block forever waiting for GC to complete. The thread running shutdown code would wait forever for the finalizer thread to signal completion, leading to a hang during shutdown.

While this and other similar occurrences could technically be considered bugs that could be fixed separately, the issue is more about blocking T1, T2, and T3 before running finalizers. Finalizers can't be reliably expected to complete successfully when some threads are arbitrarily blocked. Finalizers may rely on running in a state where the object is not referenced anymore, which is not the case here. So the state of the object may not match expectations and anything could happen as a result, especially when unmanaged resources are involved. Although, it probably wouldn't be a common case that something bad would happen.

Point being I don't expect FT and T2 to be dependent one on another.

They shouldn't be dependent on one another, but FT blocking T2 creates the issue.

What do you mean by "final round”? This should be documented in the spec.

I'll add this and a general note about timeouts to the issue description.

After FT enters its shutdown sequence, it blocks other threads, and does one last round of calling finalizers (see https://github.com/dotnet/coreclr/blob/a3a2d323653e08b7592ed70070f13833b2994828/src/vm/finalizerthread.cpp#L843).

Therefore, I suggest writing an error to the event log or stderr when such a situation occurs that includes some context on the blocking finalize method.

I believe this is by design on desktop CLR. The timeout is documented. In any case, the timeout was removed in CoreCLR, and with this proposal, the finalizer is not guaranteed to run at all. The expectation is that such cases will either use the dispose pattern, or use the ProcessExit event to clean up resources in cases where it is not clear when the process is about to exit (such as returning from Main).

Is this something new? As I recall once an Assembly is loaded into an AppDomain it can't be unloaded, you can only discard the AppDomain.. Any written references?

I'm not sure if unloading AssemblyLoadContexts has been implemented, but I believe that was the idea. The association between loaded assemblies and an AssemblyLoadContext is already there. See dotnet/coreclr#1684.

Then "context" is more of a loading parameter per "type of load" in any case I don't see how appending the word "Context" to the class helps to clarify this.

I think "AppDomain" and "Assembly" are two different concepts that should be encapsulated separately.

The name doesn't seem so bad to me, conceptually I think of it as "an AppDomain is an AssemblyLoadContext".

[kouvel]

Made a couple of changes:

• Added notes about there being no timeouts on running finalizers (already the case), and on processing the new event
• Changed the static ProcessExit event to an instance Unload event. For the time being, it will be raised at the same place as AppDomain.ProcessExit. Once AssemblyLoadContext unloading is implemented, it will move to a more appropriate spot.

[clrjunkie]

After FT enters its shutdown sequence, it tries to block T1, T2, and T3 before running finalizers.

FT enters its shutdown sequence because T3 called either Environment.Exit(0) or Process.Kill followed by WaitForExit(); before T2 called into GC Code, Correct?

Suppose T2 has just started a GC (see gc.cpp, GarbageCollectGeneration, GC is now flagged as started), then as part of trying to disable preemptive GC, it detects the shutdown case and blocks for shutdown

Again, because T3 has established the shutdown case, Correct?

So conceptually :

1. T3 sets shutdown flag to true and keeps running
2. T2 starts calling into GC Code detects that shutdown flag = true and blocks
3. Because T3 set shutdown flag FT was resumed, checked the flag and started to execute shutdown code that attempts to block all other threads, since T2 is blocked FT can;t block it ..

Is that what you mean?

A finalizer that does allocation and needs to allocate more space will go into gc.cpp, try_allocate_more_space, where it will block forever waiting for GC to complete.

Why would the GC not complete?

This works

class WorkItem
{
public void DoWork()
{
Console.WriteLine("Done");
}

    ~WorkItem()
    {
        Console.WriteLine("Finalizing");

        var b = new byte[10000000];
        b[0] = 1;
        Console.WriteLine("Finalized " + b[0]);
    }

The thread running shutdown code would wait forever for the finalizer thread to signal completion,

Are you taking about a different "special" thread dedicated to cleanup, who is this "thread running shutdown code" ? and why would it wait forever for the finelizer to complete, why would finilerzer get stuck (aside from programmer bugs) ?

[clrjunkie]

The name doesn't seem so bad to me, conceptually I think of it as "an AppDomain is an AssemblyLoadContext".

So maybe AssemblyLoadContext should be abstract and AppDomain should subclass it?

[clrjunkie]

Than it would make sense to call methods to load and unload dll's on (into) AppDomains..

[clrjunkie]

Maybe I miss understood the sequence:

T3 sets shutdown = true which will cause FT to start running BUT T2 is faster then FT it calls GC Code which sets "gc-running" = true and starts shutdown code.. now FT checks "gc-running" and
waits for it to become false which will never happen because T2 is shutting down and won't reset the flag...

Is that what's happening?

[kouvel]

Here's a test case that hangs on shutdown.

using System;
using System.Threading;

public class FinalizeTimeout
{
    public static int Main(string[] args)
    {
        Console.WriteLine("Main start");

        // Start a bunch of threads that allocate continuously, to increase the chance that when Main returns, one of the
        // threads will be blocked for shutdown while holding one of the GC locks
        for (int i = 0; i < Environment.ProcessorCount - 1; ++i)
        {
            var t = new Thread(ThreadMain);
            t.IsBackground = true;
            t.Start();
        }

        // Run the finalizer at least once to have its code be jitted
        BlockingFinalizerOnShutdown finalizableObject;
        do
        {
            finalizableObject = new BlockingFinalizerOnShutdown();
        } while (!BlockingFinalizerOnShutdown.finalizerCompletedOnce);

        Console.WriteLine("Main end");

        // Create another finalizable object, and immediately return from Main to have finalization occur during shutdown
        finalizableObject = new BlockingFinalizerOnShutdown() { isLastObject = true };
        return 100;
    }

    private static void ThreadMain()
    {
        byte[] b;
        while (true)
            b = new byte[1024];
    }

    private class BlockingFinalizerOnShutdown
    {
        public static bool finalizerCompletedOnce = false;
        public bool isLastObject = false;

        ~BlockingFinalizerOnShutdown()
        {
            if (finalizerCompletedOnce && !isLastObject)
                return;

            Console.WriteLine("Finalizer start");

            // Allocate in the finalizer for long enough to try allocation after one of the background threads blocks for
            // shutdown while holding one of the GC locks, to deadlock the finalizer. The main thread should eventually time
            // out waiting for the finalizer thread to complete, and the process should exit cleanly.
            TimeSpan timeout = isLastObject ? TimeSpan.FromMilliseconds(500) : TimeSpan.Zero;
            TimeSpan elapsed = TimeSpan.Zero;
            var start = DateTime.Now;
            int i = -1;
            object o;
            do
            {
                o = new object();
            } while ((++i & 0xff) != 0 || (elapsed = DateTime.Now - start) < timeout);

            Console.WriteLine("Finalizer end");
            finalizerCompletedOnce = true;
        }
    }
}

This test case has a lot of background threads doing allocation, so the chance of blocking during GC is high. After blocking background threads, the finalizer allocates as well and blocks on GC. The test case occasionally completes successfully on my machines, and hangs forever most of the time. If the test did not wait for the finalizer to run once before triggering shutdown, then allocation while jitting the finalizer function (or one of the library functions called inside) will block.

Is that what you mean?

Yes, to this and questions above it

Why would the GC not complete?

T2 blocked for shutdown after starting GC, it will not resume after that point.

So maybe AssemblyLoadContext should be abstract and AppDomain should subclass it?

Perhaps, regarding the subclassing but I'm not too familiar with them. I think AppDomain is not exposed in .NET Core, although I could be wrong.

Is that what's happening?

(From your last comment). Yes that is what happens in one case.

[clrjunkie]

Ok, I think understand the scenario.

So the question is:

T2 blocked for shutdown after starting GC, it will not resume after that point.

Why should T2 blocked for shutdown after starting GC? Why not wait for T2 to finish GC and than suspend it? The scenario is destructive by definition (in clean shutdown all user threads would be finished by the application) why is it important to hold the GC lock during shutdown which will cause FT to deadlock? Why not release GC Lock, and than suspend the thread and once all threads are suspended than run TF.