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.

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

mm: bots should survive server disconnects #3097

Open
norwnd opened this issue Nov 24, 2024 · 2 comments
Open

mm: bots should survive server disconnects #3097

norwnd opened this issue Nov 24, 2024 · 2 comments

Comments

@norwnd
Copy link
Contributor

norwnd commented Nov 24, 2024

I've noticed MM bot occasionally would just stop working without any errors in logs (I think this also was reported on Matrix a while ago).

I've been digging through basic MM bot code lately and it seems there is no handling for server disconnect, the code involved is quite complex to tell what would happen exactly - but from cursory overview it seems 2 outcomes are possible:

  1. once there are no update coming on the bookFeed bot will be doing nothing (won't be adjusting existing orders or placing new ones)
  2. and once the bookFeed channel actually closes the involved for loop will hit nil pointer dereference due to nil value returned immediately from the channel

^ I didn't observe nil pointer happen in practice (case 2) likely because no explicit bookie.closeFeeds() call was issued (either bookie is intentionally "kept alive" perhaps to serve other users like UI, or server connection just broke at some low level and didn't bother to notify higher levels about it).

Seems with the current implementation we are experiencing case 1 - which means botLoop doesn't really know when it needs to ditch the current bookFeed he is consuming and create a new one with m.core.SyncBook (unless we can fix the propagation of feed closing from lower to higher levels, a work-around could be to timeout "dead" feed and re-subscribe to a new one).

Note, even if bookie is intentionally "kept alive" though another consumer (of another feed) in the scenarios described in this issue it probably shouldn't be because bookie is just a multiplexer(either all of his feeds break/stop or all of them must be functional), hence the proper solution (as opposed to a workaround I've suggested above) would probably look like this:

  • fix connection termination error propagation such that we convert "case 1" into "case 2" (consistently start hitting those nil pointer dereferences instead of just hanging waiting for new updates that will never come)
  • fix "case 2" by additionally checking whether bookFeed has been closed (and re-creating new feed when necessary)
@norwnd norwnd changed the title mm: bots should survive server disconnect mm: bots should survive server disconnects Nov 24, 2024
@martonp
Copy link
Contributor

martonp commented Dec 1, 2024

If the server disconnects and reconnects, the old feed should still be working. Are you running the latest version? If you go to the orders report popup (click the button next to "All Buy Orders Placed Successfully"), does the epoch number get updated?
Screenshot 2024-12-01 at 1 45 39 PM

@norwnd
Copy link
Contributor Author

norwnd commented Dec 1, 2024

Are you running the latest version? If you go to the orders report popup (click the button next to "All Buy Orders Placed Successfully"), does the epoch number get updated?

I'll take a better/deeper look at this a bit later (busy with other stuff at the moment) and let you know what I found out, perhaps what I saw was indeed caused by something else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants