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

op-batcher: Make channel manager safe for concurrent use #6878

Merged
merged 3 commits into from
Aug 18, 2023

Conversation

trianglesphere
Copy link
Contributor

Description

Because the batcher is concurrent with respect to sending transactions, the main loop would fetch transaction data & record transaction status updates in different go routines. This would lead to concurrent access of the txChannels map which causes a panic.

This commit adds locks to the public methods for the channel manager to make it safe for concurrent use.

Reported by bnoeih in #6093

Tests

None.

Metadata

Because the batcher is concurrent with respect to sending transactions,
the main loop would fetch transaction data & record transaction status
updates in different go routines. This would lead to concurrent access
of the txChannels map which causes a panic.

This commit adds locks to the public methods for the channel manager
to make it safe for concurrent use.

Co-authored-by: bnoieh <135800952+bnoieh@users.noreply.github.com>
@codecov
Copy link

codecov bot commented Aug 17, 2023

Codecov Report

Merging #6878 (d8cda3f) into develop (edafea3) will increase coverage by 5.92%.
The diff coverage is 100.00%.

❗ Current head d8cda3f differs from pull request most recent head dade91a. Consider uploading reports for the commit dade91a to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #6878      +/-   ##
===========================================
+ Coverage    44.82%   50.74%   +5.92%     
===========================================
  Files          376      314      -62     
  Lines        29542    23677    -5865     
  Branches      1692        0    -1692     
===========================================
- Hits         13242    12016    -1226     
+ Misses       14858    10617    -4241     
+ Partials      1442     1044     -398     
Flag Coverage Δ
bedrock-go-tests 47.86% <100.00%> (+0.01%) ⬆️
cannon-go-tests 64.18% <ø> (ø)
chain-mon-tests ?
common-ts-tests ?
contracts-bedrock-tests ∅ <ø> (∅)
contracts-ts-tests ?
core-utils-tests ?
sdk-next-tests ?
sdk-tests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
op-batcher/batcher/channel_manager.go 83.26% <100.00%> (+0.90%) ⬆️

... and 69 files with indirect coverage changes

Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

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

LGTM.

@mergify
Copy link
Contributor

mergify bot commented Aug 18, 2023

This PR has been added to the merge queue, and will be merged soon.

@mergify mergify bot added the S-on-merge-train Status: This PR is in the merge queue label Aug 18, 2023
@mergify
Copy link
Contributor

mergify bot commented Aug 18, 2023

This PR is next in line to be merged, and will be merged as soon as checks pass.

1 similar comment
@mergify
Copy link
Contributor

mergify bot commented Aug 18, 2023

This PR is next in line to be merged, and will be merged as soon as checks pass.

@mergify
Copy link
Contributor

mergify bot commented Aug 18, 2023

This PR is next in line to be merged, and will be merged as soon as checks pass.

@OptimismBot OptimismBot merged commit 2011f6a into develop Aug 18, 2023
62 of 65 checks passed
@OptimismBot OptimismBot deleted the jg/race_cond_in_batcher branch August 18, 2023 15:13
@mergify mergify bot removed the S-on-merge-train Status: This PR is in the merge queue label Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-op-batcher Area: op-batcher
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants