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

Remove napi-rs factory functions #977

Merged
merged 1 commit into from Feb 11, 2022
Merged

Remove napi-rs factory functions #977

merged 1 commit into from Feb 11, 2022

Conversation

dguenther
Copy link
Member

Summary

napi-rs has a concurrency bug when constructing objects using factory functions in worker threads. This change removes the use of factory functions in favor of constructors.

Testing Plan

Tested that syncing a chain from a clean database doesn't cause incorrect block verification errors. Other than that, the strategy tests should pass.

Breaking Change

Is this a breaking change? If yes, add notes below on why this is breaking and
what additional work is required, if any.

[ ] Yes
[x] No

@dguenther dguenther requested a review from a team as a code owner February 10, 2022 07:20
@dguenther dguenther changed the base branch from master to staging February 10, 2022 07:20
@dguenther dguenther changed the title Remove napi factory Remove napi-rs factory functions Feb 10, 2022
napi-rs has a concurrency bug when constructing objects using
factory functions in worker threads. This change removes the use
of factory functions in favor of constructors.
Copy link
Contributor

@mat-if mat-if left a comment

Choose a reason for hiding this comment

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

was able to sync up with no issues 👍

@dguenther dguenther merged commit 207edc7 into staging Feb 11, 2022
@dguenther dguenther deleted the remove-napi-factory branch February 11, 2022 00:48
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.

None yet

2 participants