Unsynchronized cross-shard memory operations caused by incorrectly used updateable_value

[piodul]

The bug was introduced in 4b856cf.

In transport/controller.cc, there is a function named controller::do_start_server() which creates a cql_server service. That function runs on shard 0, where it creates a cql_server_config object. This object is then used as an argument to start the cql_server service:

cserver->start(std::ref(cql3::get_query_processor()), std::ref(_auth_service), std::ref(_mnotifier), cql_server_config).get();

Calling distributed<cql_server>::start means that the configuration is copied to all shards and used as an argument for cql_server's constructor.

The commit mentioned at the beginning converted one of cql_server_config's parameters into an updateable_value. The updateable_value is an object which tracks the value of updateable_value_source (which can be a config parameter, for example) and is updated with source's value when it changes.

The problem is that the updateable_values sent to other shards are still associated with the source on shard 0. Because they are tracked through a std::vector of pointers which is a member of the source, it can cause unsynchronized cross-shard reads and writes which is unsafe and may lead to strange bugs.

The implementation of updateable_value is definitely not safe to have cross-shard references like that - unlike what the comment above updateable_value's definition would suggest:

// A T that can be updated at runtime; uses updateable_value_base to track
// the source as the object is moved or copied. Copying across shards is supported.

[piodul]

@avikivity About the comment above updateable_value... It certainly is misleading. Should we change the comment so that it doesn't suggest that it's safe to copy across shards, or rather change the implementation so that the comment is true?

[psarna]

There's also another series of mine, already queued in next, which adds the same mistake to alternator. I'll temporarily dequeue it.

[avikivity]

@avikivity About the comment above updateable_value... It certainly is misleading. Should we change the comment so that it doesn't suggest that it's safe to copy across shards, or rather change the implementation so that the comment is true?

updatable_value was explicitly written to support this use case (I know the author personally). It should be fixed, so it is easy and cheap to pass live configuration values across shards.

[piodul]

updatable_value was explicitly written to support this use case (I know the author personally). It should be fixed, so it is easy and cheap to pass live configuration values across shards.

@avikivity I made an issue: #7316

[avikivity]

Not in any release branch, so no backports needed.