Keep worker alive if update_documents fails #37
Conversation
test/openid_connect/worker_test.exs
Outdated
|
|
||
| defp start_worker(config) do | ||
| {:ok, pid} = start_supervised({OpenIDConnect.Worker, config}) | ||
| Process.sleep(10) # allow :update_documents to run |
There was a problem hiding this comment.
Doing sleep during a test isn't ideal. It would be better if the pid of the test could be added as an option and if present the worker would notify the pid when the document was updated. Then the test would assert_received for that message and timeout/fail if it doesn't happen.
There was a problem hiding this comment.
I agree. The test doesn't really need to be notified when update_documents completes (it's mocked), but the :update_documents should be in the worker's queue before the test continues.
There was a problem hiding this comment.
as it stands, I think the PR's architecture implementation is wrong. The update_documents function should be blocking. I'd like to see failing test cases and work back from there to a solution
There was a problem hiding this comment.
update_documents is still blocking. It was already a handle_info before this PR, I haven't changed that, and it schedules itself periodically, as before. I deferred it's invocation from init/1 to allow the worker (and it's supervision tree) to remain alive in the case of a temporary network error (domain resolution failure, for example).
Feel free to look at alternatives (#30).
lib/openid_connect/worker.ex
Outdated
| :ignore | ||
| end | ||
|
|
||
| def init(provider_configs) do | ||
| def init({provider_configs, notify_pid}) do |
There was a problem hiding this comment.
I think instead this should be a separate functional call that is something like:
def init({provider_configs, pid}) is_id(pid) do
{:ok, state} = init(provider_configs)
Process.send(pid, :ready)
{:ok, state}
endyou can drop the handle_info function
There was a problem hiding this comment.
Third time lucky. I'm using Process.send_after rather than Process.send to ensure that update_documents will be in the workers inbox before the test proceeds.
|
@bcardarella sorry to bother but have you had a chance to give this any consideration? At the moment, without this PR or something similar, an |
|
sorry this has gone stale, DY doesn't have internal resources to support the library so I am going to start seeking an external maintainer. |
No problem... I'd be happy to help in a limited capacity. |
See #24