Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Supervisor process foundable with :via option #44

Merged
merged 5 commits into from
Jan 26, 2024

Conversation

DohanKim
Copy link
Contributor

(This PR is stacked with the previous one)

The current implementation of WalEx.Config.Registry

  def set_name(:set_supervisor, module, app_name), do: {:via, module, app_name}

makes the supervisor process can’t be found with GenServer.whereis
as WalEx.Supervisor is not implementing Registry behavior

test:

      assert {:ok, walex_supervisor_pid} = WalExSupervisor.start_link(@base_configs)

      assert walex_supervisor_pid ==
               GenServer.whereis(
                 WalExRegistry.set_name(:set_supervisor, WalExSupervisor, :test_name)
               )

produces the error

** (UndefinedFunctionError) function WalEx.Supervisor.whereis_name/1 is undefined or private
    (walex 3.7.0) WalEx.Supervisor.whereis_name(Sendman)
    (elixir 1.16.0) lib/gen_server.ex:1286: GenServer.whereis/1
    (elixir 1.16.0) lib/gen_server.ex:1059: GenServer.stop/3

So I changed the WalEx.Config.Registry to use

  def set_name(:set_supervisor, module, app_name), do: set_name(module, app_name)

like other modules

also,

    Supervisor.start_link(__MODULE__, configs: supervisor_opts, name: name)

doesn’t properly register the process name (it is using start_link/2)
We can check this with the test case above.

So I changed to use start_link/3

Supervisor.start_link(__MODULE__, supervisor_opts, name: name)

With this change, each Supervisor is starting with the name attached and also foundable with its name

@DohanKim
Copy link
Contributor Author

DohanKim commented Jan 25, 2024

also changed other supervisors to use start_link/3 to attach the name properly

@DohanKim
Copy link
Contributor Author

fixing the test failure

@DohanKim
Copy link
Contributor Author

there was also a bug setting WalEx.Destinations.Supervisor name
reading :app_name key instead of :name

@cpursley
Copy link
Owner

Very much appreciated @DohanKim

@cpursley cpursley merged commit d378887 into cpursley:master Jan 26, 2024
1 check passed
@cpursley
Copy link
Owner

cpursley commented Jan 26, 2024

@DohanKim I just created a new release: https://github.com/cpursley/walex/releases/tag/v3.8.0

Also, does this finally resolve #38 or are you still experiencing issues?

@DohanKim
Copy link
Contributor Author

I think it would, but as I can't reproduce the problem on dev or test environment, I have deployed the PR on my prod server and watching if it's okay. I can say it's resolved whenever a temporary DB connection problem happens and WalEx recovers the connection successfully.

@cpursley
Copy link
Owner

Okay, feel free to close the issue whenever you feel confident that it's resolved :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants