server/multi: remove server-side message caching#699
Conversation
buck54321
force-pushed
the
no-msg-cache
branch
from
f3ece8c to
79ebb9a
Compare
| // Swapper.liveAckers | ||
| LiveAckers map[uint64]*msgAckers | ||
| // Swapper.liveWaiters | ||
| LiveWaiters map[waiterKey]*handlerArgs |
There was a problem hiding this comment.
We can probably get rid of LiveWaiters eventually too, but leaving for now.
There was a problem hiding this comment.
If the client resubmits their init or redeem on reconnect?
There was a problem hiding this comment.
If we have a live coin waiter, we haven't acked the client's request, so it'll time out and try again anyways, I think.
There was a problem hiding this comment.
Makes sense, client will reconnect, check match_status, and if needed do their 'init' or 'redeem' again. That the plan?
buck54321
force-pushed
the
no-msg-cache
branch
from
79ebb9a to
0737220
Compare
chappjc
left a comment
chappjc
left a comment
There was a problem hiding this comment.
Diff looks great, quite simple actually. Despite ~1k loc of my code going to the scrap heap, I'm very happy about this change and the direction it's going.
Next steps will presumably be to revise Swapper bits (e.g. step to stop requiring ack sigs) and allowing for repeated init and redeem request from clients, then the accompanying {order,match}_status changes so clients can get back in the swap at any time as long as their match isn't revoked.
Oh, this has the |
itswisdomagain
left a comment
itswisdomagain
left a comment
There was a problem hiding this comment.
First pass. I think the message caching feature is completely retired (just a DefaultConnectTimeout constant that I think is now obsolete too). I still see that the server is set up to wait for and process acks for match-related requests. Any plans to completely do away with request acking?
| // Disconnect and remove known connections. We are creating a new one, | ||
| // but persist the response handlers. |
There was a problem hiding this comment.
We might not create a new connection for this user if some error occurs downline before the call to auth.addClient(client). Is this intentional?
Oh I see that this was moved from below, I understand the reason for moving the account disabling check up here but is it necessary to remove any existing connection earlier as well?
| log.Infof("Timeout waiting for contract 'audit' request acknowledgement from user %v (%s) for match %v", | ||
| ack.user, makerTaker(ack.isMaker), matchID) |
There was a problem hiding this comment.
Looks like we'll be getting a lot of these since clients are no longer required to send acks. Do you plan to subsequently retire this wait for ack feature for match-related requests? Basically, just send the request, if it doesn't get to the user, the match state can be recovered on connect.
There was a problem hiding this comment.
I was thinking this was something we'd want in the logs. I do suspect that we'll see a few of these, but hopefully not too many. Are you thinking Debugf?
I'm certainly open to it. It's becoming less and less relevant. |
3 participants
Removes all server message caching, as well as the requirement to acknowledge
match,audit, andredemptionrequests before progressing through match negotiation. This is intended to be the base upon which amatch_status/order_statusconflict resolution system is built. #677 is theorder_statuspart.