-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
utils: chunked_vector: fill ctor: make exception safe #18636
utils: chunked_vector: fill ctor: make exception safe #18636
Conversation
@@ -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() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this enough to guarantee exception safety?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, as the unit test that wasn't added proves.
(I'm guessing)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unit test added in v2
test/boost/chunked_vector_test.cc
Outdated
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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, can defer
test/boost/chunked_vector_test.cc
Outdated
--(*objects); | ||
} | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this class at all. Please document it so readers don't have to reverse-engineer it.
test/boost/chunked_vector_test.cc
Outdated
auto size_dist = std::uniform_int_distribution<unsigned>(1, 4 * max_chunk_capacity); | ||
|
||
ssize_t objects = 0; | ||
ssize_t inject = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that's external state. Suggest wrapping it in some class.
Looks good. I'm sure we have something similar to inject_exceptions we can reuse. |
I'll look for that. |
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>
23f1bde
to
4bbb66f
Compare
In v2 (4bbb66f):
|
exception_safe_class(exception_safe_class&&) = default; | ||
exception_safe_class(exception_safe_class&& x) noexcept : _esc(x._esc) { | ||
_esc.add_live_object_noexcept(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the move ctor was broken here.
🟢 CI State: SUCCESS✅ - Build Build Details:
|
@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
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