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

weird behaviour when deep merging endpoint options #5758

Open
sasa1977 opened this issue Mar 21, 2024 · 1 comment
Open

weird behaviour when deep merging endpoint options #5758

sasa1977 opened this issue Mar 21, 2024 · 1 comment

Comments

@sasa1977
Copy link
Contributor

Environment

  • Elixir version (elixir -v): 1.16.0
  • Phoenix version (mix deps): 1.7.11
  • Operating system: Ubuntu

Actual behavior

Phoenix endpoint supports providing options overrides via the start_link argument. Suppose that in runtime.exs we have the following:

config :my_app, MyEndpoint,
  http: [
    transport_options: [socket_opts: [:inet6]],
    ...
  ]

And then we use the childspec {MyEndpoint, http: [transport_options: [socket_opts: [debug: true]]]}.

The final configuration will be: socket_opts: [:inet6]. I.e. the provided socket_opts override was ignored.

OTOH, if socket_opts in the config is a keyword list, merging will succeed, and the final socket_opts will be a result of merging both kw lists. In this case, the socket_opts list passed to start_link will take precedence (i.e. it will overwrite the values with the same keys from app env).

example script
Application.put_env(:phoenix, :json_library, Jason)

Application.put_env(:sample, SamplePhoenix.Endpoint,
  http: [
    port: 4000,
    transport_options: [socket_opts: [:inet6]]
  ],
  server: true,
  secret_key_base: String.duplicate("a", 64)
)

Mix.install([
  {:plug_cowboy, "~> 2.5"},
  {:jason, "~> 1.0"},
  {:phoenix, "~> 1.7.0"}
])

defmodule SamplePhoenix.Endpoint do
  use Phoenix.Endpoint, otp_app: :sample
end

{:ok, _} =
  Supervisor.start_link(
    [{SamplePhoenix.Endpoint, http: [transport_options: [socket_opts: [debug: true]]]}],
    strategy: :one_for_one
  )

IO.inspect(SamplePhoenix.Endpoint.config(:http))

Expected behavior

Not sure :-)

Ideally, two socket_opts lists would be merged, but I understand that this is a tricky decision which might cause implicit issues in other places.

If there's no merging, then I'd at least expect the option passed to start_link to take precedence, i.e. to overwrite the one from the app env. Because that's how it behaves when both values are proper kw lists.

Yet another option is to warn/error, but I'm not sure this is a good idea in general.

@mtrudel
Copy link
Member

mtrudel commented Mar 21, 2024

The tricky thing here is that the transport_opts list (which gets passed more or less directly to the underlying transport; either :gen_tcp or :ssl) isn't a keyword list, it's a proplist. Moreover, :gen_tcp and :ssl don't use the 'first value wins' semantic for duplicate entries in this list, which makes merging an explicit affair. See mtrudel/thousand_island#113 and mtrudel/thousand_island#111 for details.

I feel pretty strongly that Thousand Island should expose the exact same interface for configuration as the underlying transport does (that is, I really don't want to be in the business of rewriting people's configurations as it's a never-ending treadmill of special cases). Unsure of the best way to resolve this, TBH. Open to options!

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

No branches or pull requests

2 participants