Skip to content
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.

Sign up for GitHub

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

Jump to bottom

create_scheduling_group / scheduling_group_key_create not exception safe when SG key data constructor throws #2222

Open
piodul opened this issue May 6, 2024 · 2 comments · May be fixed by #2617
Open

create_scheduling_group / scheduling_group_key_create not exception safe when SG key data constructor throws #2222

piodul opened this issue May 6, 2024 · 2 comments · May be fixed by #2617
Assignees
@piodul @mlitvk

Comments

@piodul
Copy link
Contributor

piodul commented May 6, 2024

There are two cases where data for a scheduling group key can be constructed:

  • When the SG key already exists and a new SG is created (create_scheduling_group)
  • When a new SG key is being created (scheduling_group_key_create)

In both cases, the SG key data constructor can fail. This is an example test for the second case:

SEASTAR_THREAD_TEST_CASE(sg_key_constructor_exception_when_creating_new_key) {
    struct thrower {
        thrower() {
            throw std::runtime_error("oopsie daisy");
        }
        ~thrower() {
            // Shouldn't get here because the constructor shouldn't succeed
            BOOST_ASSERT(false);
        }
    };
    scheduling_group_key_config thrower_conf = make_scheduling_group_key_config<thrower>();
    BOOST_REQUIRE_THROW(scheduling_group_key_create(thrower_conf).get(), std::runtime_error);
}

Even though the constructor of thrower always throws, its destructor is called anyway:

scheduling_group_test: /home/piodul/code/seastar/tests/unit/scheduling_group_test.cc:392: sg_key_constructor_exception_when_creating_new_key::do_run_test_case()::thrower::~thrower(): Assertion `false' failed.
scheduling_group_test: /home/piodul/code/seastar/tests/unit/scheduling_group_test.cc:392: sg_key_constructor_exception_when_creating_new_key::do_run_test_case()::thrower::~thrower(): Assertion `false' failed.
scheduling_group_test: /home/piodul/code/seastar/tests/unit/scheduling_group_test.cc:392: sg_key_constructor_exception_when_creating_new_key::do_run_test_case()::thrower::~thrower(): Assertion `false' failed.
Aborted (core dumped)

It looks like both functions are not exception safe and cause issues later if the SG key data constructor fails with an exception. I believe that both operations should be able to roll back in case of an exception.
@piodul piodul self-assigned this May 9, 2024
@piodul
Copy link
Contributor Author

piodul commented Jan 13, 2025

@mlitvk Your PR #2585 only fixed #2231 and not this one (#2222), correct?

@mlitvk
Copy link
Contributor

mlitvk commented Jan 13, 2025

@mlitvk Your PR #2585 only fixed #2231 and not this one (#2222), correct?

yes

mlitvk added a commit to mlitvk/seastar that referenced this issue Jan 15, 2025
@mlitvk
scheduling_group: improve scheduling group creation exception safety
99e0dc9 
Improve handling of exceptions during scheduling group and scheduling
group key creation, where a user-provided constructor for the keys may
fail, for example.

We use a new struct `specific_val` and smart pointers to manage memory
allocation, construction and destruction of scheduling group data in a
safe manner.

We also reorder the initialization order to make it safer. For
example, when creating a scheduling group, first allocate all data and
then swap it into the scheduling group's data structure.

Fixes scylladb#2222
@mlitvk mlitvk linked a pull request Jan 15, 2025 that will close this issue
Open
@mlitvk mlitvk self-assigned this Jan 15, 2025
mlitvk added a commit to mlitvk/seastar that referenced this issue Jan 15, 2025
@mlitvk
scheduling_group: improve scheduling group creation exception safety
2fb1270 
Improve handling of exceptions during scheduling group and scheduling
group key creation, where a user-provided constructor for the keys may
fail, for example.

We use a new struct `specific_val` and smart pointers to manage memory
allocation, construction and destruction of scheduling group data in a
safe manner.

We also reorder the initialization order to make it safer. For
example, when creating a scheduling group, first allocate all data and
then swap it into the scheduling group's data structure.

Fixes scylladb#2222
mlitvk added a commit to mlitvk/seastar that referenced this issue Jan 15, 2025
@mlitvk
scheduling_group: improve scheduling group creation exception safety
b16a627 
Improve handling of exceptions during scheduling group and scheduling
group key creation, where a user-provided constructor for the keys may
fail, for example.

We use a new struct `specific_val` and smart pointers to manage memory
allocation, construction and destruction of scheduling group data in a
safe manner.

We also reorder the initialization order to make it safer. For
example, when creating a scheduling group, first allocate all data and
then swap it into the scheduling group's data structure.

Fixes scylladb#2222
mlitvk added a commit to mlitvk/seastar that referenced this issue Jan 15, 2025
@mlitvk
scheduling_group: improve scheduling group creation exception safety
88c4e8a 
Improve handling of exceptions during scheduling group and scheduling
group key creation, where a user-provided constructor for the keys may
fail, for example.

We use a new struct `specific_val` and smart pointers to manage memory
allocation, construction and destruction of scheduling group data in a
safe manner.

We also reorder the initialization order to make it safer. For
example, when creating a scheduling group, first allocate all data and
then swap it into the scheduling group's data structure.

Fixes scylladb#2222
mlitvk added a commit to mlitvk/seastar that referenced this issue Jan 16, 2025
@mlitvk
scheduling_group: improve scheduling group creation exception safety
cd957c2 
Improve handling of exceptions during scheduling group and scheduling
group key creation, where a user-provided constructor for the keys may
fail, for example.

We use a new struct `specific_val` and smart pointers to manage memory
allocation, construction and destruction of scheduling group data in a
safe manner.

We also reorder the initialization order to make it safer. For
example, when creating a scheduling group, first allocate all data and
then swap it into the scheduling group's data structure.

Fixes scylladb#2222
mlitvk added a commit to mlitvk/seastar that referenced this issue Jan 21, 2025
@mlitvk
scheduling_group: improve scheduling group creation exception safety
a9615eb 
Improve handling of exceptions during scheduling group and scheduling
group key creation, where a user-provided constructor for the keys may
fail, for example.

We use a new struct `specific_val` and smart pointers to manage memory
allocation, construction and destruction of scheduling group data in a
safe manner.

We also reorder the initialization order to make it safer. For
example, when creating a scheduling group, first allocate all data and
then swap it into the scheduling group's data structure.

Fixes scylladb#2222
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@piodul piodul

@mlitvk mlitvk

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

scheduling_group: improve scheduling group creation exception safety mlitvk/seastar
2 participants
@piodul @mlitvk