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

Ensure :name is given in Registry.start_link/1 #8850

Merged
merged 3 commits into from Mar 4, 2019

Conversation

Projects
None yet
3 participants
@SpaceEEC
Copy link
Contributor

SpaceEEC commented Mar 3, 2019

Issue

Currently when omitting a :name when starting Registry it will be named nil.

Small example:

Interactive Elixir (1.9.0-dev) - press Ctrl+C to exit (type h() ENTER for help)
iex(1)> {:ok, _pid} = Registry.start_link(keys: :unique)
{:ok, #PID<0.102.0>}
iex(2)> {:ok, _pid} = Registry.register(nil, :foo, :bar)
{:ok, #PID<0.103.0>}
iex(3)> [{_pid, val}] = Registry.lookup(nil, :foo)
[{#PID<0.100.0>, :bar}]

This seems to be because this line defaults name to nil:

name = Keyword.get(options, :name)

Solution

I added Keyword.has_key?(options, :name) to the check below the mentioned line to ensure that a name actually was given.

unless is_atom(name) do

Small example

Interactive Elixir (1.9.0-dev) - press Ctrl+C to exit (type h() ENTER for help)
iex(1)> Registry.start_link(keys: :unique)
** (ArgumentError) expected :name to be given and to be an atom, got: nil
    (elixir) lib/elixir/lib/registry.ex:322: Registry.start_link/1

It would also be possible to default it to a non-atom value, but that would probably make the error message even more misleading.

Notes

  • The error message will be expected :name to be given and to be an atom, got: nil then.
    But nil is a valid name! It just was not explicitly given.

Would another unless/2 with a more explicit error message be appropriate?

  • I also added a test for this.
    As there are none for the other options this might not be necessary, or misplaced.
@whatyouhide

This comment has been minimized.

Copy link
Member

whatyouhide commented Mar 3, 2019

We might be better off with something like this:

name =
  case Keyword.fetch(options, :name) do
    {:ok, name} when is_atom(name) -> name
    {:ok, other} -> raise ArgumentError, "expected :name to be an atom, got: #{inspect(other)}"
    :error -> raise ArgumentError, "expected :name option to be present"
  end

...

Wdyt?

@SpaceEEC

This comment has been minimized.

Copy link
Contributor Author

SpaceEEC commented Mar 3, 2019

I think this is a good idea. 👍

@@ -602,7 +602,7 @@ defmodule RegistryTest do
end

test "raises if :name is missing" do
assert_raise ArgumentError, ~r/expected :name/, fn ->
assert_raise ArgumentError, ~r/expected :name option to be present/, fn ->

This comment has been minimized.

@whatyouhide

whatyouhide Mar 3, 2019

Member

Can we quickly add a test for the case where :name is present but not an atom? Then we're good to merge!

@ericmj

ericmj approved these changes Mar 3, 2019

@whatyouhide whatyouhide merged commit 33d4842 into elixir-lang:master Mar 4, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@whatyouhide

This comment has been minimized.

Copy link
Member

whatyouhide commented Mar 4, 2019

Thanks @SpaceEEC! 💟

@SpaceEEC SpaceEEC deleted the SpaceEEC:fix/registry_name branch Mar 5, 2019

Hanspagh added a commit to Hanspagh/elixir that referenced this pull request Mar 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.