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

Use monad for handling errors and general flow, also config file #1

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Use monad for handling errors and general flow, also config file #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
dfbed99
handle read error in read method
vikrambombhi Dec 4, 2018
b580cee
ignore elixir language server files
vikrambombhi Dec 4, 2018
ea682d4
config file with single upstream node address and ms before timeout f…
vikrambombhi Dec 5, 2018
85c0408
use timeout specified in config file when reading from upstream node
vikrambombhi Dec 5, 2018
c74eef2
cleaner functional flow for read_from_client |> forward_to_upstream |…
vikrambombhi Dec 5, 2018
75cda5f
use monad to handle errors, Thanks @JackyChiu
vikrambombhi Dec 5, 2018
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
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,5 @@ erl_crash.dump
# Ignore package tarball (built via "mix hex.build").
active_proxy-*.tar

# Elixir language server
.elixir_ls
4 changes: 4 additions & 0 deletions config/config.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,7 @@ use Mix.Config
# here (which is why it is important to import them last).
#
# import_config "#{Mix.env()}.exs"

config :active_proxy,
timeout: 100,
node1_address: "159.203.44.11"
Copy link
Member

Choose a reason for hiding this comment

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

This should be a list key instead of a indexed key lol

upstream_hosts: [
  "159.203.44.11"
]

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we talked about change address during runtime so I assumed named addresses would be best. Not sure if we still want to do that.

Copy link
Member

Choose a reason for hiding this comment

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

Did we decide that we didn't want dynamic changes to the upstream hosts?

Copy link
Member Author

Choose a reason for hiding this comment

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

I honestly don't remember.

Copy link
Member

@JackyChiu JackyChiu Dec 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Proposal for actual storage: store active/inactive data in a persistent KV store instead because it's more flexible. Imagine the case where we decide to have dynamic upstream being added and the proxy restarts. That state is lost and the proxy goes back to relying on the static config. Whereas with the KV store we'd have that state saved. Obviously, this is out of scope for this PR but we should consider this for future active/inactive storage.

For this PR I say you just make a decision of a list or a named active/inactive hosts.

60 changes: 18 additions & 42 deletions lib/active_proxy/proxy.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,16 @@ defmodule ActiveProxy.Proxy do
end

defp start_serve_process(socket) do
timeout = Application.get_env(:active_proxy, :timeout)
application_address = Application.get_env(:active_proxy, :node1_address)
Copy link
Member

Choose a reason for hiding this comment

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

Can you look into if we can have these as constants on the module? and for the module to eval them on startup?

Otherwise, an init on the module should accept a config object and they should be set as constants there

Logger.info("Forwarding to application at #{application_address} with timout of #{timeout}ms")
Copy link
Member

Choose a reason for hiding this comment

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

Move this log to where that goes as well ^


{:ok, pid} =
Task.Supervisor.start_child(ActiveProxy.TaskSupervisor, fn ->
# TODO: make the host configurable
{:ok, upstream_socket} =
:gen_tcp.connect(
'159.203.44.11',
to_charlist(application_address),
4000,
[
:binary,
Expand All @@ -38,54 +42,26 @@ defmodule ActiveProxy.Proxy do
1000
)

serve(socket, upstream_socket)
serve(socket, upstream_socket, timeout)
end)

:ok = :gen_tcp.controlling_process(socket, pid)
end

defp serve(socket, upstream_socket) do
timeout = 10
{ok, packet} = read(socket)

if {ok, packet} == {:error, :closed} do
# If reading from socket closed exit serve loop
# Should probably log error
#
#
# Also handle other types of errors other than :closed
defp serve(client_socket, upstream_socket, timeout) do
with {:ok, packet} <- :gen_tcp.recv(client_socket, 0),
:ok <- :gen_tcp.send(upstream_socket, packet),
{:ok, packet} <- :gen_tcp.recv(upstream_socket, 0, timeout),
:ok <- :gen_tcp.send(client_socket, packet) do
serve(client_socket, upstream_socket, timeout)
Copy link
Member

Choose a reason for hiding this comment

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

Print this function out and hang it on a wall 😍

else
# TODO: Handle failure to write to application
write(upstream_socket, packet)

case read(upstream_socket, timeout) do
{:ok, payload} ->
write(socket, payload)

{:error, _} ->
# shutdown writes to signal that no more data is to be sent and wait for the read side of the socket to be closed
:gen_tcp.shutdown(socket, :write)
end

serve(socket, upstream_socket)
end
end

defp read(socket) do
:gen_tcp.recv(socket, 0)
end
{:error, :closed} ->
nil

defp read(socket, timeout) do
case :gen_tcp.recv(socket, 0, timeout) do
{:ok, packet} ->
{:ok, packet}

{:error, timeout} ->
{:error, timeout}
# TODO: Handle errors
# Consider doing a failover
_ ->
nil
end
end

defp write(socket, packet) do
:gen_tcp.send(socket, packet)
end
end