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

Throw error when agent cannot be started #23

Merged
merged 1 commit into from
Feb 2, 2024

Conversation

phunehehe
Copy link
Contributor

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"

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
Copy link
Contributor Author

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
Copy link
Collaborator

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.

@mjheilmann mjheilmann merged commit 07a295d into elixir-grpc:main Feb 2, 2024
1 check passed
@phunehehe phunehehe deleted the show-agent-start-error branch February 3, 2024 01:57
@phunehehe
Copy link
Contributor Author

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants