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

Fix deadlock due to incorrect mutex #91

Merged
merged 8 commits into from
Oct 24, 2022

Conversation

gusin13
Copy link
Collaborator

@gusin13 gusin13 commented Oct 24, 2022

No description provided.

@gusin13 gusin13 marked this pull request as ready for review October 24, 2022 02:45
@gusin13 gusin13 requested a review from vitsalis October 24, 2022 02:45
Copy link
Member

@SebastianElvis SebastianElvis left a comment

Choose a reason for hiding this comment

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

Thanks! Some questions on the design.

types/btccache.go Outdated Show resolved Hide resolved
types/btccache.go Outdated Show resolved Hide resolved
gusin13 and others added 2 commits October 23, 2022 23:33
Co-authored-by: Runchao Han <[email protected]>
This reverts commit 4e0f9f5.
types/btccache.go Outdated Show resolved Hide resolved
Copy link
Member

@SebastianElvis SebastianElvis left a comment

Choose a reason for hiding this comment

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

LGTM except for some minor issues!

Comment on lines 48 to 53
if b.size() >= b.maxEntries {
b.blocks = b.blocks[1:]
}

b.blocks = append(b.blocks, ib)
return nil
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if b.size() >= b.maxEntries {
b.blocks = b.blocks[1:]
}
b.blocks = append(b.blocks, ib)
return nil
return b.add(ib)

}

// Reverse reverses the order of blocks in cache in place. Thread-safe.
func (b *BTCCache) Reverse() error {
b.Lock()
defer b.Unlock()

Copy link
Member

Choose a reason for hiding this comment

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

Ditto, you can just do

	return b.reverse()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

return nil
}

// Lock free version of Add
Copy link
Member

Choose a reason for hiding this comment

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

Lock-free has its own meaning. So perhaps just say "thread-unsafe" here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

func (b *BTCCache) Size() uint64 {
b.RLock()
defer b.RUnlock()

return uint64(len(b.blocks))
}

func (b *BTCCache) reverse() error {
// lock free version of Size
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@@ -94,6 +112,15 @@ func (b *BTCCache) reverse() error {
return nil
}

// lock free version of reverse
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@gusin13 gusin13 merged commit a7e3d25 into main Oct 24, 2022
@gusin13 gusin13 deleted the BM-244-deadlock-due-incorrect-locks-in-reporter branch October 24, 2022 04:53
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

Successfully merging this pull request may close these issues.

2 participants