samples-dotnet icon indicating copy to clipboard operation
samples-dotnet copied to clipboard

[Bug] temporal-sdk-dotnet process doesn't close if test is stopped during debug on Visual Studio 2022

Open ltlombardi opened this issue 1 year ago • 4 comments

What are you really trying to do?

Debugging unit test.

Describe the bug

During debug, If I stop before debug finishes on Visual Studio, temporal-sdk-dotnet-1.3.1.0.exe never closes / stops running. In the video I show normal operation, that temporal-sdk-dotnet process starts and closes, if test finished normally. On the second part of the video, I do it again, but stop the test execution before it finishes and temporal-sdk-dotnet never goes away. If I run another test, It creates a new instance and it starts to acumulate multiple orphaned instances. image

https://github.com/user-attachments/assets/5161ee0d-4988-40f8-8e07-d2a694a3dc80

Minimal Reproduction

Demo project to run TemporalDemo - Copy.zip

Environment/Versions

  • OS and processor: Windows 11, x86
  • Temporal Version: temporal-sdk-dotnet-1.3.1.0
  • Are you using Docker or Kubernetes or building Temporal from source? None. Unit tests doesn't use those.

ltlombardi avatar Dec 11 '24 18:12 ltlombardi

If I stop before debug finishes on Visual Studio

StartLocalAsync returns an IAsyncDisposable. Does stopping the process in this manner properly dispose objects? Or does every async disposable not get disposed when stopping this way? This SO post suggests that killing the process in this manner will not properly dispose things which includes not properly stopping the external process we start.

cretz avatar Dec 11 '24 21:12 cretz

Well, considering the SO post you provided, it seems the problem is with the way Visual Studio and the Xunit test explorer "plugin" work... ChatGPT suggested doing a clean up during the start of the unit test, so that any left overs from a previous stopped run are cleaned. It is something that can reduce the problem, not fix it. Something like this:

public class UnitTest1
{
    public UnitTest1()
    {
        ProcessManager.KillMatchingProcesses();
    }

    public static class ProcessManager
    {
        private const string ProcessNamePrefix = "temporal-sdk-dotnet";

        public static void KillMatchingProcesses()
        {
            foreach (var process in Process.GetProcesses())
            {
                try
                {
                    if (process.ProcessName.StartsWith(ProcessNamePrefix, StringComparison.OrdinalIgnoreCase))
                    {
                        process.Kill();
                        process.WaitForExit(); // Ensure it's fully terminated
                        Console.WriteLine($"Killed process: {process.ProcessName} (ID: {process.Id})");
                    }
                }
                catch (Exception ex)
                {
                    Console.WriteLine($"Failed to terminate process {process.Id}: {ex.Message}");
                }
            }
        }
    }
}

ltlombardi avatar Dec 12 '24 10:12 ltlombardi

Yes, you can add this to clean up if you do not properly dispose. This is the same for other non-sub/child processes started in an app that is terminated non-gracefully. Note you may affect other running tests in other processes that have started their own test servers though. I do not believe we would add this in the SDK though, rather we strongly suggest not killing processes in a non-graceful manner. For some users, they may intentionally start a detached test workflow environment from .NET code. it is the user's choice to not gracefully dispose the workflow environment, and some may choose to do so intentionally.

cretz avatar Dec 12 '24 14:12 cretz

It is something can happens without intent.

  1. I start a unit test, with debug, to investigate why it was failing.
  2. I find the problem.
  3. I stop, to fix it, since there is not reason to let it finish.

I never though that it was better to let it finish, to avoid this kind of problems..

Thank you for the quick reply 🥇 ❤️

ltlombardi avatar Dec 12 '24 15:12 ltlombardi