Blame ets errors directlty in Elixir stdlib

[josevalim]

Hi @TheFirstAvenger!

Thanks for showing me this library doing ElixirConf. I have been thinking about it and I realized that the part related to the error messages could be added directly to Elixir itself!

Elixir exceptions have a blame mechanism, which is used to add more information. We can use the blame mechanism for ArgumentError to see if the error came from ets and provide more information. We already do this for erlang:apply/3, for example:

https://github.com/elixir-lang/elixir/blob/master/lib/elixir/lib/exception.ex#L685

What do you think? Would you be interested in sending a PR? We could start with something simple, such as blaming ets:lookup, and then build it from there.

[TheFirstAvenger]

@josevalim Love it! Looking at that solution and the current implementation of ETS in this library, I see two possible paths to success here. One is too take the concepts from this library and strip them down to fit inside this blame mechanism. This would allow those using :ets directly to benefit from the improved exception message strings, but would miss out on the other features, such as being able to pattern match on error tuples and the improved design around the new function. The other route would be to add the ETS module to Elixir similar to how it is designed here, and then call that module inside the blame function. This would give Elixir users two paths, the :ets path for backwards compatibility with improved string error messages, but also the ETS path for the improved developer experience. Thoughts?

[josevalim]

Yes, I am mostly thinking about the blame bits and improving the exceptions. Adding an ETS module to Elixir would open up a whole bunch of discussions and changes that won't lead anywhere but the exceptions bits are a no brainer!

[TheFirstAvenger]

@josevalim Ok sounds good. I had to ask 😁.

Took an initial stab at a methodology this morning, here is what I am thinking:

  # Captures all ets argument errors and runs them through `blame_ets` for a new error message
  def blame(
        %{message: "argument error"} = exception,
        [{:ets, fun, args, _} | _] = stacktrace
      ) do
    {%{exception | message: blame_ets(fun, args)}, stacktrace}
  end

  # With statement tests all known possible failures for this function (:insert), falling through to generic if none found
  defp blame_ets(:insert, [table_name, _]) do
    with false <- ets_table_missing?(table_name) do
      "argument error calling :ets.insert"
    end
  end

  # For ets functions we don't yet capture
  defp blame_ets(fun, args) do
    "argument error calling :ets.#{fun}"
  end

  # First check. Will be the first line in most `blame_ets` functions
  defp ets_table_missing?(table_name) do
    case :ets.info(table_name) do
      :undefined -> "ets table not found: #{inspect(table_name)}"
      _ -> false
    end
  end

With this methodology I can build out both the functions I am able to catch issues on (by adding blame_ets for each one) and the types of issues I can detect (by adding ets_* functions for them and adding them to the with statement of applicable blame_ets functions).

Thoughts?

[zachdaniel]

Not that I think this is a reason not to do this, but couldn't this result in an incorrect error message? Say you do :ets.insert and get a different argument error, and then in between that and the blame, some other process deletes the table. Is that a case worth considering even?

[TheFirstAvenger]

@zachdaniel Yes, you are correct, this "race condition" is a possibility based on the approach of testing after-the-fact. Not sure how best to address this.

[zachdaniel]

I wonder if just changing the copy is a good enough, something more to the effect of "it is likely that the table does not exit" or something like that?

[TheFirstAvenger]

👍 I was thinking the same thing.

[josevalim]

Yeah, that's precisely it and I agree regarding the table being removed. I would also do things like check if we have access or not. I suggest for us to start with three functions, like insert, lookup and lookup_element as the starting point in the initial PR and then we build the rest. WDYT?

[TheFirstAvenger]

elixir-lang/elixir#9379