Fix double shutdown

[reyang]

Changes

• Fixed the double shutdown call issue for CompositeActivityProcessor (there is a similar bug in FanOutActivityProcessor which I'm not fixing in this PR).
• Cleaned up / simplified the code.

For significant contributions please make sure you have completed the following items:

• Design discussion issue #
• Changes in public API reviewed

[codecov]

Codecov Report

Merging #1017 into master will decrease coverage by 0.04%.
The diff coverage is 53.33%.

@@            Coverage Diff             @@
##           master    #1017      +/-   ##
==========================================
- Coverage   75.56%   75.52%   -0.05%     
==========================================
  Files         222      222              
  Lines        6208     6210       +2     
==========================================
- Hits         4691     4690       -1     
- Misses       1517     1520       +3     

Impacted Files	Coverage Δ
...metryProtocol/Implementation/ActivityExtensions.cs	90.22% <ø> (ø)
...c/OpenTelemetry.Exporter.ZPages/ZPagesProcessor.cs	91.30% <ø> (ø)
...strumentation.Http/Implementation/HttpTagHelper.cs	92.85% <ø> (ø)
src/OpenTelemetry/Metrics/MeterProviderSdk.cs	75.00% <ø> (ø)
src/OpenTelemetry/Sdk.cs	63.77% <ø> (ø)
.../OpenTelemetry/Trace/CompositeActivityProcessor.cs	0.00% <0.00%> (ø)
...rc/OpenTelemetry/Trace/TracerProviderExtensions.cs	0.00% <ø> (ø)
src/OpenTelemetry/Trace/ActivityProcessor.cs	73.68% <66.66%> (-12.99%)	⬇️
src/OpenTelemetry/Trace/TracerProviderSdk.cs	100.00% <100.00%> (ø)

[CodeBlanch]

This is probably fine but I don't think necessarily it is the standard/preferred pattern. This is what I'm used to doing:

public void Shutdown()
{
   if (!IsRunning) return;

   // Shutdown

   IsRunning = false;
}

public void Dispose()
{
   Dispose(true);
   GC.SuppressFinalize(this);
}

~ClassName()
{
   Dispose(false);
}

protected virtual void Dispose(bool disposing)
{
   Shutdown();

   if (disposing && !disposed)
   {
       // Call dispose on owned refs.

       disposed = true;
   }
}

The reason for that is if someone makes a mistake and forgets to call Dispose, you want the finalizer to still call Shutdown. Because it might have a thread or something it needs to signal which will prevent the process from spinning down.

[reyang]

I see your point. I was concerned about running complex managed code in Finalizer thread as it might clog the entire GC (which turns an application developer's problem into a runtime library developer's problem). Thoughts?

For example, I would avoid running context manipulation code in the Finalizer thread.

[CodeBlanch]

@reyang Also a good point. I suppose it's a tradeoff. We want users to call Dispose, and most will, negating any hit, but we need to utilize the finalizer to do the cleanup just in case the user messes up?

@noahfalk @tarekgh Would love to get an expert opinion on this if you guys have the time. Any official guidance on calling Shutdown/Close in Dispose/Finalizer?

[reyang]

I think this is something we can change later, not necessarily blocking the PR.
Created #1024 for tracking.

[tarekgh]

I didn't look at the details, but here is the guidlines for disposing in general https://docs.microsoft.com/en-us/dotnet/standard/garbage-collection/implementing-dispose. it is recommended to use SafeHandle instead of providing finalizer.

[CodeBlanch]

Sent a comment, but LGTM.

[ThomsonTan]

Do we need set disposed to true if exception happened in the call to ShutdownAsync, or just throw the exception out?

[reyang]

I think we need to set to true as dispose is not supposed to fail / retry.