antlai-temporal
commented
7 months ago Agreed. I would call this try_update_options and only swap if the connection succeeds. Also I find leaving the old options to be scary since now things aren't in sync.
This is what I tried first, but as I explained above, it is all asynchronous, with lazy endpoints, and there is no error callback, so it is difficult to make the update only when it works... Leaving the old options is really ugly, agreed. I checked that we don't use the possibly outdated fields in any critical manner but there is no guarantee that something will eventually break. It is also unrealistic that we will always have a mutable ref to client unless we add a top level mutex, which would mess up backwards compatibility. Adding a lock on ClientOptions does not completely solve the problem, you need to hold both locks (also for update field), to safely do the update, and locks need to be acquired once, and in the correct order, to avoid deadlocks. I thought that could be quite intrusive too...
If we can live with occasional inconsistencies, we could lock them independently... That would improve on what we have when we have no mutable ref...
It'd be fine to put a mutex around the options as well, so that they can be updated too. This also fits better with the idea of using the ClientOptions directly rather than ClientOptionsUpdate See discussion above... The problem is that you need to hold both locks to keep it consistent
What was changed
Clients can now dynamically update certain options fields such as the target url, the TLS configuration, the origin, or the keep alive settings, without creating a new client.
Why?
This will allow, e.g., updating a TLS certificate without restarting workers, or losing state associated with the client, such as, eager workflow start tracking, or metrics...
Checklist
477
Integration tests and unit tests