ArgumentError :erlang.binary_to_existing_atom/2

[kuznetskikh]

While trying to work with FunWithFlagsUI (especially with any new flag) from time to time we get such weird error:

ArgumentError :erlang.binary_to_existing_atom/2
errors were found at the given arguments: * 1st argument: not an already existing atom

lib/fun_with_flags/ui/router.ex in anonymous fn/3 in FunWithFlags.UI.Router.do_match/4 at line 99
lib/plug/router.ex in anonymous fn/4 in FunWithFlags.UI.Router.dispatch/2 at line 246
/opt/app/deps/telemetry/src/telemetry.erl in :telemetry.span/3 at line 321
lib/plug/router.ex in FunWithFlags.UI.Router.dispatch/2 at line 242
lib/fun_with_flags/ui/router.ex in FunWithFlags.UI.Router.plug_builder_call/2 at line 1
lib/phoenix/router/route.ex in Phoenix.Router.Route.call/2 at line 42
lib/phoenix/router.ex in Phoenix.Router.__call__/5 at line 425

I think it comes from there:

https://github.com/tompave/fun_with_flags_ui/blob/master/lib/fun_with_flags/ui/router.ex

defmodule FunWithFlags.UI.Router do
  ...
  get "/flags/:name" do
    case Utils.get_flag(name) do
      ...
    end
  end
  ...
end

https://github.com/tompave/fun_with_flags_ui/blob/master/lib/fun_with_flags/ui/utils.ex

defmodule FunWithFlags.UI.Utils do
  ...
  def get_flag(name) do
    ..
    case FunWithFlags.SimpleStore.lookup(String.to_existing_atom(name)) do
      ...
    end
    ...
  end
  ...
end

Maybe it's worth to replace String.to_existing_atom with String.to_atom to avoid the issue, what do you think?

[tompave]

Hey there, thank you for using the package and for reporting this.

How many BEAM notes are you running? I wonder to what extent this could be caused by the GET request landing on a cold node that has never loaded the flag in memory before (that's what creates the flag-name atom for the first time).

[kuznetskikh]

Hello @tompave! Thanks for the answer. It happened while having 3 nodes.

[tompave]

I see. Then that happens because your GET /flags/:flag_name request lands on a BEAM node that hasn't loaded that flag in memory yet. The earliest that can happen is by visiting GET /flags, as all flags are loaded in memory from the DB, and the atoms for the names are created.

The reason I'm hesitant to replace String.to_existing_atom/1 with String.to_atom/1 is that in the case of GET /flags/:flag_name we're dealing with user input. Since atoms are not garbage collected, creating atoms for user input would be an attack vector that could make the BEAM node go out-of-memory. This is a bit safer when loading flags from the DB, where String.to_atom/1 is used.

[kuznetskikh]

Oh, I got it. I'll try to provide force early visiting of /flags, to load flags in memory 👍
So I think we can close this ticket. Thanks @tompave!

[tompave]

It's still not a great solution though. Even if just a few nodes, refreshing GET /flags won't guarantee that you hit all nodes. And as the number of nodes increases, the solution becomes less and less practical.

This is not normally a problem when the flag is actually checked by the application at runtime (outside FunWithFlags.UI), as that would cause the flag name atom to be encountered first and loaded in memory, because you have to reference it as an atom literal in your code, in order to check a flag. So, in that case, if the flag is checked in a frequently executed code path, chances are that all your BEAM nodes will have already loaded it by the time you try to access it at GET /flags/:flag_name. Even in this case, though, the situation is not perfect.

Perhaps the right solution is to wrap that String.to_existing_atom/1 with a try...rescue, and if it fails with the atom error, check if the flag actually exists. We could also check it immediately before trying to load the flag.

[tompave]

Ok, I've just realized that I had already solved this problem ages ago. That flag lookup is wrapped with if safe_flag_exists?(name):

fun_with_flags_ui/lib/fun_with_flags/ui/utils.ex

Lines 75 to 77 in caca4f7

	def get_flag(name) do
	if safe_flag_exists?(name) do
	case FunWithFlags.SimpleStore.lookup(String.to_existing_atom(name)) do

Which basically does what I described above:

fun_with_flags_ui/lib/fun_with_flags/ui/utils.ex

Lines 151 to 158 in caca4f7

	defp safe_flag_exists?(name) do
	try do
	{:ok, all} = FunWithFlags.all_flag_names()
	Enum.member?(all, String.to_existing_atom(name))
	rescue
	ArgumentError -> false
	end
	end

I'm not sure why you're getting that error then.
Perhaps you're running on a DB cluster, and there is some replica lag?

[tompave]

Your stacktrace:

lib/fun_with_flags/ui/router.ex in anonymous fn/3 in FunWithFlags.UI.Router.do_match/4 at line 99
lib/plug/router.ex in anonymous fn/4 in FunWithFlags.UI.Router.dispatch/2 at line 246

Ok, so that's happening on different endpoints, not on GET /flags/:flag_name. That makes sense then. The fix it to make those safe as well.

The line numbers don't perfectly align. What version of FWF.UI are you using?

[kuznetskikh]

Hmm, as I see in my mix.lock: 0.8.1

[tompave]

Ok, it might be an effect of the Plug macros.

Anyway, I can see what's happening here. The fix would be to make the other endpoints safe the same way that GET is.

[michallepicki]

We encountered this issue when trying to clean up after a feature flag that we removed from our code, so its atom name was no longer referenced/created anywhere.