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

Use a convenient global instead of passing use_memory_pools explicitly #2059

Merged
merged 1 commit into from Jun 4, 2019

Conversation

2 participants
@wezrule
Copy link
Collaborator

commented Jun 4, 2019

Originally it was discussed whether a global would be more beneficial if these pools were going to be used in more areas, globals are generally frowned upon given initialization issues etc.. it was decided that it would be riskier given we are close to a release. However @cryptocode found passing it as a parameter was not much safer. The nano::confirm_ack constructor has a new bool parameter, the prototype is now:
confirm_ack (bool &, nano::stream &, nano::message_header const &, bool, nano::vote_uniquer * = nullptr);

The function nano::message_parser::deserialize_confirm_ack declares the following:
nano::confirm_ack incoming (error, stream_a, header_a, &vote_uniquer);

This was mistakenly not modified. The &vote_uniquer is implicitly cast to the new bool parameter, so vote_uniquer is always nullptr here. Compiler errors didn't help here, and it probably shows we need a test for the vote_uniquer used here as well.

It seems a global would actually be safer as we make heavy use of default parameters. It doesn't need to be protected by a mutex luckily as we set the variable during initialization before it is used and we don't currently use it in any other global variables, I've done an ASAN run with the daemon on Mac/Clang a few times and it didn't find any issues.

For testing purposes, I've left the core_test using memory pool, rpc_test isn't, and the load_test uses 50% of both.

@wezrule wezrule added this to the V19.0 milestone Jun 4, 2019

@wezrule wezrule requested a review from cryptocode Jun 4, 2019

@wezrule wezrule self-assigned this Jun 4, 2019

@wezrule wezrule merged commit e68c6c5 into nanocurrency:master Jun 4, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wezrule wezrule deleted the wezrule:use_global_memory_pool branch Jun 4, 2019

@zhyatt zhyatt added this to RC 4 (TBD) in V19 Jun 6, 2019

argakiig added a commit that referenced this pull request Jun 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.