mirrors/scylladb
1
0
Fork 0
mirror of https://github.com/scylladb/scylladb.git synced 2026-05-12 19:02:12 +00:00
Code Issues Packages Projects Releases Wiki Activity

data: make cell::make_collection(): more consistent and safer

Browse Source 
3ec889816 changed cell::make_collection() to take different code paths
depending whether its `data` argument is nothrow copyable/movable or
not. In case it is not, it is wrapped in a view to make it so (see the
above mentioned commit for a full explanation), relying on the methods
pre-existing requirement for callers to keep `data` alive while the
created writer is in use.
On closer look however it turns out that this requirement is neither
respected, nor enforced, at least not on the code level. The real
requirement is that the underlying data represented by `data` is kept
alive. If `data` is a view, it is not expected to be kept alive and
callers don't, it is instead copied into `make_collection()`.
Non-views however *are* expected to be kept alive. This makes the API
error prone.
To avoid any future errors due to this ambiguity, require all `data`
arguments to be nothrow copyable and movable. Callers are now required
to pass views of nonconforming objects.

This patch is a usability improvement and is not fixing a bug. The
current code works as-is because it happens to conform to the underlying
requirements.

Refs: #5575
Refs: #5341

Tests: unit(dev)
Signed-off-by: Botond Dénes <bdenes@scylladb.com>
Message-Id: <20200115084520.206947-1-bdenes@scylladb.com>
This commit is contained in:
Botond Dénes
2020-01-15 10:45:20 +02:00
committed by Avi Kivity
parent 9aab75db60
commit 0c52c2ba50
2 changed files with 14 additions and 32 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
collection_mutation.cc
View File

@@ -33,7 +33,7 @@ collection_mutation::collection_mutation(const abstract_type& type, collection_m
: _data(imr_object_type::make(data::cell::make_collection(v.data), &type.imr_state().lsa_migrator())) {}
collection_mutation::collection_mutation(const abstract_type& type, const bytes_ostream& data)
: _data(imr_object_type::make(data::cell::make_collection(data), &type.imr_state().lsa_migrator())) {}
: _data(imr_object_type::make(data::cell::make_collection(fragment_range_view(data)), &type.imr_state().lsa_migrator())) {}
static collection_mutation_view get_collection_mutation_view(const uint8_t* ptr)
{

44
data/cell.hh
View File

@@ -277,23 +277,6 @@ private:
static thread_local imr::alloc::lsa_migrate_fn<external_chunk,
imr::alloc::context_factory<chunk_context>> lsa_chunk_migrate_fn;
template<typename FragmentRangeView>
GCC6_CONCEPT(
requires std::is_nothrow_move_constructible_v<std::decay_t<FragmentRangeView>> &&
std::is_nothrow_copy_constructible_v<std::decay_t<FragmentRangeView>> &&
std::is_nothrow_copy_assignable_v<std::decay_t<FragmentRangeView>> &&
std::is_nothrow_move_assignable_v<std::decay_t<FragmentRangeView>>
)
static auto do_make_collection(FragmentRangeView&& data) noexcept {
return [data] (auto&& serializer, auto&& allocations) noexcept {
return serializer
.serialize(imr::set_flag<tags::collection>(),
imr::set_flag<tags::external_data>(data.size_bytes() > maximum_internal_storage_length))
.template serialize_as<tags::collection>(variable_value::write(data), allocations)
.done();
};
}
public:
/// Make a writer that copies a cell
///
@@ -309,25 +292,24 @@ public:
/// \arg data needs to remain valid as long as the writer is in use.
/// \returns imr::WriterAllocator for cell::structure.
template<typename FragmentRange, typename = std::enable_if_t<is_fragment_range_v<std::decay_t<FragmentRange>>>>
static auto make_collection(const FragmentRange& data) noexcept {
// The writer (`do_make_collection()`) is noexcept so we have to make
// sure the data can be copied/moved without the risk of throwing
// exceptions. If `data` is nothrow copyable already, which is the
// case when `data` is a view, we forward it as-is. Otherwise we
// wrap `data` in a view and forward the view. We require the caller
// to keep `data` alive anyway.
if constexpr (std::is_nothrow_move_constructible_v<std::decay_t<FragmentRange>> &&
GCC6_CONCEPT(
requires std::is_nothrow_move_constructible_v<std::decay_t<FragmentRange>> &&
std::is_nothrow_copy_constructible_v<std::decay_t<FragmentRange>> &&
std::is_nothrow_copy_assignable_v<std::decay_t<FragmentRange>> &&
std::is_nothrow_move_assignable_v<std::decay_t<FragmentRange>>) {
return do_make_collection(data);
} else {
return do_make_collection(fragment_range_view(data));
}
std::is_nothrow_move_assignable_v<std::decay_t<FragmentRange>>
)
static auto make_collection(FragmentRange data) noexcept {
return [data = std::move(data)] (auto&& serializer, auto&& allocations) noexcept {
return serializer
.serialize(imr::set_flag<tags::collection>(),
imr::set_flag<tags::external_data>(data.size_bytes() > maximum_internal_storage_length))
.template serialize_as<tags::collection>(variable_value::write(data), allocations)
.done();
};
}
static auto make_collection(bytes_view data) noexcept {
return do_make_collection(single_fragment_range(data));
return make_collection(single_fragment_range(data));
}
/// Make a writer for a dead cell