Avoid double Client SafeHandle implementation

[jmaeagle99]

What was changed

Changed the bridge client to no longer implement SafeHandle and to only implement IDisposable. This required that more information is pass via a new IBridgeClientProviderInternal interface that exposes the SafeClientHandle and the Runtime instances from the TemporalConnection.

Why?

Avoid the double creation of SafeHandle implementations for bridge clients.

Checklist

1. Closes Refactor Bridge.Client to be IDisposable instead of a SafeHandle #587
2. How was this tested: Unit tests
3. Any docs updates needed? No

[Sushisource]

Guess this one fell off the radar somehow huh

[cretz]

Can we just remove this class and only have SafeClientHandle? Same for the Worker one. I can't really see a benefit of having this thin layer on top.

[jmaeagle99]

Possibly. Their methods would have to go somewhere (not the Safe*Handle, because that's the whole point of the separation).

[cretz]

Why can't the methods be on the safe handle? Is the separation beneficial? It's not truly wrapping it if you're still accessing and using the safe handle elsewhere.

[cretz]

I am a bit confused on the need for this and IBridgeClientProvider. Is this duplicating that one's purpose? What was wrong with what was there and the cast?

[jmaeagle99]

As I am breaking apart the safe handles from their wrapper classes, I'm also passing around only the safe handle and not the wrapper classes anymore.

So if you look at the bridge Worker constructor, I now require passing the SafeClientHandle and the Runtime instead of the Client instance. This requires that I'm able to get the Runtime from the bridge provider somehow; but now that Client doesn't implement SafeHandle anymore, I can't just cast IBridgeClientProvider.BridgeClient to Client.

I have to either update that public interface (which might be okay, if that's not meant to be implemented by anyone outside of the SDK) or introduce a new internal interface that we can put whatever we want on it.

[cretz]

Why not pass around the wrapper? The reason we made ITemporalConnection be an IBridgeClientProvider and made that public is so if someone wrapped/implemented the connection, they could return the underlying one's BridgeClient. And that can still be used for creating workers. It was all public by intention. But at first glance, it looks like this is a breaking change because now a user's IWorkerClient that might have returned their ITemporalConnection (which implements IBridgeClientProvider by maybe delegating to the underlying one) that used to work for creating a worker can no longer work. I admit it is a bit of a contrived use case, but we did make it public on purpose.

I have to either update that public interface

Why? The worker needs the SafeHandle, why can't it continue to provide it? Isn't that what the internal one is doing, just providing the safe handle? Also, arguably the runtime should be on the safe handle for this use case.

Maybe I don't quite get the value of the separation/wrapper vs just having one safe client handle that does everything a bridge client needs to do.