Cinderella-Man / hands-on-elixir-and-otp-cryptocurrency-trading-bot-source-code

Resources related to the "Hands-on Elixir & OTP: Cryptocurrency trading bot" book
https://elixircryptobot.com
75 stars 24 forks source link

In some cases, How to tell to the supervisor stop restarting the trader? #11

Closed juanpabloaj closed 3 years ago

juanpabloaj commented 3 years ago

@Cinderella-Man thanks, for your book, it has been really interesting and useful to understand more about Elixir.

I was not sure if it could be the best place to ask questions. If you think that this question is not related to the book please close the issue.

Sometimes when something was wrong when an order is created an error is returned.

{:error, {:binance_error, %{code: -1013, msg: "Filter failure: MIN_NOTIONAL"}}}

it doesn't match with

    {:ok, %Binance.OrderResponse{} = order} =
      @binance_client.order_limit_buy(symbol, quantity, price, "GTC")

and the trader is restarted. Great.

But maybe under some circumstances could be useful if the trader stops being restarted, for example, because you need to change some settings.

Does exist some way to tell to the supervisor: stop restarting the trader?

I was thinking about how to match some error cases

    new_state =
      case @binance_client.order_limit_buy(symbol, quantity, price, "GTC") do
        {:ok, %Binance.OrderResponse{} = order} ->
          new_state = %{state | buy_order: order}
          Naive.Leader.notify(:trader_state_updated, new_state)
          new_state

        {:error, {:error, {:binance_error, %{code: -1013}}}} ->
          Naive.Leader.notify(:critical_error, state)
          state
      end

but I am not sure about which could be a good way to tell to the supervisor stop restarting the trader.

Or maybe that is not the Elixir way to do things. What do you think?

Thanks in advice for your feedback.

The full log of those types of cases.

2021-06-20 17:32:45.718 [error] GenServer #PID<0.341.0> terminating
** (FunctionClauseError) no function clause matching in Binance.parse_order_response/1
    (binance 1.0.1) lib/binance.ex:505: Binance.parse_order_response({:error, {:binance_error, %{code: -1013, msg: "Filter failure: MIN_NOTIONAL"}}})
    (naive 0.1.0) lib/naive/trader.ex:77: Naive.Trader.handle_info/2
    (stdlib 3.15.1) gen_server.erl:695: :gen_server.try_dispatch/4
    (stdlib 3.15.1) gen_server.erl:771: :gen_server.handle_msg/6
    (stdlib 3.15.1) proc_lib.erl:226: :proc_lib.init_p_do_apply/3
Last message: %Streamer.Binance.TradeEvent{buyer_market_maker: false, buyer_order_id:, event_time: 1624210365370, event_type: "trade", price: "1.39800000", quantity: "10.86000000", seller_order_id:, symbol: "ADAUSDT", trade_id:, trade_time: 1624210365370}
State: %Naive.Trader.State{budget: #Decimal<10>, buy_down_interval: "0.002", buy_order: nil, id:, profit_interval: "0.0012", rebuy_interval: "0.0035", rebuy_notified: false, sell_order: nil, step_size: "0.01000000", symbol: "ADAUSDT", tick_size: "0.00010000"}

]}, {Naive.Trader, :handle_info, 2, [file: 'lib/naive/trader.ex', line: 77]}, {:gen_server, :try_dispatch, 4, [file: 'gen_server.erl', line: 695]},
{:gen_server, :handle_msg, 6, [file: 'gen_server.erl', line: 771]},
{:proc_lib, :init_p_do_apply, 3, [file: 'proc_lib.erl', line: 226]}]} of type Tuple. This protocol is implemented for the following type(s): Decimal, Version.Requirement, Time, DateTime, Integer, List, Version, URI, Atom, BitString, Date, Float, NaiveDateTime
    (elixir 1.12.1) lib/string/chars.ex:3: String.Chars.impl_for!/1
    (elixir 1.12.1) lib/string/chars.ex:22: String.Chars.to_string/1
    (naive 0.1.0) lib/naive/leader.ex:133: Naive.Leader.handle_info/2
    (stdlib 3.15.1) gen_server.erl:695: :gen_server.try_dispatch/4
    (stdlib 3.15.1) gen_server.erl:771: :gen_server.handle_msg/6
    (stdlib 3.15.1) proc_lib.erl:226: :proc_lib.init_p_do_apply/3
Last message: {:DOWN, #Reference<0.1217684777.3447455745.34513>, :process, #PID<0.341.0>, {:function_clause, [{Binance, :parse_order_response, [error: {:binance_error, %{code: -1013, msg: "Filter failure: MIN_NOTIONAL"}}],
[file: 'lib/binance.ex', line: 505]}, {Naive.Trader, :handle_info, 2, [file: 'lib/naive/trader.ex', line: 77]},
{:gen_server, :try_dispatch, 4, [file: 'gen_server.erl', line: 695]},
{:gen_server, :handle_msg, 6, [file: 'gen_server.erl', line: 771]},
{:proc_lib, :init_p_do_apply, 3, [file: 'proc_lib.erl', line: 226]}]}}
State: %Naive.Leader.State{settings: %{budget: 30, buy_down_interval: "0.002", chunks: 3, profit_interval: "0.0012", rebuy_interval: "0.0035", step_size: "0.01000000", symbol: "ADAUSDT", tick_size: "0.00010000"}, symbol: "ADAUSDT", traders:
[%Naive.Leader.TraderData{pid: #PID<0.341.0>, ref: #Reference<0.1217684777.3447455745.34513>,
state: %Naive.Trader.State{budget: #Decimal<10>, buy_down_interval: "0.002", buy_order: nil, id: 1624210365312, profit_interval: "0.0012", rebuy_interval: "0.0035", rebuy_notified: false, sell_order: nil, step_size: "0.01000000", symbol: "ADAUSDT", tick_size: "0.00010000"}}]}

Thanks again.

Cinderella-Man commented 3 years ago

Hi @juanpabloaj, thank you very much for your kind words - I thrilled to know that you like it :heart:

In regards to your question:

In normal circumstances when a "worker" process (like the Naive.Trader) is supervised by the Supervisor, there's a built-in system that will stop it from restarting the worker forever: https://hexdocs.pm/elixir/1.12/Supervisor.Spec.html#supervise/2-options (I'm talking about :max_restarts[defaults to 3] and :max_seconds[defaults to 5]).

This would be the first "Elixir way" of fixing this problem - it would quickly die 3 times and it wouldn't try to restart it.

but we are not using this mechanism as we disabled it by setting the Naive.Trader to restart: :temporary we are only relying on our Naive.Leader process to react to the :DOWN info message. At this moment we don't have any implementation that would count the number of restarts etc - so the Naive.Leader process will keep on restarting the "broken"(for example: because of configuration) Naive.Trader process.

One way to "fix" that would be to add the case as you pointed out and instead of calling the Naive.Leader to tell it to not restart the process. We could return {:stop, :your_special_error_not_restart, state} from the handle_info inside the Trader then we could write the:

def handle_info(
                {:DOWN, _ref, :process, trader_pid, :your_special_error_not_restart},
                %{traders: traders, symbol: symbol, settings: settings} = state
 )  do ....

inside the Naive.Leader

Inside the Naive.Leader's handler you would decide what you want to do with a specific error.


In case of normal supervision it would be enough to log a warning and return {:stop, :normal, state} but this would stop just a single process that suffered from error - if you would want to stop all of the ones assigned to the symbol you would need somehow terminate tree from above (as you suggested by using the Naive.Leader process).

I hope that helps,

PS. I would greatly appreciate it if we could recreate this inside GitHub Discussions that I just enabled for exact topics like this. If you could just copy across your question there and I will copy this one so other people could read it and possibly join the conversation or open other questions/topics. I would greatly appreciate that :+1: If you would have further questions - we can chat there if you don't mind.

Thank you once again for your feedback and contributions to make this book better :rocket: