TracerProvider allows calling Tracer() while it's shutting down

[ash2k]

Fixes #3890.

I decided to introduce a separate mutex to decouple Tracer() from ALL other methods. I think this is way more robust than trying to release locks early, etc. Releasing mu early would have unintended, potentially undesirable effect of allowing Register/Unregister vs Shutdown to be unblocked earlier, while a concurrent call is still running. It may be hard to use the tracer provider in some scenarios if it works like that. It may be fine, but I think this is very subtle and can be undesirable behavior.

I've also moved logging out of the protected section just to be sure no external code can be called while holding the lock to avoid any chance of deadlock.

[codecov]

Codecov Report

Merging #3924 (f07c478) into main (ae90c44) will decrease coverage by 0.1%.
The diff coverage is 73.9%.

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #3924     +/-   ##
=======================================
- Coverage   81.8%   81.7%   -0.1%     
=======================================
  Files        170     170             
  Lines      12911   12943     +32     
=======================================
+ Hits       10563   10577     +14     
- Misses      2129    2143     +14     
- Partials     219     223      +4     

Impacted Files	Coverage Δ
sdk/trace/provider.go	84.2% <73.9%> (-3.0%)	⬇️

... and 2 files with indirect coverage changes

[Aneurysm9]

I think the first step in Tracer() needs to be to check whether the provider has been shut down and to return a no-op if it has:

After the call to Shutdown, subsequent attempts to get a Tracer are not allowed. SDKs SHOULD return a valid no-op Tracer for these calls, if possible.

In that case there would not be a way for a SpanProcessor's Shutdown() to enter protected sections of Tracer() as the flag indicating the provider is shut down would already be set. I think that puts Tracer() back in a position of needing to take a lock on the mutex guarding that flag, though, and not just the tracer map so maybe the flag should be changed to an atomic.Bool?

[ash2k]

@Aneurysm9 This makes sense, thanks! I've changed the code accordingly.

[pellared]

In order to avoid deadlocks, I believe the double-checked locking is needed in all exported methods (similarly to Tracer()) as all of them can be used within Shutdown() (or UnregisterSpanProcessor()) via external code (sp.Shudown() and otel.Handle())

Leaving the decission for #3924 (comment) to the maitainers