-
-
Notifications
You must be signed in to change notification settings - Fork 560
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
feat(elixir): add unix domain sockets transport and getting-started example #4220
base: develop
Are you sure you want to change the base?
Conversation
b413f2e
to
aeace20
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi. Thanks for contributing!
The architecture makes sense.
Left some comments. Mostly related to the address type implementation.
We're currently working to phase out and eventually deprecate this feature in favour of connection workers. This would simplify transport implementation and examples.
The way it should work is that you would create a connection worker like:
{:ok, client} = UDS.Client.create(uds_address)
And then send messages to this client address instead of the UDAddress:
Ockam.Router.route(%Ockam.Message{onward_route: [client, "echo"], payload: "HI"})
See Ockam.Transport.TCP.Client
Also added some notes about connection management.
Let us know if you have any questions about this approach or how to make ockam workers out of gen servers.
|
||
# Connect to a secure channel listener and perform a handshake. | ||
alias Ockam.Transport.UDSAddress | ||
r = [UDSAddress.new("/tmp/sock1"), "h1", UDSAddress.new("/tmp/sock2"), "secure_channel_listener"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're trying to steer away from using implicit connections expressed in specific address types and towards explicit connection workers.
I think we can simplify example by first creating a connection worker like {:ok, client} = UDS.Client.create("/tmp/sock1")
and then sending messages through it.
That removes support for multi-hop thing and simplifies the example as it would only have 2 nodes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @hairyhum, this is great feedback! I went back and did some more reading and implemented an Ockam.Transport.UDS.Client
and used it in a Routing example, similar to how you specified it in your main comment. I still have to clean up the get_started examples and the UDSAddress.
@@ -0,0 +1,16 @@ | |||
defmodule Ockam.Transport.UDSAddress do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in other comment, we're trying to steer away from specific address types, so let's not add new ones.
Supervisor.init(children, strategy: :rest_for_one) | ||
end | ||
|
||
def handle_transport_message(message) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't use a special transport type, this becomes unnecessary.
pick_destination_and_set_onward_route(message), | ||
{:ok, encoded_message} <- Wire.encode(message), | ||
{:ok, socket} <- | ||
:gen_tcp.connect({:local, destination}, 0, [:binary, active: true, reuseaddr: true]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For connecting to a remote node, if we do that in the message handler, what happens with the connection?
Should we have some "client" worker to manage the connection in that case?
end | ||
|
||
@impl true | ||
def handle_info({:tcp, socket, data}, %__MODULE__{socket: socket} = state) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This module currently only supports TCP->Ockam messages.
How do we reply to those messages?
In other transport handler workers we were tracing the address of the worker in the message return route (see Ockam.Transport.TCP.Handler
around line 70).
I see this is not yet a worker, this comment is to keep in mind when you make it a worker.
@@ -0,0 +1,23 @@ | |||
defmodule Ockam.Transport.UDS.ConnectionSupervisor do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this approach.
def run(path) do | ||
case :gen_tcp.listen(0, [ | ||
:binary, | ||
{:active, true}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we listen in the active mode, does it mean that it's possible to get connection messages on the wrong process if there is some race condition between accept and change of controlling process?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, perhaps one way to deal with this would be to start with passive or active once and switch it once the Connection child is up.
alias Ockam.Transport.UDSAddress | ||
|
||
# Create a Echoer type worker at address "echoer". | ||
{:ok, _echoer} = Echoer.create(address: UDSAddress.new("/tmp/sock2")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand it's WIP, but the address
option we pass to the worker would be this worker address.
Passing transport addresses in this option is not going to work because transport addresses are handled differently. And I believe that wasn't intended behaviour in the first place.
{:ok, hop} = Ockam.Transport.UDS.Client.create(path: "/tmp/client.sock") | ||
|
||
Ockam.Router.route(%{ | ||
onward_route: [server_host_address, hop, "echoer"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This route sends a message through server uds transport and then a "client.sock" uds transport and then to "echo".
Although that should probably work, it can be a bit confusing.
I think hop
and "client.sock" can be removed to make a simpler example.
Hi @tzumby I can see it has been a while since this PR has been opened. Is there anything we can do to help you finish it? |
Hey @etorreborre, thanks for messaging, I would love to push this over the finish line, but I'm absolutely swamped with work. If this is blocking anything maybe it makes sense that I either close it or have someone else co-author this with me ? |
@tzumby no worries this is not on the critical path at the moment but we will see if we can maybe find someone to help you out with this. |
Current Behavior
Currently there is no implementation of Unix Domain Socket Transports for Elixir implementation.
Proposed Changes
This PR is still Work in Progress
Checks