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: api itest panic #12238

Merged
merged 2 commits into from
Jul 16, 2024
Merged

fix: api itest panic #12238

merged 2 commits into from
Jul 16, 2024

Conversation

aarshkshah1992
Copy link
Contributor

  1. Fixes flakiness in testNonGenesisMiner in api_test by de-racing the closing of the miner repo and the sealer reading it in a go-routine. If the go-routine gets scheduled after the miner repo is closed in the test, this causes a panic which fails the test. The right thing to do here is for the sealer to read the repo when it is instantiated and pass the config to the go-routine.

  2. Adds some logging to the testOutOfGasError itest to see what's going on when it fails (very very rare).

@aarshkshah1992 aarshkshah1992 added the skip/changelog This change does not require CHANGELOG.md update label Jul 15, 2024
@aarshkshah1992 aarshkshah1992 changed the title fix: api itest panic [skip_changelog]:fix: api itest panic Jul 15, 2024
@aarshkshah1992 aarshkshah1992 requested a review from rvagg July 15, 2024 16:18
node/impl/full/gas.go Outdated Show resolved Hide resolved
@rvagg
Copy link
Member

rvagg commented Jul 16, 2024

This breaks 2 public APIs, which while we may not expect people to be using them, we should at least signal and not just roll this out in a casual patch release.

My preference for signalling breakage is in commit message, which, if we can do consistently, will really help our changelog, so your commit message could be something like:

fix!: sealer: handle initialisation error without panic

storage/pipeline.NewPreCommitBatcher and storage/pipeline.New now have an additional
error return to deal with errors arising from fetching the sealing config

Also API breakage may be something we shouldn't skip changelog on?

@rjan90
Copy link
Contributor

rjan90 commented Jul 16, 2024

Also API breakage may be something we shouldn't skip changelog on?

Yes, we should add this the changelog / write down what the API´s we are breaking - so its clear/understandable what should be added in a Upgrade Warnings section of a new release.

storage/pipeline.NewPreCommitBatcher and storage/pipeline.New now have an additional
error return to deal with errors arising from fetching the sealing config.
@aarshkshah1992 aarshkshah1992 removed the skip/changelog This change does not require CHANGELOG.md update label Jul 16, 2024
@aarshkshah1992 aarshkshah1992 changed the title [skip_changelog]:fix: api itest panic fix: api itest panic Jul 16, 2024
@aarshkshah1992
Copy link
Contributor Author

@rvagg Addressed your review. Please can you review ?

@aarshkshah1992 aarshkshah1992 merged commit bae64f9 into master Jul 16, 2024
81 checks passed
@aarshkshah1992 aarshkshah1992 deleted the log/failed-api-itest branch July 16, 2024 06:24
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.

3 participants