feat: add workflow client updater for updating workflow client

[robcao]

What was changed

Added an optional WorkerClientUpdater parameter to TemporalWorkerOptions. If specified, the worker will subscribe to worker client changes from the updater, and update its underlying worker client.

Why?

This allows users to update the underlying IWorkerClient on running workers without having to restart the workers entirely when using the DI extension.

Checklist

1. Closes
   [Feature Request] Support worker client update when using generic host #257

2. How was this tested:

Added test cases both for manual instantiation and DI based instantiation.

3. Any docs updates needed?

I updated the README of the hosting extension. I didn't update the README of the regular library as worker refresh is not documented there, and I don't really think there is less of a reason to use the worker client updater if not using DI.

[robcao]

I think this was a typo from the original PR? I changed the test case to check handle2 instead of handle1.

[cretz]

Yes, thanks! (hopefully my typo here didn't cause the test to pass when it shouldn't have)

[robcao]

nope, tests passed fine!

[robcao]

I wanted to re-use some of these methods that were originally in WorkflowWorkerTests.cs in TemporalWorkerServiceTests.cs, so I pulled them out into extension methods here.

[cretz]

Can you instead add them to the AssertMore class that already exists (and remove the Assert prefix of the methods and not make them extension methods)?

[robcao]

Done

[cretz]

Thanks for the PR! My original thought was that this was best in the hosting extension only since those creating workers directly don't need this abstraction.

[cretz]

I don't think any of this is needed in the Temporalio project. This is a generic host problem only. Those making workers directly can just use the Client setter. I also don't think an interface is needed unless you really think it's important for testing (even then not sure the event handler needs to be part of the interface). Also, even though it's a mouthful, I'd call this TemporalWorkerClientUpdater w/ the Temporal prefix to match the worker class name for discoverability.

[robcao]

Makes sense, done.

I don't think a lack of an interface hampers testing, since the implementation has no dependencies

[cretz]

Can you instead add them to the AssertMore class that already exists (and remove the Assert prefix of the methods and not make them extension methods)?

[cretz]

Thanks! Some more comments about visibility and general simplification I think we can do

[cretz]

Suggested change

	private readonly TemporalClientConnectOptions? newClientOptions;
	private readonly ITemporalClient? existingClient;
	private readonly TemporalWorkerOptions workerOptions;
	private readonly TemporalWorkerClientUpdater? workerClientUpdater;
	private readonly ITemporalClient? existingClient;
	private readonly TemporalWorkerServiceOptions workerOptions;

Now that we are basically storing the three separate pieces anyways, let's just store the full service options as one field

[robcao]

There's not really a good way I've found to handle the constructors that take in TemporalWorkerOptions directly (from some testing, cloning is not contravariant).

I don't know of a better way to convert an instance of TemporalWorkerOptions into TemporalWorkerServiceOptions other than writing a direct translation method, which imo is not desirable for maintenance. Do you have a suggestion?

I think it would be easier if TemporalWorkerServiceOptions was composed of TemporalWorkerOptions instead of directly inheriting from it, but that is a breaking change.

[cretz]

Ah, I see now that we have multiple constructors for this TemporalWorkerService that accept partial forms. Ok, can ignore my comment and leave these as separate fields.

[cretz]

Looks great! Will merge a bit later today.

[cretz]

Looks like locking technically not required per https://stackoverflow.com/questions/22825117/is-it-thread-safe-to-register-for-a-c-sharp-event, but this is fine