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.

Sign up for GitHub

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

Jump to bottom

Support dynamic port for c:Endpoint.url/0 #5553

Closed
wants to merge 3 commits into from
Closed

Support dynamic port for c:Endpoint.url/0 #5553

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d1e7923
Resolve port 0 to dynamic port when server is started
LostKobrakai Aug 16, 2023
3c4ded3
Add check for callback
LostKobrakai Aug 16, 2023
cf5911f
Deduplicate endpoint module
LostKobrakai Aug 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion lib/phoenix/endpoint/cowboy2_adapter.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ defmodule Phoenix.Endpoint.Cowboy2Adapter do
Application.ensure_all_started(:ssl)
end

ref = Module.concat(endpoint, scheme |> Atom.to_string() |> String.upcase())
ref = build_ref(endpoint, scheme)

plug =
if code_reloader? do
Expand Down Expand Up @@ -133,6 +133,18 @@ defmodule Phoenix.Endpoint.Cowboy2Adapter do
_ -> scheme
end

@doc false
def dynamic_port(endpoint, scheme) do
case :ranch.get_addr(build_ref(endpoint, scheme)) do
{:local, _unix_path} -> raise "Trying to determine dynamic port, but the adapter is configured to use unix sockets. Remove the configuration for `[port: 0]` on the #{scheme} scheme."
Comment on lines +138 to +139
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest plumbing through a value of nil for the port in the case of domain sockets; it's better than falling back to the scheme port as we're doing below.

Regardless, it's an unwinnable battle either way I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I saw it the other way round. If you're using unix socket nothing should even ask for a port, as that doesn't really make sense. Given the above discussion returning nil might be more appropriate though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. If we step back and observe that the goal here is to build a URI for the server, we should be asking 'what does that URI even look like for a domain socket' (and secondarily, 'is that the URI that the user cares about').

We have answers to this already in Bandit and Cowboy. If we wanted to follow down that path we should be extracting both the address and the port from the underlying server (that makes sense to me, FWIW).

{_addr, port} -> port
end
end

defp build_ref(endpoint, scheme) do
Module.concat(endpoint, scheme |> Atom.to_string() |> String.upcase())
end

# TODO: Remove this once {:system, env_var} deprecation is removed
defp port_to_integer({:system, env_var}), do: port_to_integer(System.get_env(env_var))
defp port_to_integer(port) when is_binary(port), do: String.to_integer(port)
Expand Down
37 changes: 35 additions & 2 deletions lib/phoenix/endpoint/supervisor.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,8 +351,8 @@ defmodule Phoenix.Endpoint.Supervisor do

{scheme, port} =
cond do
https -> {"https", https[:port] || 443}
http -> {"http", http[:port] || 80}
https -> {"https", transport_port(endpoint, https, :https)}
http -> {"http", transport_port(endpoint, http, :http)}
Comment on lines -354 to +355
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to be so explicit about sorting through the config ourselves? Rather than special casing the times where the port is dynamic, we could just always ask the adapter for the port (I'm happy to add dynamic_port/2 to Bandit.PhoenixAdapter) and be done with it. Taken with the changes suggested for domain sockets above, it would strip away most of the complexity of the changes to this file, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds good, but it also kinda prepends on the adapter being able to return a port / being started. That should be a simpler check though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point - at many of the calls we're guaranteed that the server won't be running. What ought to be the behaviour in those cases, do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if the server is not running the current behaviour (of rendering :0 as port) is already the best one. Phoenix builds up a url it expects the server to bind to. That behaviour just doesn't help when a server is actually started and urls need to be build to connect to that server. In that case phoenix should be able to provide the port the server was bound to instead.

This means querying the information after startup and at best also make sure that cached information stays up to date with the bound port in case of crashes/restarts or the suspend you mentioned.

true -> {"http", 80}
end

Expand All @@ -369,6 +369,39 @@ defmodule Phoenix.Endpoint.Supervisor do
%URI{scheme: scheme, port: port, host: host}
end

defp transport_port(endpoint, config, scheme) do
cond do
value = config[:port] ->
value
|> port_to_integer()
|> handle_dynamic_port(endpoint, scheme)

scheme == :https ->
443

scheme == :http ->
80
end
end

def handle_dynamic_port(0, endpoint, scheme) do
config =
cond do
server = endpoint.config(:server) -> [server: server]
true -> []
end

adapter = endpoint.config(:adapter) || Phoenix.Endpoint.Cowboy2Adapter

if function_exported?(adapter, :dynamic_port, 2) and server?(config) do
adapter.dynamic_port(endpoint, scheme)
else
0
end
end

def handle_dynamic_port(port, _, _), do: port

defp warmup_static(endpoint, %{"latest" => latest, "digests" => digests}) do
Phoenix.Config.put_new(endpoint, :cache_static_manifest_latest, latest)
with_vsn? = !endpoint.config(:cache_manifest_skip_vsn)
Expand Down
22 changes: 22 additions & 0 deletions test/phoenix/endpoint/endpoint_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,28 @@ defmodule Phoenix.Endpoint.EndpointTest do
assert_receive {^pid, :sample}
end

@tag :capture_log
test "uses bound dynamic port if server is started" do
Application.put_env(:phoenix, __MODULE__.DynamicPortStartedEndpoint, http: [port: 0], server: true)
defmodule DynamicPortStartedEndpoint do
use Phoenix.Endpoint, otp_app: :phoenix
end
DynamicPortStartedEndpoint.start_link()
assert %{port: port} = DynamicPortStartedEndpoint.url() |> URI.parse()
assert port > 1023
end

@tag :capture_log
test "uses dynamic port 0 if server is not started" do
Application.put_env(:phoenix, __MODULE__.DynamicPortEndpoint, http: [port: 0], server: false)
defmodule DynamicPortEndpoint do
use Phoenix.Endpoint, otp_app: :phoenix
end
DynamicPortEndpoint.start_link()
assert %{port: port} = DynamicPortEndpoint.url() |> URI.parse()
assert port == 0
end

@tag :capture_log
test "uses url configuration for static path" do
Application.put_env(:phoenix, __MODULE__.UrlEndpoint, url: [path: "/api"])
Expand Down