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.

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

Row cache updates do not provide strong exception safety guarantees #15576

Closed
bhalevy opened this issue Sep 28, 2023 · 6 comments · Fixed by #15577
Closed

Row cache updates do not provide strong exception safety guarantees #15576

bhalevy opened this issue Sep 28, 2023 · 6 comments · Fixed by #15577
Assignees

Comments

@bhalevy
Copy link
Member

bhalevy commented Sep 28, 2023

The row_cache update path uses an external_updater class with two api functions: prepare and execute.
The current design calls for:

  • prepare to create only temporary state, so if it fails, the failure is handled gracefully and the error is propagated to row_cache::update caller leaving no state changes behind (to the row cache nor to the updated table).
  • execute role is to apply the temporary state prepare left behind.

Currently (as of 5.4-dev 652153c), we let errors from execute escape row_cache::update, but there is no guarantee that the execute implementation provides strong exception safety gurantees, i.e. either succeed in whole or be exception safe leaving the state unchanged in case an exception is thrown.

See

scylladb/row_cache.hh

Lines 172 to 182 in 6d34f99

// A function which adds new writes to the underlying mutation source.
// All invocations of external_updater on given cache instance are serialized internally.
// Must have strong exception guarantees. If throws, the underlying mutation source
// must be left in the state in which it was before the call.
class external_updater_impl {
public:
virtual ~external_updater_impl() {}
virtual future<> prepare() { return make_ready_future<>(); }
// FIXME: make execute() noexcept, that will require every updater to make execution exception safe,
// also change function signature.
virtual void execute() = 0;

See also #13937 (comment)

@bhalevy bhalevy self-assigned this Sep 28, 2023
denesb added a commit that referenced this issue Oct 31, 2023
…y Halevy

Currently the cache updaters aren't exception safe
yet they are intended to be.

Instead of allowing exceptions from
`external_updater::execute` escape `row_cache::update`,
abort using `on_fatal_internal_error`.

Future changes should harden all `execute` implementations
to effectively make them `noexcept`, then the pure virtual
definition can be made `noexcept` to cement that.

Fixes #15576

Closes #15577

* github.com:scylladb/scylladb:
  row_cache: abort on exteral_updater::execute errors
  row_cache: do_update: simplify _prev_snapshot_pos setup
avikivity pushed a commit that referenced this issue Oct 31, 2023
…y Halevy

Currently the cache updaters aren't exception safe
yet they are intended to be.

Instead of allowing exceptions from
`external_updater::execute` escape `row_cache::update`,
abort using `on_fatal_internal_error`.

Future changes should harden all `execute` implementations
to effectively make them `noexcept`, then the pure virtual
definition can be made `noexcept` to cement that.

Fixes #15576

Closes #15577

* github.com:scylladb/scylladb:
  row_cache: abort on exteral_updater::execute errors
  row_cache: do_update: simplify _prev_snapshot_pos setup
@bhalevy
Copy link
Member Author

bhalevy commented Oct 31, 2023

Backport should be applicable to all live versions

@mykaul mykaul added backport/5.2 Issues that should be backported to 5.2 branch once they'll be fixed Requires-Backport-to-5.1 backport/5.4 Issues that should be backported to 5.4 branch once they'll be fixed labels Oct 31, 2023
@bhalevy
Copy link
Member Author

bhalevy commented Nov 28, 2023

@scylladb/scylla-maint please backport

@denesb
Copy link
Contributor

denesb commented Nov 29, 2023

There are conflicts, please provide backport PRs. Actually, a single PR against branch-5.4 should be enough, I can try to cherr-pick that backport to the older branches too.

bhalevy pushed a commit to bhalevy/scylla that referenced this issue Dec 1, 2023
…y Halevy

Currently the cache updaters aren't exception safe
yet they are intended to be.

Instead of allowing exceptions from
`external_updater::execute` escape `row_cache::update`,
abort using `on_fatal_internal_error`.

Future changes should harden all `execute` implementations
to effectively make them `noexcept`, then the pure virtual
definition can be made `noexcept` to cement that.

\Fixes scylladb#15576

\Closes scylladb#15577

* github.com:scylladb/scylladb:
  row_cache: abort on exteral_updater::execute errors
  row_cache: do_update: simplify _prev_snapshot_pos setup

(cherry picked from commit 4a0f164)
avikivity pushed a commit that referenced this issue Dec 3, 2023
…y Halevy

Currently the cache updaters aren't exception safe
yet they are intended to be.

Instead of allowing exceptions from
`external_updater::execute` escape `row_cache::update`,
abort using `on_fatal_internal_error`.

Future changes should harden all `execute` implementations
to effectively make them `noexcept`, then the pure virtual
definition can be made `noexcept` to cement that.

\Fixes #15576

\Closes #15577

* github.com:scylladb/scylladb:
  row_cache: abort on exteral_updater::execute errors
  row_cache: do_update: simplify _prev_snapshot_pos setup

(cherry picked from commit 4a0f164)

Closes #16256
denesb added a commit that referenced this issue Dec 7, 2023
…y Halevy

Currently the cache updaters aren't exception safe
yet they are intended to be.

Instead of allowing exceptions from
`external_updater::execute` escape `row_cache::update`,
abort using `on_fatal_internal_error`.

Future changes should harden all `execute` implementations
to effectively make them `noexcept`, then the pure virtual
definition can be made `noexcept` to cement that.

\Fixes #15576

\Closes #15577

* github.com:scylladb/scylladb:
  row_cache: abort on exteral_updater::execute errors
  row_cache: do_update: simplify _prev_snapshot_pos setup

(cherry picked from commit 4a0f164)

Closes #16256
@denesb
Copy link
Contributor

denesb commented Dec 7, 2023

The 5.4 backport PR was good for 5.2 but it doesn't apply to 5.1.

@denesb denesb removed 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 Dec 7, 2023
@mykaul
Copy link
Contributor

mykaul commented Dec 7, 2023

The 5.4 backport PR was good for 5.2 but it doesn't apply to 5.1.

Once 5.4 is released, we only support it and 5.2. There's no need for a 5.1 backport.

@denesb
Copy link
Contributor

denesb commented Dec 7, 2023

@bhalevy please provide a backport for enterprise, if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants