feat(lp_bot): cancel order on stopping mm2 context

[Milerius]

No description provided.

[smk762]

• Upon starting mm2, automatically cancel all orders from prior session. To allow for cases where user may not want this to happen, a boolean MM2.json config parameter could define this behavior.
• When running stop_simple_market_maker_bot, cancel_all_orders (or cancel_order for list of UUIDs / pairs initiated by makerbot) could be initiated to overcome delay while awaiting completion of current loop.
• When running stop, cancel_all_orders (or cancel_order for list of UUIDs / pairs initiated by makerbot) could be initiated cancel active orders to avoid them appearing on next load.

Which method of mitigation does this implement?

[Milerius]

• Upon starting mm2, automatically cancel all orders from prior session. To allow for cases where user may not want this to happen, a boolean MM2.json config parameter could define this behavior.
• When running stop_simple_market_maker_bot, cancel_all_orders (or cancel_order for list of UUIDs / pairs initiated by makerbot) could be initiated to overcome delay while awaiting completion of current loop.
• When running stop, cancel_all_orders (or cancel_order for list of UUIDs / pairs initiated by makerbot) could be initiated cancel active orders to avoid them appearing on next load.

Which method of mitigation does this implement?

The last one

[smk762]

Confirm orders are cancelled if stopping mm2 gracefully.
I did encounter some delays on myorders response at times tho.

[sergeyboyko0791]

Thanks for the fix!
Could you please consider my comments?

[sergeyboyko0791]

What do you think about adding a static function dispatch_lp_event that will look like:

pub fn dispatch_lp_event(ctx: MmArc, event: LpEvents) -> Result<(), MmError<DispatcherError>> {
  let dispatcher_ctx = DispatcherContext::from_ctx(&ctx).or_mm_error(|| Error::MmCtxNotAvailable)?;
  dispatcher_ctx.dispatcher.read().await.dispatch_async(ctx, event).await;
  Ok()
}

I think we can add it because we're supposed to produce events from a lot different places in the future, and it will reduce amount of code.

[Milerius]

Sounds like a good idea, will do!

[sergeyboyko0791]

Can you please replace dispatcher_ctx.dispatcher.read().await.dispatch_async(ctx.clone(), event.into()).await; with dispatch_lp_event().await?

[sergeyboyko0791]

We call the MmCtx::stop method from another one place: on the extern function mm2_stop.

I suggest using MmCtx::on_stop to dispatch StopCtxEvent like it does spawn_rpc.

[Milerius]

Is the function MmCtx::on_stop is called before the member of the structure are destroyed? In this case that's probably a good idea!

[sergeyboyko0791]

Also I'd say that I think using RwLock is a good decision for the dispatcher.
I tried to imagine a case when it's worth using AsyncMutex instead, and what I realized:

What if an event listener adds another listener when dispatches an event?

async fn process_event_async(&self, ctx: MmArc, event: AnyEvent) {
  let mut dispatcher = dispatcher_ctx.dispatcher.write().await;
  dispatcher.add_listener(simple_market_maker_bot_ctx.clone());
}

But it's more likely to face the deadlock if we use AsyncMutex because event listener may produce an event while dispatching another already.

I think it's out of this PR because I'm afraid that there are other places where we may face the deadlock too.

[Milerius]

Also I'd say that I think using RwLock is a good decision for the dispatcher. I tried to imagine a case when it's worth using AsyncMutex instead, and what I realized:

What if an event listener adds another listener when dispatches an event?

async fn process_event_async(&self, ctx: MmArc, event: AnyEvent) {
  let mut dispatcher = dispatcher_ctx.dispatcher.write().await;
  dispatcher.add_listener(simple_market_maker_bot_ctx.clone());
}

But it's more likely to face the deadlock if we use AsyncMutex because event listener may produce an event while dispatching another already.

I think it's out of this PR because I'm afraid that there are other places where we may face the deadlock too.

It's exactly the reason that I decided to use RwLock also I think there is some other place we should use it, for example for my_orders, take the following case:

• The bot is launched and require my_orders to iterate and update order -> lock the mutex, clone the orders
• The GUI call my_orders RPC to refresh orders in the GUI -> wait for the lock to be released in order to lock it again

Here I think there is a possible long lock (not a deadlock because the lock will be released). Allowing multiple readers as the same time may be a solution for this kind of use case too.

My advice to avoid deadlock when using the dispatcher:

Add the events that you would like to listen too at the creation of the object / context. Remove them when you shutdown gracefully your app or your context.

[Milerius]

Summary of the last fixes:

• Implement dispatch_lp_event
• Dispatch the stopping event before calling ctx.stop() to ensure the possibility to use the ctx in the listeners
• Use block_on in the mm2_stop native function when dispatching the event in order to be able to return the status_code afterwards (using spawn will not allow to return anything in the function in this case) - should be ok in this function since it's the stopping of the application

[sergeyboyko0791]

Only one thing

[sergeyboyko0791]

Just one question: are we planning to add fields to the StopCtxEvent structure someday? If not, I'd suggest making it as pub struct StopCtxEvent; to simplify it an instance creation: StopCtxEvent.into().

[sergeyboyko0791]

Can you please replace dispatcher_ctx.dispatcher.read().await.dispatch_async(ctx.clone(), event.into()).await; with dispatch_lp_event().await?

[sergeyboyko0791]

Thanks for the changes! Approve!

[sergeyboyko0791]

Use block_on in the mm2_stop native function when dispatching the event in order to be able to return the status_code afterwards (using spawn will not allow to return anything in the function in this case) - should be ok in this function since it's the stopping of the application

We decided not to use block_on on the mm2_stop function call since it may lead to the GUI thread blocking. It's enough to check if the mm2 instance is stopping already and spawn async future.