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.

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

Add IsMutable method back to Configurable class #8928

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Add IsMutable method back to Configurable class #8928

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6a939a2
Add tests
mrambacher Sep 16, 2021
513259b
Add tests
mrambacher Sep 16, 2021
2f0deee
Merge branch 'main' into IsPrepared
mrambacher Sep 17, 2021
7741cb2
Add IsMutable to Configurable class
mrambacher Sep 17, 2021
c22ee4b
make format
mrambacher Sep 17, 2021
a6d1141
Attempt to fix Windows build
mrambacher Sep 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions db/db_options_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,17 @@ TEST_F(DBOptionsTest, SetMutableTableOptions) {
{"table_factory.block_restart_interval", "7"}}));
ASSERT_EQ(c_bbto->block_size, 16384);
ASSERT_EQ(c_bbto->block_restart_interval, 13);

ASSERT_EQ(options.table_factory, c_opts.table_factory);
ConfigOptions config_options;
config_options.ignore_unsupported_options = false;
ASSERT_FALSE(options.table_factory->IsMutable());
ASSERT_NOK(options.table_factory->ConfigureOption(config_options,
"block_align", "false"));
ASSERT_OK(options.table_factory->ConfigureOption(config_options, "block_size",
"8192"));
c_opts = dbfull()->GetOptions(cfh);
ASSERT_EQ(c_bbto->block_size, 8192);
}

TEST_F(DBOptionsTest, SetBytesPerSync) {
Expand Down
4 changes: 4 additions & 0 deletions include/rocksdb/configurable.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,10 @@ class Configurable {
// initialized.
virtual Status PrepareOptions(const ConfigOptions& config_options);

// Returns true if the object is mutable. An object may be mutable until it
// is prepared and/or validated.
virtual bool IsMutable() const;
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand the problem we're trying to solve but I don't think this method really helps solve it. The problem is that there is an inherent race condition in this interface: in a multi-threaded context, the value returned by IsMutable is potentially stale by the time the caller acts on it. That is, the following snippet does not guarantee that PrepareOptions will be called only once since another thread might have swooped in between us checking IsMutable's return value and calling PrepareOptions:

if (config->IsMutable()) {
   Status status = config->PrepareOptions(opts);
   // ...
}

The way around this is to perform the check and prepare the options atomically (under the same mutex lock) in PrepareOptions. Seems to me this is already done in e.g. BlockBasedTableFactory::PrepareOptions, so I don't think IsMutable itself buys us anything.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not having IsMutable in the interface would also eliminate the question of what the default implementation is supposed to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you have a better ide

Not having IsMutable in the interface would also eliminate the question of what the default implementation is supposed to do.

There are two issues that this PR attempts to address:

  1. Some objects can only be initialized once. These include things like Cache and MemoryAllocator.
  2. Once an object is "prepared", immutable options cannot be changed via the "configure" methods.

The first issue could/can be solved in a thread-safe way without the IsMutable being in the interface. The solution there is to do a create/configure/prepare in a single step where there can be no other threads accessing the new object. There is no race and this is how I implemented it in the MemoryAllocator and Cache customizable PRs. The TableFactory methods theoretically still could have this potential race, though in reality I think most of that is avoided (since I do not believe a TableFactory can be shared between databases and will be prepared and validated at DB setup).

The only way I can see to solve the second issue is to put IsMutable into the API (either public or protected). Ideally this would simply set a bool (atomic ?). If we allow non-mutable options to be changed, then the values used by the system may not be updated correctly. For example, you should not be able to set the capacity of the Cache the same way once it is initialized and you cannot update any setting of the JEMallocator once it is initialized.

Do you have an alternative way of solving the second issue?

Copy link
Contributor

@ltamasi ltamasi Oct 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I might be missing something but I don't see a conceptual difference between the two issues. I think the second issue could be solved (in a thread-safe fashion) the same way as the first: by performing the mutability check and configuring the object in a single "atomic" operation

Copy link
Contributor

Choose a reason for hiding this comment

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

P.S. The current Configurable::ConfigureFromMap code in the PR seems thread-unsafe for the same reason as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I might be missing something but I don't see a conceptual difference between the two issues. I think the second issue could be solved (in a thread-safe fashion) the same way as the first: by performing the mutability check and configuring the object in a single "atomic" operation

Options can be configured (or serialized or compared) after an object is made immutable by many APIs. The DB code does exactly this internally currently and only allows mutable options to be updated. There is nothing that would prevent the user from changing immutable ones outside of the DB however.

For the objects that are "initialize only once" (Cache, MemoryAllocator, etc), the process is create, configure, prepare, lock, return the new object. There are no threading issues to deal with. There can be no race because the object cannot yet be shared amongst multiple objects. But to prevent the object from being configured later, the Configurable class (or each "initialize only once" class -- which would require a lot of overridden methods instead of a single one) needs to know that the object has been prepared.

P.S. The current Configurable::ConfigureFromMap code in the PR seems thread-_un_safe for the same reason as above

Yes, there is the potential for some objects (like TableFactory) to have Configure and Validate/Prepare methods called concurrently by multiple threads. Since this is a highly unlikely event (since these objects are not shared between databases), we could probably live with it. For TableFactory, there are typically two ways they are created -- either NewXXXFactory or loaded from the Options file (a two-stage process). For the former, we could "lock" the configurations (Prepare and mark immutable without ramifications -- outside of a few odd tests that do not expect "PreparedOptions"). For the latter, we could mark them immutable when they are loaded from the options file.

Note that the user typically has to go out of their way to create objects that are not prepared. The default option is to prepare options when they are created, so the potential race condition could be documented as a warning to the user on a potentially undefined behavior.

In other words, the race conditions you are concerned about are things we can prevent from happening by default, prevent from happening in many internal cases, and document as potential undefined behavior if the user choses to override the defaults.

Copy link
Contributor

Choose a reason for hiding this comment

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

Options can be configured (or serialized or compared) after an object is made immutable by many APIs. The DB code does exactly this internally currently and only allows mutable options to be updated. There is nothing that would prevent the user from changing immutable ones outside of the DB however.

Sure, I understand the issue. What I'm saying is that the two problems you outlined are similar in the sense that some configuration operations are not allowed after a certain point, namely once the configurable is prepared. And in the presence of multiple threads, we would need method(s) that combine the check and the operation in question in order to avoid race conditions. This is the same kind of interface problem that affects multi-threaded data structures like queues or stacks: there's no point in providing, say, separate IsEmpty and Pop methods because the value returned by IsEmpty can go stale by the time the caller acts on it and calls (or doesn't call) Pop. (The solution is the same there too: fold the emptiness check and the pop operation into a TryPop method.)

Yes, there is the potential for some objects (like TableFactory) to have Configure and Validate/Prepare methods called concurrently by multiple threads. Since this is a highly unlikely event (since these objects are not shared between databases), we could probably live with it.

I believe we do support sharing a single TableFactory between multiple databases.

In other words, the race conditions you are concerned about are things we can prevent from happening by default, prevent from happening in many internal cases, and document as potential undefined behavior if the user choses to override the defaults.

In my opinion, the chances of the application attempting to "illegally" reconfigure an object via a pointer they hold are very slim to start with. In addition, even if we plug every hole in the Configurable framework (which this PR doesn't do), the application can still mess with the objects behind the framework's back, and there's no way we can prevent that. Given all this, I'm not sure if this partial solution is worth the extra complexity.

Copy link
Contributor

Choose a reason for hiding this comment

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

P.S. One way to completely eliminate unwanted reconfigurations would be to clone the objects provided by the application. Unfortunately, this would also preclude us from sharing objects across DBs etc., so it's not really an option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Options can be configured (or serialized or compared) after an object is made immutable by many APIs. The DB code does exactly this internally currently and only allows mutable options to be updated. There is nothing that would prevent the user from changing immutable ones outside of the DB however.

Sure, I understand the issue. What I'm saying is that the two problems you outlined are similar in the sense that some configuration operations are not allowed after a certain point, namely once the configurable is prepared. And in the presence of multiple threads, we would need method(s) that combine the check and the operation in question in order to avoid race conditions. This is the same kind of interface problem that affects multi-threaded data structures like queues or stacks: there's no point in providing, say, separate IsEmpty and Pop methods because the value returned by IsEmpty can go stale by the time the caller acts on it and calls (or doesn't call) Pop. (The solution is the same there too: fold the emptiness check and the pop operation into a TryPop method.)

I agree this PR does not combine the "check" and "operate" for TableFactories -- this gap is currently there for backwards compatibility. But for TableFactory there is nothing that goes completely haywire if the operation goes stale. For the objects where it goes stale (MemoryAllocator, Cache), the "check" and "operate" are combined (by always returning an object that is already prepared and cannot be later modified) and there is no race. Without the changes to the Configurable class for IsMutable, I cannot prevent the Configure/Prepare methods from being called when they should not be unless I override all of them for every class that falls under this pattern, which seems like more code that is harder to maintain than to put it into one base class.

In my opinion, the chances of the application attempting to "illegally" reconfigure an object via a pointer they hold are very slim to start with. In addition, even if we plug every hole in the Configurable framework (which this PR doesn't do), the application can still mess with the objects behind the framework's back, and there's no way we can prevent that. Given all this, I'm not sure if this partial solution is worth the extra complexity.

The chances of an application "illegally" reconfiguring an object are much greater than hitting the threading issues (empty/pop). The API is there for every object and can be called from outside of the DB

The only way I am aware of for objects being messed with behind the framework's back is by getting a pointer to the options (Configurable::GetOptions) and changing the values, which is (and was before this framework existed) documented as unsupported behavior.

I fully agree with you that this change does not completely plug the mutable holes for the TableFactory, but it was also not intended to do so. But I believe this change does allow the hole to be plugged completely for objects like MemoryAllocator and Cache in a simpler manner than could otherwise be implemented. Without this change, I believe those classes will be much more complex and harder to make Customizable and fit into the framework.


// Checks to see if the settings are valid for this object.
// This method checks to see if the input DBOptions and ColumnFamilyOptions
// are valid for the settings of this object. For example, an Env might not
Expand Down
61 changes: 38 additions & 23 deletions options/configurable.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,14 @@ void Configurable::RegisterOptions(
// Methods for Initializing and Validating Configurable Objects
//
//*************************************************************************
bool Configurable::IsMutable() const {
// By default, assumes that if there are options, the object is mutable
return !options_.empty();
}

Status Configurable::PrepareOptions(const ConfigOptions& opts) {
// We ignore the invoke_prepare_options here intentionally,
// as if you are here, you must have called PrepareOptions explicitly.
Status status = Status::OK();
#ifndef ROCKSDB_LITE
for (auto opt_iter : options_) {
for (auto map_iter : *(opt_iter.type_map)) {
Expand All @@ -51,13 +54,15 @@ Status Configurable::PrepareOptions(const ConfigOptions& opts) {
Configurable* config =
opt_info.AsRawPointer<Configurable>(opt_iter.opt_ptr);
if (config != nullptr) {
status = config->PrepareOptions(opts);
if (!status.ok()) {
return status;
if (config->IsMutable()) {
Status status = config->PrepareOptions(opts);
if (!status.ok()) {
return status;
}
}
} else if (!opt_info.CanBeNull()) {
status =
Status::NotFound("Missing configurable object", map_iter.first);
return Status::NotFound("Missing configurable object",
map_iter.first);
}
}
}
Expand All @@ -66,7 +71,7 @@ Status Configurable::PrepareOptions(const ConfigOptions& opts) {
#else
(void)opts;
#endif // ROCKSDB_LITE
return status;
return Status::OK();
}

Status Configurable::ValidateOptions(const DBOptions& db_opts,
Expand Down Expand Up @@ -152,22 +157,19 @@ Status Configurable::ConfigureFromMap(
const ConfigOptions& config_options,
const std::unordered_map<std::string, std::string>& opts_map,
std::unordered_map<std::string, std::string>* unused) {
return ConfigureOptions(config_options, opts_map, unused);
}

Status Configurable::ConfigureOptions(
const ConfigOptions& config_options,
const std::unordered_map<std::string, std::string>& opts_map,
std::unordered_map<std::string, std::string>* unused) {
std::string curr_opts;
bool is_mutable = IsMutable();
ConfigOptions copy = config_options;
Status s;
if (!opts_map.empty()) {
// There are options in the map.
// Save the current configuration in curr_opts and then configure the
// options, but do not prepare them now. We will do all the prepare when
// the configuration is complete.
ConfigOptions copy = config_options;
copy.invoke_prepare_options = false;
if (!is_mutable) {
copy.mutable_options_only = true;
}
#ifndef ROCKSDB_LITE
if (!config_options.ignore_unknown_options) {
// If we are not ignoring unused, get the defaults in case we need to
Expand All @@ -177,17 +179,15 @@ Status Configurable::ConfigureOptions(
GetOptionString(copy, &curr_opts).PermitUncheckedError();
}
#endif // ROCKSDB_LITE

s = ConfigurableHelper::ConfigureOptions(copy, *this, opts_map, unused);
s = ConfigureOptions(copy, opts_map, unused);
}
if (config_options.invoke_prepare_options && s.ok()) {
s = PrepareOptions(config_options);
}
#ifndef ROCKSDB_LITE
if (!s.ok() && !curr_opts.empty()) {
ConfigOptions reset = config_options;
ConfigOptions reset = copy;
reset.ignore_unknown_options = true;
reset.invoke_prepare_options = true;
reset.ignore_unsupported_options = true;
// There are some options to reset from this current error
ConfigureFromString(reset, curr_opts).PermitUncheckedError();
Expand All @@ -196,6 +196,14 @@ Status Configurable::ConfigureOptions(
return s;
}

Status Configurable::ConfigureOptions(
const ConfigOptions& config_options,
const std::unordered_map<std::string, std::string>& opts_map,
std::unordered_map<std::string, std::string>* unused) {
return ConfigurableHelper::ConfigureOptions(config_options, *this, opts_map,
unused);
}

Status Configurable::ParseStringOptions(const ConfigOptions& /*config_options*/,
const std::string& /*opts_str*/) {
return Status::OK();
Expand Down Expand Up @@ -238,10 +246,15 @@ Status Configurable::ConfigureFromString(const ConfigOptions& config_options,
Status Configurable::ConfigureOption(const ConfigOptions& config_options,
const std::string& name,
const std::string& value) {
return ConfigurableHelper::ConfigureSingleOption(config_options, *this, name,
value);
if (IsMutable() || config_options.mutable_options_only) {
return ConfigurableHelper::ConfigureSingleOption(config_options, *this,
name, value);
} else {
ConfigOptions copy = config_options;
copy.mutable_options_only = true;
return ConfigurableHelper::ConfigureSingleOption(copy, *this, name, value);
}
}

/**
* Looks for the named option amongst the options for this type and sets
* the value for it to be the input value.
Expand Down Expand Up @@ -415,7 +428,9 @@ Status ConfigurableHelper::ConfigureCustomizableOption(
return configurable.ParseOption(copy, opt_info, name, value, opt_ptr);
} else if (value.empty()) {
return Status::OK();
} else if (custom == nullptr || !StartsWith(name, custom->GetId() + ".")) {
} else if (custom == nullptr ||
EndsWith(opt_name, OptionTypeInfo::kIdPropSuffix()) ||
name == OptionTypeInfo::kIdPropName()) {
return configurable.ParseOption(copy, opt_info, name, value, opt_ptr);
} else if (value.find("=") != std::string::npos) {
return custom->ConfigureFromString(copy, value);
Expand Down
Loading