Coroutinize distributed loader

[bhalevy]

Before touching any of this code for #9559,
that requires a change when loading sstables from the staging subdirectory,
simplify it using coroutines.

[scylladb-promoter]

CI state FAILURE - https://jenkins.scylladb.com/job/releng/job/Scylla-CI/441/

[bhalevy]

In v2 (b36d004):

• kept seastar threads where deferred_stop(directory) was required.
  • to keep 'em simple
• fixed premature freeing of func in sstable_directory: parallel_for_each_restricted that causes use-after-free exposed by coroutinizing make_sstables_available
• rebased + retested

[scylladb-promoter]

CI state SUCCESS - https://jenkins.scylladb.com/job/releng/job/Scylla-CI/457/

[bhalevy]

In v3 (b3e2204):

• simplified process_sstable_dir as per Coroutinize distributed loader #10609 (comment)
• kep parallel_for_each in populate_keyspace as per Coroutinize distributed loader #10609 (comment)

[scylladb-promoter]

CI state SUCCESS - https://jenkins.scylladb.com/job/releng/job/Scylla-CI/467/

[bhalevy]

@xemul ping. please re-review.

[xemul]

Erm... I don't insist, but shouldn't it rather look like

try {
    co_await table.add_sstables_and_update_cache();
} catch (...) {
    dblog.error(...);
    abort();
}

?

[bhalevy]

It could, but why throw exception when they can be handled elegantly as above?
I'd use try/catch if the exception handling would have covered a number of statements where we don't care which of them failed and they have common exception handling.

[xemul]

Won't dir go out of scope because of this? There had been several bugs like that already, e.g. see my patch Save coroutine's captured variable on stack (to be fair -- I don't understand this problem completely)

[bhalevy]

dir is passed by reference and is instantiated in a seastar thread in distributed_loader::process_upload_dir
so it doesn't go out of scope.

[bhalevy]

(to be fair -- I don't understand this problem completely)

I think the following explains it well.

https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#cp51-do-not-use-capturing-lambdas-that-are-coroutines

A lambda results in a closure object with storage, often on the stack,
that will go out of scope at some point. When the closure object goes
out of scope the captures will also go out of scope.
Normal lambdas will have finished executing by this time so it is not a problem.
Coroutine lambdas may resume from suspension after the closure object has
destructed and at that point all captures will be use-after-free memory access.

The point is that the lambda object is temporary and goes out of scope when to lambda coroutine suspends.
One way to extend its lifetime is, if the calling function is a coroutine as well, keep the coroutine lambda in an automatic variable and that variable is retained on the calling coroutine stack frame.

I.e. instead of e.g.

    co_await parallel_for_each([captures] () -> future<> { co_await async_call; });

do:

    auto f = [captures] () -> future<> { co_await async_call; };
    co_await parallel_for_each(f);

[avikivity]

Does coroutine::parallel_for_each help?

[bhalevy]

Does coroutine::parallel_for_each help?

Yes, since it moves the Func&& to the coroutine::parallel_for_each object. and so it extends its lifetime until it's fully resolved. but I'm not sure all continuations do that.

Anyhow, it's not required for this particular use case.