elixir-grpc / grpc-reflection

elixir graph reflection support
Apache License 2.0
9 stars 6 forks source link

Throw error when agent cannot be started #23

Closed phunehehe closed 8 months ago

phunehehe commented 8 months ago

Previously if there's an error when starting an agent, the error is effectively silenced. I'm guessing this is to suppress already_started "errors". Unfortunately this also suppress any error

With this change, already_started will be ignored, but any other error will blow up, making it easy to see what went wrong. I considered logging an error and continuing, but it's simpler to blow up, and feels aligned with "let it crash"

phunehehe commented 8 months ago

FWIW the error I was getting was

{:error,
 {:badarg,
  [
    {:erlang, :binary_to_existing_atom,
     ["Elixir.Abc.V1.GetSomethingRequest", :utf8],
     [error_info: %{module: :erl_erts_errors}]},
    {:elixir_aliases, :safe_concat, 1, [file: 'src/elixir_aliases.erl', line: 141]},
    {GrpcReflection.Service.Builder, :process_reference, 1,
     [file: 'lib/grpc_reflection/service/builder.ex', line: 62]},
    {GrpcReflection.Service.Builder, :"-process_references/1-fun-0-", 2,
     [file: 'lib/grpc_reflection/service/builder.ex', line: 24]},
    {Enum, :"-reduce/3-lists^foldl/2-0-", 3, [file: 'lib/enum.ex', line: 2396]},
    {GrpcReflection.Service.Builder, :process_references, 1,
     [file: 'lib/grpc_reflection/service/builder.ex', line: 24]},
    {GrpcReflection.Service.Agent, :start_link, 2,
     [file: 'lib/grpc_reflection/service/agent.ex', line: 17]},
    {DynamicSupervisor, :start_child, 3, [file: 'lib/dynamic_supervisor.ex', line: 693]}
  ]}}

It happened because I have a step after building the protos, to rename Abc to ABC in the generated files to match a naming convention. That's kinda my fault I will remove it, but I thought you may want to consider doing something like looking up the type module instead of reconstructing it from the string type name

mjheilmann commented 8 months ago

An earlier implementation of the reflection module did perform an open-ended lookup of registered modules within the erlang runtime, but we changed the approach due to the following:

  1. Allowing public or external clients to perform module-name discovery against a running service can be a security risk, and I've worked in environments where that would be enough to cause problems.
  2. When you start your reflection instance, the grpc service and all message types are provided, so a search should not be necessary.

As far as the enforced naming convention, this was written to cooperate with the names and types that are exported from the upstream grpc plugin and compiler, I think that if you were willing to go through some effort you may be able to achieve your naming convention consistency at the expense of not being able to regenerate your modules when you want, as well as editing the generated descriptor/0 functions; but that use case is not tested of course.

phunehehe commented 8 months ago

This is way out of what I know but I didn't mean an open ended lookup. Looking in the generated Elixir files I see both the name and the type, so I'm naively thinking it may be possible to build a precise mapping from name to type using that info. Anyway, you clearly have thought about it and I don't want the headache of fighting the tools. Thanks for merging it so quickly!