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

Unable to specify gun tcp_opts field when using gun v2 #176

Closed
ljmarks opened this issue Sep 29, 2020 · 2 comments · Fixed by #236
Closed

Unable to specify gun tcp_opts field when using gun v2 #176

ljmarks opened this issue Sep 29, 2020 · 2 comments · Fixed by #236
Labels

Comments

@ljmarks
Copy link

ljmarks commented Sep 29, 2020

Describe the bug
When passing options for gun via the :adapter_opts key in the 3rd argument to the function GRPC.Stub.connect/3, if you specify the tcp_opts field (which should be of type [gen_tcp:connect_option()], see: https://ninenines.eu/docs/en/gun/2.0/manual/gun/) the field is always replaced with the value of @default_transport_opts.

To Reproduce
Ensure you are using gun v2 (you should be unless you specify a different version with override: true in your mix.exs) and make sure the gun application is loaded. Add some logging to GRPC.Stub.connect_insecurely/2 or trace :gun.open/3 using e.g. recon_trace:calls, and then call GRPC.Stub.connect(my_addr, my_port, [adapter_opts: %{tcp_opts: my_tcp_opts}]) where my_tcp_opts is a proplist containing some options you want to give to gun's tcp_opts field.

Expected behavior
The options passed to gun:open/3 should include the values you provided for the tcp_opts field. Instead you will find that you can only ever get the default tcp_opts value provided by the module attribute @default_transport_opts in the GRPC.Adapter.Gun module

Logs

Interactive Elixir (1.10.1) - press Ctrl+C to exit (type h() ENTER for help)
iex(1)> :code.ensure_loaded(:gun)
{:module, :gun}
iex(2)> :recon_trace.calls({:gun, :open, :_}, 1)
2
iex(3)> grpc_opts = [adapter_opts: %{tcp_opts: [port: 6969, nodelay: true]}]
[adapter_opts: %{tcp_opts: [port: 6969, nodelay: true]}]
iex(4)> GRPC.Stub.connect("localhost", 50051, grpc_opts)
14:26:13.021314 <0.250.0> gun:open("localhost", 50051, #{http2_opts=>#{settings_timeout=>infinity}, protocols=>[http2], retry=>100, retry_fun=>fun 'Elixir.GRPC.Adapter.Gun':retry_fun/2, tcp_opts=>[{nodelay,true}], transport=>tcp})
Recon tracer rate limit tripped.

Versions:

  • OS: NixOS 20.03
  • Erlang: Erlang/OTP 22 [erts-10.5.5]
  • Elixir: 1.10.1
  • mix.lock(grpc, gun, cowboy, cowlib):
"grpc": {:git, "https://github.com/elixir-grpc/grpc.git", "9b8bfce9ba529805ac0bd6d183bf9563a9f24b0c", []}
"cowboy": {:hex, :cowboy, "2.7.0", "91ed100138a764355f43316b1d23d7ff6bdb0de4ea618cb5d8677c93a7a2f115", [:rebar3], [{:cowlib, "~> 2.8.0", [hex: :cowlib, repo: "hexpm", optional: false]}, {:ranch, "~> 1.7.1", [hex: :ranch, repo: "hexpm", optional: false]}], "hexpm", "04fd8c6a39edc6aaa9c26123009200fc61f92a3a94f3178c527b70b767c6e605"},
 "cowlib": {:hex, :cowlib, "2.9.1", "61a6c7c50cf07fdd24b2f45b89500bb93b6686579b069a89f88cb211e1125c78", [:rebar3], [], "hexpm", "e4175dc240a70d996156160891e1c62238ede1729e45740bdd38064dad476170"},
 "gun": {:hex, :grpc_gun, "2.0.0", "f99678a2ab975e74372a756c86ec30a8384d3ac8a8b86c7ed6243ef4e61d2729", [:rebar3], [{:cowlib, "~> 2.8.0", [hex: :cowlib, repo: "hexpm", optional: false]}], "hexpm", "03dbbca1a9c604a0267a40ea1d69986225091acb822de0b2dbea21d5815e410b"}

Additional context
The problem is that on lines 27 and 45 of GRPC.Adapter.Gun, the call to Map.merge/2 always replaces the value of tcp_opts in the first argument (which is the value supplied by the user) with the value in the second argument (the default value).

Note that things work as expected if the user is using gun v1 and instead uses the key transport_opts. Do we really need the checks to the version of gun here? I think it makes sense to just pass through whichever options the user supplies, since gun itself will validate the options when gun:open is called anyway.

@ljmarks ljmarks added the bug label Sep 29, 2020
@polvalente
Copy link
Contributor

We can resolve this by doing a merge on the options in a way that allows for a per-call override

@polvalente
Copy link
Contributor

The related PR above seems to resolve the issue. The options still need to be passed through :transport_opts regardless if they you're using TCP or TLS, but other than that it seems to be working :)

Could you validate this?

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

Successfully merging a pull request may close this issue.

2 participants