utils: chunked_vector: fill ctor: make exception safe #18636

bhalevy · 2024-05-13T06:00:32Z

Currently, if the fill ctor throws an exception,
the destructor won't be called, as it object is not fully constructed yet.

Call the default ctor first (which doesn't throw)
to make sure the destructor will be called on exception.

Fixes #18635

Although the fixes is for a rare bug, it has very low risk and so it's worth backporting to all live versions

denesb · 2024-05-13T07:48:59Z

utils/chunked_vector.hh

@@ -351,7 +351,8 @@ chunked_vector<T, max_contiguous_allocation>::chunked_vector(Iterator begin, Ite
 }

 template <typename T, size_t max_contiguous_allocation>
-chunked_vector<T, max_contiguous_allocation>::chunked_vector(size_t n, const T& value) {
+chunked_vector<T, max_contiguous_allocation>::chunked_vector(size_t n, const T& value)
+        : chunked_vector() {


Is this enough to guarantee exception safety?

No, as the unit test that wasn't added proves.

(I'm guessing)

It is enough for calling the destructor if the body throws an exception, see https://godbolt.org/z/85W1aEjfE
And since the back_inserter updates _size for each insertion, the destructor will destroy all the items that were filled successfully.

In the context of #18609, we can optimize this and fill one chunk at a time using std::uninitialized_fill_n, that is exception safe.
See https://en.cppreference.com/w/cpp/memory/uninitialized_fill_n

If an exception is thrown during the initialization, the objects already constructed are destroyed in an unspecified order.

I can add unit tests to prove that :)

unit test added in v2

avikivity · 2024-05-13T10:05:26Z

test/boost/chunked_vector_test.cc

+    using chunked_vector = utils::chunked_vector<inject_exceptions, chunk_size>;
+    constexpr size_t max_chunk_capacity = chunked_vector::max_chunk_capacity();
+
+    auto rand = std::default_random_engine();


To support seed in a sane way we'll need to convert the test to be a seastar test (rather than a pure boost test)

ok, can defer

avikivity · 2024-05-13T10:08:23Z

test/boost/chunked_vector_test.cc

+            --(*objects);
+        }
+    }
+};


I don't understand this class at all. Please document it so readers don't have to reverse-engineer it.

avikivity · 2024-05-13T10:09:18Z

test/boost/chunked_vector_test.cc

+    auto size_dist = std::uniform_int_distribution<unsigned>(1, 4 * max_chunk_capacity);
+
+    ssize_t objects = 0;
+    ssize_t inject = 0;


Ah, that's external state. Suggest wrapping it in some class.

avikivity · 2024-05-13T10:10:40Z

Looks good. I'm sure we have something similar to inject_exceptions we can reuse.

bhalevy · 2024-05-13T10:38:26Z

Looks good. I'm sure we have something similar to inject_exceptions we can reuse.

I'll look for that.
FWIW, we have the error injection api, maybe it can be reused for this test.

avikivity · 2024-05-13T10:52:56Z

Looks good. I'm sure we have something similar to inject_exceptions we can reuse.

I'll look for that. FWIW, we have the error injection api, maybe it can be reused for this test.

exception_safety_checker/exception_safe_class.

Currently, if the fill ctor throws an exception, the destructor won't be called, as it object is not fully constructed yet. Call the default ctor first (which doesn't throw) to make sure the destructor will be called on exception. Fixes scylladb#18635 Signed-off-by: Benny Halevy <bhalevy@scylladb.com>

We have to account for moved objects as well as copied objects so they will be balanced with the respective `del_live_object` calls called by the destructor. However, since chunked_vector requires the value_type to be nothrow_move_constructible, just count the additional live object, but do not modify _countdown or, respectively, throw an exception, as this should be considered only for the default and copy constructors. Signed-off-by: Benny Halevy <bhalevy@scylladb.com>

For insertion, with and without reservation, and for fill and copy constructors. Reproduces scylladb#18635 Signed-off-by: Benny Halevy <bhalevy@scylladb.com>

bhalevy · 2024-05-13T14:21:40Z

In v2 (4bbb66f):

improved unit test
- reuse existing exception_safety_checker/exception_safe_class

avikivity · 2024-05-13T14:25:08Z

test/boost/chunked_vector_test.cc

-    exception_safe_class(exception_safe_class&&) = default;
+    exception_safe_class(exception_safe_class&& x) noexcept : _esc(x._esc) {
+        _esc.add_live_object_noexcept();
+    }


Yes, the move ctor was broken here.

scylladb-promoter · 2024-05-13T23:12:57Z

🟢 CI State: SUCCESS

✅ - Build
✅ - Unit Tests Custom
The following new/updated tests ran 100 times for each mode:
🔹 boost/chunked_vector_test
✅ - Container Test
✅ - dtest with topology changes
✅ - dtest
✅ - Unit Tests

Build Details:

Duration: 4 hr 59 min
Builder: i-08fbe3263f6c60739 (m5ad.12xlarge)

bhalevy · 2024-05-14T08:33:11Z

@scylladb/scylla-maint please merge

…n safe' from ScyllaDB Currently, if the fill ctor throws an exception, the destructor won't be called, as it object is not fully constructed yet. Call the default ctor first (which doesn't throw) to make sure the destructor will be called on exception. Fixes #18635 - [x] Although the fixes is for a rare bug, it has very low risk and so it's worth backporting to all live versions (cherry picked from commit 64c51cf) (cherry picked from commit 88b3173) (cherry picked from commit 4bbb66f) Refs #18636 Closes #18679 * github.com:scylladb/scylladb: chunked_vector_test: add more exception safety tests chunked_vector_test: exception_safe_class: count also moved objects utils: chunked_vector: fill ctor: make exception safe

…n safe' from ScyllaDB Currently, if the fill ctor throws an exception, the destructor won't be called, as it object is not fully constructed yet. Call the default ctor first (which doesn't throw) to make sure the destructor will be called on exception. Fixes #18635 - [x] Although the fixes is for a rare bug, it has very low risk and so it's worth backporting to all live versions (cherry picked from commit 64c51cf) (cherry picked from commit 88b3173) (cherry picked from commit 4bbb66f) Refs #18636 Closes #18680 * github.com:scylladb/scylladb: chunked_vector_test: add more exception safety tests chunked_vector_test: exception_safe_class: count also moved objects utils: chunked_vector: fill ctor: make exception safe

bhalevy added backport/5.2 Issues that should be backported to 5.2 branch once they'll be fixed backport/5.4 Issues that should be backported to 5.4 branch once they'll be fixed labels May 13, 2024

bhalevy mentioned this pull request May 13, 2024

utils: chunked_vector: optimize for trivially_copyable types #18609

Closed

1 task

denesb reviewed May 13, 2024

View reviewed changes

avikivity reviewed May 13, 2024

View reviewed changes

bhalevy added 3 commits May 13, 2024 17:18

chunked_vector_test: add more exception safety tests

4bbb66f

For insertion, with and without reservation, and for fill and copy constructors. Reproduces scylladb#18635 Signed-off-by: Benny Halevy <bhalevy@scylladb.com>

bhalevy force-pushed the chunked_vector-make-fill-ctor-safe branch from 23f1bde to 4bbb66f Compare May 13, 2024 14:20

avikivity reviewed May 13, 2024

View reviewed changes

avikivity approved these changes May 13, 2024

View reviewed changes

scylladb-promoter merged commit a15a9c3 into scylladb:master May 14, 2024
10 checks passed

github-actions bot added the promoted-to-master label May 14, 2024

This was referenced May 14, 2024

[Backport 5.4] utils: chunked_vector: fill ctor: make exception safe #18679

Merged

[Backport 5.2] utils: chunked_vector: fill ctor: make exception safe #18680

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

utils: chunked_vector: fill ctor: make exception safe #18636

utils: chunked_vector: fill ctor: make exception safe #18636

bhalevy commented May 13, 2024 •

edited

denesb May 13, 2024

avikivity May 13, 2024

bhalevy May 13, 2024

bhalevy May 13, 2024

avikivity May 13, 2024

bhalevy May 13, 2024

avikivity May 13, 2024

avikivity May 13, 2024

avikivity May 13, 2024

avikivity commented May 13, 2024

bhalevy commented May 13, 2024 •

edited

avikivity commented May 13, 2024

bhalevy commented May 13, 2024

avikivity May 13, 2024

scylladb-promoter commented May 13, 2024

bhalevy commented May 14, 2024

utils: chunked_vector: fill ctor: make exception safe #18636

utils: chunked_vector: fill ctor: make exception safe #18636

Conversation

bhalevy commented May 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avikivity commented May 13, 2024

bhalevy commented May 13, 2024 • edited

avikivity commented May 13, 2024

bhalevy commented May 13, 2024

Choose a reason for hiding this comment

scylladb-promoter commented May 13, 2024

🟢 CI State: SUCCESS

Build Details:

bhalevy commented May 14, 2024

bhalevy commented May 13, 2024 •

edited

bhalevy commented May 13, 2024 •

edited