8000 fix(rpc): concurrent map iteration and write by GAtom22 · Pull Request #1650 · evmos/evmos · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix(rpc): concurrent map iteration and write #1650

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
Merged
merged 5 commits into from
Jul 6, 2023
Merged

fix(rpc): concurrent map iteration and write #1650

merged 5 commits into from
Jul 6, 2023

Conversation

GAtom22
Copy link
Contributor
@GAtom22 GAtom22 commented Jul 5, 2023 •
edited
Loading

Description

This PR introduces the changes to solve reported racing conditions on the RPC pubSub logic

Reported issue logs:

fatal error: concurrent map iteration and map write

goroutine 12880390 [running]:
github.com/evmos/evmos/v12/rpc/ethereum/pubsub.(*memEventBus).publishAllSubscribers(0xc106c0b500, {0xc000fdab80?, 0xe53d86?}, {{0xc4b4d0baa0, 0x19}, {0x2987720, 0xc2bdef9880}, 0xc4b714af30})
	/go/src/github.com/tharsis/evmos/rpc/ethereum/pubsub/pubsub.go:152 +0x16f
github.com/evmos/evmos/v12/rpc/ethereum/pubsub.(*memEventBus).publishTopic(0xc106c0b500, {0xc000fdab80, 0x19}, 0xc1051f8bd0?)
...

Closes #XXX

@GAtom22 GAtom22 marked this pull request as ready for review July 5, 2023 22:49
@GAtom22 GAtom22 requested a review from a team as a code owner July 5, 2023 22:49
@GAtom22 GAtom22 requested review from Vvaradinov and MalteHerrmann and removed request for a team July 5, 2023 22:49
@GAtom22 GAtom22 mentioned this pull request Jul 5, 2023
Closed
@github-actions github-actions bot added the build label Jul 5, 2023
@codecov
Copy link
codecov bot commented Jul 5, 2023 •
edited
Loading

Codecov Report

Merging #1650 (63f8e83) into main (c87aaf6) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1650      +/-   ##
==========================================
+ Coverage   72.90%   72.94%   +0.04%     
==========================================
  Files         291      291              
  Lines       21227    21227              
==========================================
+ Hits        15475    15484       +9     
+ Misses       5003     4994       -9     
  Partials      749      749
Impacted Files Coverage Δ
rpc/ethereum/pubsub/pubsub.go 85.71% <100.00%> (+9.89%) ⬆️

facs95
facs95 approved these changes Jul 5, 2023
View reviewed changes
Copy link
Contributor
@facs95 facs95 left a comment

Choose a reason for hiding this comment

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

way better man thanks! Should we do the same iwth the closeAllSubscribers function?

@GAtom22
Copy link
Contributor Author
GAtom22 commented Jul 5, 2023

way better man thanks! Should we do the same iwth the closeAllSubscribers function?

That's already done on that function. There's a defer on the unlock

MalteHerrmann
MalteHerrmann reviewed Jul 6, 2023
View reviewed changes
Copy link
Contributor
@MalteHerrmann MalteHerrmann left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this @GAtom22! 🙏 I do have one question, if I revert the changes in pubsub.go the test is still passing, is that expected?

@GAtom22
Copy link
Contributor Author
GAtom22 commented Jul 6, 2023 •
edited
Loading

Thanks for fixing this @GAtom22! pray I do have one question, if I revert the changes in pubsub.go the test is still passing, is that expected?

@MalteHerrmann It is not deterministic, try running that test many times, it should panic when the concurrent iteration and write occurs

@GAtom22 GAtom22 merged commit 68f19ed into main Jul 6, 2023
@GAtom22 GAtom22 deleted the GAtom22/fix-pubsub branch July 6, 2023 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MalteHerrmann MalteHerrmann MalteHerrmann left review comments

@facs95 facs95 facs95 approved these changes

@Vvaradinov Vvaradinov Awaiting requested review from Vvaradinov Vvaradinov is a code owner automatically assigned from evmos/core-engineering

Assignees
No one assigned
Labels
build
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@GAtom22 @facs95 @MalteHerrmann
0