From 0c52c2ba5083f2fb6fc6fd62197dfc2d6a7176cf Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Botond=20D=C3=A9nes?= <bdenes@scylladb.com>
Date: Wed, 15 Jan 2020 10:45:20 +0200
Subject: [PATCH] data: make cell::make_collection(): more consistent and safer
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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>
---
 collection_mutation.cc |  2 +-
 data/cell.hh           | 44 +++++++++++++-----------------------------
 2 files changed, 14 insertions(+), 32 deletions(-)

diff --git a/collection_mutation.cc b/collection_mutation.cc
index 1ac64639bb..bc0eb0ad9f 100644
--- a/collection_mutation.cc
+++ b/collection_mutation.cc
@@ -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)
 {
diff --git a/data/cell.hh b/data/cell.hh
index cd97b501d7..1da72b0964 100644
--- a/data/cell.hh
+++ b/data/cell.hh
@@ -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