-
Notifications
You must be signed in to change notification settings - Fork 94
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
Use parking lot mutex for maker orders #1163
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! It's a first review iteration. Could you please also kickstart a testing session with @Milerius?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! I had to puzzle a bit to imagine a case where a deadlock could occur.
Just one comment
mm2src/lp_ordermatch.rs
Outdated
.ok(); | ||
let maybe_order_mutex = ordermatch_ctx.my_maker_orders.lock().remove(&uuid); | ||
if let Some(order_mutex) = maybe_order_mutex { | ||
let order = order_mutex.lock().await; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be wrong, but I think it's better to remove the order from OrdermatchContext::my_maker_orders
in the loop above when the order AsyncMutex<MakerOrder>
mutex is locked already, how you've done here.
It's not a recommendation, but a topic for discussion :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you, I see now that the orders were canceled in another loop because we were iterating over the hashmap we needed to remove the order from but now since we iterate over a clone of the map it should be done in the above loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 @sergeyboyko0791 Please check that your remarks are resolved. You can merge it afterward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
fixes #1152