fix: multithreading issue part 2 by pankcuf · Pull Request #512 · dashpay/dashsync-iOS

pankcuf · 2023-08-01T12:15:59Z

Issue being fixed or feature implemented

Fix: some thread-safety issues
Add disconnect reason entity

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated relevant unit/integration/functional/e2e tests
I have made corresponding changes to the documentation

For repository code-owners and collaborators only

I have assigned this pull request to a milestone

…h in delegate queue

tikhop · 2023-08-02T12:50:06Z

 - (void)updateAttributesFromSimplifiedMasternodeEntry:(DSSimplifiedMasternodeEntry *)simplifiedMasternodeEntry atBlockHeight:(uint32_t)blockHeight knownOperatorAddresses:(NSDictionary<NSString *, DSAddressEntity *> *)knownOperatorAddresses knownVotingAddresses:(NSDictionary<NSString *, DSAddressEntity *> *)knownVotingAddresses localMasternodes:(NSDictionary<NSData *, DSLocalMasternodeEntity *> *)localMasternodes {
    if (self.updateHeight < blockHeight) {
-        NSAssert(simplifiedMasternodeEntry.updateHeight == blockHeight, @"the block height should be the same as the entry update height");
+        //NSAssert(simplifiedMasternodeEntry.updateHeight == blockHeight, @"the block height should be the same as the entry update height");


@pankcuf Why commented?

tikhop

Most of the PR looks good except the overuse of '@synchronized'. I don't think this is the solution to our multithreading issues.

tikhop · 2023-08-02T12:53:18Z

-    double maxAmount = self.masternodeListRetrievalQueueMaxAmount;
-    if (!amountLeft) {
-        return self.store.masternodeListsAndQuorumsIsSynced;
+    @synchronized (self) {


@pankcuf I don't think @synchronized is a real solution, I personally against using @synchronized because it can lead performance decreasing and other "unknowns"

tikhop · 2023-08-02T12:53:47Z

-            return 1;
-        } else {
-            return 0;
+        @synchronized (self.peerManager) {


@pankcuf I don't think @synchronized is a real solution, I personally against using @synchronized because it can lead performance decreasing and other "unknowns"

tikhop · 2023-08-02T12:54:11Z

    self.store.lastQueriedBlockHash = blockHash;
    NSData *blockHashData = uint256_data(blockHash);
-    [self.store.masternodeListQueriesNeedingQuorumsValidated addObject:blockHashData];
+    @synchronized (self.store.masternodeListQueriesNeedingQuorumsValidated) {


@pankcuf I don't think @synchronized is a real solution, I personally against using @synchronized because it can lead performance decreasing and other "unknowns"

tikhop · 2023-08-02T12:54:17Z

        NSData *masternodeListBlockHashData = uint256_data(masternodeListBlockHash);
-        if ([neededMissingMasternodeLists count] && [self.store.masternodeListQueriesNeedingQuorumsValidated containsObject:masternodeListBlockHashData]) {
+        BOOL hasAwaitingQuorumValidation;
+        @synchronized (self.store.masternodeListQueriesNeedingQuorumsValidated) {


@pankcuf I don't think @synchronized is a real solution, I personally against using @synchronized because it can lead performance decreasing and other "unknowns"

tikhop · 2023-08-02T12:54:28Z

            if (uint256_eq(self.store.lastQueriedBlockHash, masternodeListBlockHash)) {
                self.masternodeListDiffService.currentMasternodeList = masternodeList;
-                [self.store.masternodeListQueriesNeedingQuorumsValidated removeObject:masternodeListBlockHashData];
+                @synchronized (self.store.masternodeListQueriesNeedingQuorumsValidated) {


@pankcuf I don't think @synchronized is a real solution, I personally against using @synchronized because it can lead performance decreasing and other "unknowns"

tikhop · 2023-08-02T12:58:32Z

In addition all tests are failing and it will be good to have them working again

tikhop · 2023-08-02T13:00:20Z

Another minor issue is PR's title. We agreed a year ago to use conventional style.

pankcuf · 2023-08-29T15:29:27Z

@tikhop You claim that the use "@synchronized" leads to some kind of overhead. In general I agree with that point. But from my perspective, we must proceed from practical considerations and we have several alternatives:

to deal with periodic anonymous crashes related to the thread unsafety (pause this PR).
to proceed with this solution that uses @synchronized directive – It will not slowing down anything (merge this PR),
go deep into that story, and rewrite a half of dashsync to make everything more efficient (...).
I decided to stick with the second option, as it seems to me the most reasonable under the current conditions.
And there are several reasons for this:

Because a small overhead is better than random crashes themselves.
DashSync code itself uses a lot of @synchronized. Without resolving them, it is impossible to write a really good architecture.
It seems to me that the task of implementing a more competent approach, in terms of orchestration of threads, is much closer to a rust solution than in DashSync. So it's just not worth it to rewrite DashSync.

Therefore, I suggest as follows, roll out the build from this feature branch and test it. If it's ok, then to merge it.

pankcuf added 18 commits July 21, 2023 17:56

fix: make peer handlers (ping/pong)

fb9d4ff

fix: ping/pong continue

11a0d08

fix: all found thread-safety issues

2bbece5

fix: some more thread sync

d25ce51

chore: @sync masternode cache

2617c7a

chore: some more syncs

1d5fce3

chore: format

25994dd

chore: peer: synchronized knownTxHashes & separate method for dispatc…

24cb890

…h in delegate queue

improve: rewrite initialisation for blocks RAM storage

c471c4b

fix: access to chain quick height

f244348

fix: synced remove masternode list cache

7086777

fix: revert assign-sync-weights

766046d

fix: @sync blocks storage when wiping

78316d2

fix: tmp remove assert

fb194b4

fix: revert peer @sync status (it can lead to deadlocks)

0dd60c9

fix: @sync masternode cache while retrieving it from DB

5ddc7db

fix: main thread deadlock

e50aa01

chore: remove unsued

e2c34f3

pankcuf requested a review from a team August 1, 2023 12:16

tikhop reviewed Aug 2, 2023

View reviewed changes

tikhop suggested changes Aug 2, 2023

View reviewed changes

pankcuf changed the title ~~Fix/multithreading issue part 2~~ fix: multithreading issue part 2 Aug 29, 2023

chore: update dash-shared-core 0.4.11 (fix: check script nullability)

88e9145

pankcuf merged commit dda76aa into develop Sep 18, 2023

pankcuf mentioned this pull request Sep 18, 2023

chore(release): bump version to v2.4.11 #514

Merged

5 tasks

pankcuf mentioned this pull request Oct 2, 2023

chore: Release 7.1.0 dashpay/dashwallet-ios#607

Merged

5 tasks

pankcuf deleted the fix/multithreading-issue-part-2 branch November 10, 2023 16:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: multithreading issue part 2#512

fix: multithreading issue part 2#512
pankcuf merged 19 commits into
developfrom
fix/multithreading-issue-part-2

pankcuf commented Aug 1, 2023

Uh oh!

tikhop Aug 2, 2023

Uh oh!

tikhop left a comment

Uh oh!

tikhop Aug 2, 2023

Uh oh!

tikhop Aug 2, 2023

Uh oh!

tikhop Aug 2, 2023

Uh oh!

tikhop Aug 2, 2023

Uh oh!

tikhop Aug 2, 2023

Uh oh!

tikhop commented Aug 2, 2023

Uh oh!

tikhop commented Aug 2, 2023

Uh oh!

pankcuf commented Aug 29, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

pankcuf commented Aug 1, 2023

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

Uh oh!

tikhop Aug 2, 2023

Choose a reason for hiding this comment

Uh oh!

tikhop left a comment

Choose a reason for hiding this comment

Uh oh!

tikhop Aug 2, 2023

Choose a reason for hiding this comment

Uh oh!

tikhop Aug 2, 2023

Choose a reason for hiding this comment

Uh oh!

tikhop Aug 2, 2023

Choose a reason for hiding this comment

Uh oh!

tikhop Aug 2, 2023

Choose a reason for hiding this comment

Uh oh!

tikhop Aug 2, 2023

Choose a reason for hiding this comment

Uh oh!

tikhop commented Aug 2, 2023

Uh oh!

tikhop commented Aug 2, 2023

Uh oh!

pankcuf commented Aug 29, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants