absinthe_plug
absinthe_plug copied to clipboard
Handle "unavailable" schema module status more correctly
I've been dancing around these unpredictable "is not a valid Absinthe.Schema
" errors for months now, and finally took some time to figure out exactly what's going on. It turns out that when checking the validity of the schema with Code.ensure_compiled/1
, it's possible that the module is not available yet, but may be later. We don't currently handle that condition, so if the compiler hits a particular race condition, the schema is simply rejected. This little tweak seems to fix my issues completely :)
The Elixir documentation specifies for Code.ensure_compiled/1 that {:error, :unavailable} is not necessarily an error condition, as the deadlock could still be broken by the compiler. So we give the compiler a little time to break the deadlock and retry a number of times before we give up.
Anything you'd like me to do to help get this merged?
@benwilson512 This fixes a pretty severe bug. It particularly affects users that need to pull in functions from other modules (e.g. values for enums). The race condition can completely break schema compilation, and the resulting error is very obscure.
Given that the fix is so simple, can we get this merged?
What version of Elixir are you on?
Elixir 1.13.0
So, FYI, from the Elixir docs:
If it succeeds in loading the module, it returns {:module, module}. If not, returns {:error, reason} with the error reason. If the module being checked is currently in a compiler deadlock, this function returns {:error, :unavailable}. Unavailable doesn't necessarily mean the module doesn't exist, just that it is not currently available, but it (or may not) become available in the future.
https://hexdocs.pm/elixir/Code.html#ensure_compiled/1
The :unavailable
situation is the one that is currently not handled, and this PR introduces the check required to handle it correctly.
For what it's worth, I've been using this patch in production for several months now, and it's completely resolved the schema build issues we were having, and doesn't seem to have introduced any new issues.
@kdawgwilk Any chance we could get this merged?