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.

Sign up for GitHub

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

Jump to bottom

[r2r] Avoid deadlock on DuplexMutex #1453

Merged
merged 3 commits into from
Sep 2, 2022
Merged

[r2r] Avoid deadlock on DuplexMutex #1453

merged 3 commits into from
Sep 2, 2022

Conversation

sergeyboyko0791
Copy link

  • Replace DuplexMutex with parking_lot::Mutex as it's more reliable
  • Refactor log.rs to avoid locking multiple mutexes at the same time
  • Replace Relaxed with SeqCst ordering.

@sergeyboyko0791
Avoid deadlock on DuplexMutex
fb20958 
* Replace `DuplexMutex` with `parking_lot::Mutex` as it's more reliable
* Refactor `log.rs` to avoid locking multiple mutexes at the same time
* Replace `Relaxed` with `SeqCst`
@sergeyboyko0791
Copy link
Author

Please note that it was difficult to figure out the relationship between different atomic variables, so I used SeqCst ordering at the expense of performance.

@sergeyboyko0791 sergeyboyko0791 self-assigned this Aug 27, 2022
@sergeyboyko0791 sergeyboyko0791 linked an issue Aug 27, 2022 that may be closed by this pull request
mm2 crashes in log.rs #1451
Closed
onur-ozkan
onur-ozkan reviewed Aug 29, 2022
View reviewed changes
Copy link
Member

@onur-ozkan onur-ozkan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good that we no longer need duplex mutex implementation. Great work!

But the log module implementation seems so harmful. I think we can optimize this much much better. Should we do this in this PR or create an issue as another task?

@sergeyboyko0791
Copy link
Author

@ozkanonur that would be awesome.

I think we can optimize this much much better

It's not a trivial task, I already tried to refactor it before, but I just got gray hair 😁. Let's postpone the refactoring

Should we do this in this PR or create an issue as another task?

@onur-ozkan onur-ozkan mentioned this pull request Aug 29, 2022
Open
onur-ozkan
onur-ozkan previously approved these changes Aug 29, 2022
View reviewed changes
Copy link
Member

@onur-ozkan onur-ozkan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🔥

laruh
laruh previously approved these changes Aug 29, 2022
View reviewed changes
Copy link
Member

@laruh laruh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

borngraced
borngraced previously approved these changes Aug 29, 2022
View reviewed changes
Copy link
Member

@borngraced borngraced left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! This looks great.

@borngraced
Copy link
Member

Please note that it was difficult to figure out the relationship between different atomic variables, so I used SeqCst ordering at the expense of performance.

@sergeyboyko0791 this should help https://www.youtube.com/watch?v=rMGWeSjctlY

shamardy
shamardy previously approved these changes Aug 29, 2022
View reviewed changes
Copy link
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work 🔥 ! Only some non-blocker comments :)

mm2src/common/log.rs Outdated Show resolved Hide resolved
mm2src/common/log.rs Show resolved Hide resolved
mm2src/common/log.rs Outdated Show resolved Hide resolved
mm2src/common/log.rs Show resolved Hide resolved
mm2src/common/log.rs Show resolved Hide resolved
mm2src/common/log.rs Show resolved Hide resolved
@sergeyboyko0791 sergeyboyko0791 changed the title Avoid deadlock on DuplexMutex [wip] Avoid deadlock on DuplexMutex Aug 30, 2022
@sergeyboyko0791
Fix PR issues
946cfaa 
* Import `parking_lot::Mutex` as `PaMutex`
@sergeyboyko0791 sergeyboyko0791 changed the title [wip] Avoid deadlock on DuplexMutex [r2r] Avoid deadlock on DuplexMutex Aug 30, 2022
@sergeyboyko0791
Copy link
Author

@cipig could you please run a node with these changes to test if the problem reproduces?

shamardy
shamardy approved these changes Aug 30, 2022
View reviewed changes
Copy link
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-Approve

artemii235
artemii235 approved these changes Sep 1, 2022
View reviewed changes
Copy link
Member

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

onur-ozkan
onur-ozkan approved these changes Sep 1, 2022
View reviewed changes
Copy link
Member

@onur-ozkan onur-ozkan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥

@cipig
Copy link
Member

cipig commented Sep 1, 2022

@cipig could you please run a node with these changes to test if the problem reproduces?

I updated all my 6 nodes to this version. Will try to reproduce. I guess many parallel swaps with same coin (so many swaps finish at the same time, when a block arrives) was triggering this.

cipig
cipig approved these changes Sep 1, 2022
View reviewed changes
Copy link
Member

@cipig cipig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

started 2000 swaps like this (same that did crash the node last time)

#!/bin/bash
for i in {1..2000}
do
curl --url "http://127.0.0.1:7783" --data "{\"userpass\":\"$userpass\",\"method\":\"buy\",\"base\":\"TKL\",\"rel\":\"KMD\",\"volume\":0.01,\"price\":0.013}"
sleep 0.01
done

and no crash in log.rs

@artemii235 artemii235 merged commit 43deac0 into mm2.1 Sep 2, 2022
@artemii235 artemii235 deleted the remove-duplex-mutex branch September 2, 2022 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@onur-ozkan onur-ozkan onur-ozkan approved these changes

@shamardy shamardy shamardy approved these changes

@cipig cipig cipig approved these changes

@artemii235 artemii235 artemii235 approved these changes

@laruh laruh laruh left review comments

@borngraced borngraced borngraced left review comments

@caglaryucekaya caglaryucekaya Awaiting requested review from caglaryucekaya

Assignees

@sergeyboyko0791 sergeyboyko0791

Labels
P0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

mm2 crashes in log.rs
7 participants
@sergeyboyko0791 @borngraced @cipig @artemii235 @shamardy @onur-ozkan @laruh