Gone PIping Mad -- Is this ok, could this be better designed?

Phoenix Forum Questions / Help
1

I’m in the process of building an availability monitor. I enqueue a request/response using a worker, which then performs code found in my models. This generally means performing the actual request, checking matches on certain values of the response (status code, body, headers), and potentially sending an alert based on a policy.

I’m only half a week into designing it, and have already started to abuse those beautiful pipes, and wondering how best to approach this.

Here is a very simplified flow. Below, the request_response is my model struct, whereas the poison is a HTTPoison struct. I’ve just wrote this out from scratch and the naming is not the same in my codebas – this is just for brevity.

defmodule Check do
  def enqueue_check(check_id) do
    MyWorker.enqueue(check_id)
  end

  def perform(check) do
    RequestResponse.new_from_check(check)
    |> RequestResponse.perform
  end
end

defmodule MyWorker do
  def perform(check_id) do
    # find check and run Check.perform
  end
end

defmodule RequestResponse do
  def perform(request_response) do
    make_request(request_response) |> process_response
  end

  def make_request(request_response) do
    # HTTPoison request, returns {:ok/error, response}

    {request_response, poison}
  end

  def process_response({request_response, poison}) do
     {request_response, poison} = request_response
     |> run_checks
     |> run_alerts

     Repo.insert!(request_response)
   end

   def run_checks({request_response, poison}) do
     request_response
     |> check_status_match
     |> check_etc
     |> check_etc
     |> check_etc
     |> pass_or_fail
   end
end

I’m wondering if this reliance on piping is a good idea or is going to lead to trouble in the future? Piped functions leading to other piped functions? To be fair, I find it simple enough to follow, but I did build it :confused:

I should also note, this is mostly in my models, but I do plan to encapsulate these different responsibilities in separate modules.

One of the big issues I’ve had is that some of the functions I’m piping might only require a single argument (the request_response), and some require a couple (a HTTPoison struct). But because of my perhaps abuse of pipes, I’m having to pass and return both of these arguments in each function.

Another point I would like advice on is the process_response function. I am retrieving the modified request_response after piping it through my run_checks, and then inserting it using a repo. Does this seem sane? Any glaring holes that might be worth considering? I’ve not even added error messaging, so want to be sure I’m starting on the right foot.

Would appreciate any input.

2

I’ve not even added error messaging, so want to be sure I’m starting on the right foot.

Might be time to look at with/1.

Edit: in some situations the Exceptional library can be helpful.

3

Ahh! I did run across that in my travels. Thanks.

I can put expressions between clauses! With great power haha

4

I usually just use throw for something like this.

Actually, I’ve used throw only in three projects, and two of those had to do with parsing binary data. I used throw to get out of nested function calls like this


  # for decoding ws frames

  def decode_frame(frame) do
    try do
      decode_fin(frame)
    catch
      :throw, reason ->
        {:error, reason}
    end
  end

  defp decode_fin(<<fin::1, rest::bits>>) do
    decode_rsv(rest, fin === 1)
  end

  defp decode_rsv(<<0::3, rest::bits>>, fin?) do
    decode_opcode(rest, fin?)
  end
  defp decode_rsv(<<rsv1::1, rsv2::1, rsv3::1, _rest::bits>>, _fin?) do
    throw({:unknown_extension, {rsv1, rsv2, rsv3}})
  end

  alias __MODULE__.Frame
  opcodes = %{
    quote(do: [@continuation_frame::4]) => Frame.Continuation,
    quote(do: [@text_frame::4]) => Frame.Text,
    quote(do: [@binary_frame::4]) => Frame.Binary,
    quote(do: [@connection_close_frame::4]) => Frame.Close,
    quote(do: [@ping_frame::4]) => Frame.Ping,
    quote(do: [@pong_frame::4]) => Frame.Pong
  }
  for {k, struct} <- opcodes do
    defp decode_opcode(<<unquote_splicing(k), rest::bits>>, fin?) do
      decode_mask(rest, fin?, unquote(struct))
    end
  end
  defp decode_opcode(<<opcode::4, _rest::bits>>, _fin?) do
    throw({:unknown_opcode, opcode})
  end

  # etc

This approach allowed me to keep a single match context and was very performant at the time (twice as fast as cowlib).

@BitGonzo Doesn’t really apply to your case though … But with your code it would look something like this

   @spec run_checks(HTTPoison.Response.t, ???) :: {:ok, any} | {:error, term}
   def run_checks(request_response, poison) do
     request_response
     |> check_status_match()
     |> check_etc(poison) # you can pass poison or any other argument
     |> check_etc(poison) # only when needed
     |> check_etc()
     |> pass_or_fail()
   catch
     :throw, error -> {:error, error}
   else
     resp -> {:ok, resp}
   end

   defp check_etc(%{status_code: 200, ...}) do
     # do stuff
   end
   defp check_etc(%{status_code: status_code}) do
     throw({:status_code_fail, status_code})
   end
1 Like
5

Pipes are one of the most beautiful things about elixir. They are there to be used! But you are right to be worried about painting yourself into a corner.

On the process_response function, I’m not sure why you are passing in {request_response, poison} then on the next line have {request_response, poison} = request_response? Why not just pass in request_response as the first argument, and poison as the second? Also if the poison argument is the same between each function, you can just pass that in as a second argument explicitly. With my limited knowledge of your code, I would probably do something more like this:

def process_response(request_response, poison) do
    request_response
    |> run_checks(poison)
    |> run_alerts(poison)
    |> Repo.insert!
end

Of course, this would mean changing your other functions to only return the request_response struct. This is all assuming your poison argument is consistent through the process? Hard to tell without seeing the run_checks or run_alerts.

Also, why does your run_checks function accept {request_response, poison} when poison seems unused?

6

Thanks!

That suggestion is exactly the direction I started to move towards when I looked into how argument passing works with pipes. I do apologise for the initial code I wrote up there – it isn’t match for match and has a typo. I actually have something like:

{request_response, poison}
|> pipe
|> pipe
|> pipe

I don’t need poison for the run_alerts, so I would assume I could do something like?

request_response
|> run_checks(poison)
|> run_alerts
|> Repo.insert!
7

Appreciate the rundown. You’ve given me a few things to think about there.

8

In the context of this topic it may be beneficial to point out that throw is different from raise (especially as an uncaught thrown value will still kill a process - it’s meant to be used as a “Beam me up Scotty!” function return - hence the description as a “non-local return”).

Caveat: try disables last call optimization:

Note: It is important to know that the protected part of an exception can’t be tail recursive. The VM must always keep a reference there in case there’s an exception popping up.

That being said throw can be useful if it is used with discretion but it’s use is largely restricted to your own code.

I think Exceptional’s use case is more directed towards dealing with divergent values that may be returned by your third-party dependencies - especially when there are possible results that are rare, unusual, and inconvenient but not exceptional enough to let your process crash over it.

1 Like
9

Yes! This is a perfect reason for why you would want to explicitly pass all arguments except for the first. I think you’re on the right track.

1 Like
10

Caveat: try disables last call optimization:

Yeah, that’s probably the worst thing about it. Until recently some usages of with disabled it as well. But I don’t think I was relying on last call optimization in any of the three projects that I used throw in (no function was calling itself recursively).

It also shouldn’t affect performance of

   @spec run_checks(HTTPoison.Response.t, ???) :: {:ok, any} | {:error, term}
   def run_checks(request_response, poison) do
     request_response
     |> check_status_match()
     |> check_etc(poison) # you can pass poison or any other argument
     |> check_etc(poison) # only when needed
     |> check_etc()
     |> pass_or_fail()
   catch
     :throw, error -> {:error, error}
   else
     resp -> {:ok, resp}
   end

in any way.

The problem with raise as I’ve recently found out, is that any unhandled usage of it with plug/phoenix makes cowboy 1.0 drop connections (I used to think that Plug.Exception and Plug.ErrorHandler would catch raised exceptions and leave connections intact, but they reraise). Not particularly relevant here though.

11

Sounds like you want monadic pipelines! ^.^
(They really should be added to elixir…)

But yeah I do similar things as you in a lot of cases. :slight_smile:
I find that pattern fine in dedicated pipelines like that. In other cases where reusability is more important I’d abstain though.

But overall with is more useful. :slight_smile:

1 Like