Support mutable lambdas in sharded::invoke_on_others

[xemul]

Currently only sharded::invoke_on_all() can call mutable lambdas, the ...on_others fails to compile. This PR fixes it and adds test for more invoke_on... compilation options.

[tchaikov]

left a couple comments.

[tchaikov]

nit, IMHO, the argument is not quite relevant to the problem we are trying to resolve here.

[tchaikov]

also, might want to rename fn to a name which can be distinguished from its non-const cousin. for instance, "access()" versus "mutate()".

and, this function is never used in this test. is this expected?

when the compiler looks up for a function based on its arguments, the function which is resolved to, is not dependent to if the enclosing lambda has the mutable specifier. it depends on, well, the arguments' types.

in this case, if the this argument which is implicitly passed to the argument is const, the const version is used. see https://godbolt.org/z/M77v3Yzbc.

so, i think it'd be better to use different names for const function and for non-const function when testing the mutable lambda and non-mutable lambda.

[xemul]

nit, IMHO, the argument is not quite relevant to the problem we are trying to resolve here.

I need to have calling lambda with capture (below), so I capture int a. In order not to have unused capture, I pass it here, so the argument is indeed unused. I can plug it the other way, if you prefer

[xemul]

and, this function is never used in this test. is this expected?

The non-const fn is in use. The const one isn't, but it's left "for future", see #2278. I can remove it for now

[tchaikov]

I need to have calling lambda with capture (below)

this is why i am confused. what makes it important to capture something in this context? i assume you want to test something. but what is it?

[xemul]

what makes it important to capture something in this context?

Because lambda without capture, even being mutable, compiles just fine without the fix. I do need non-empty capture here. And if I use [a] capture, but don't do anything with a inside lambda, compiler warns me about unused lambda capture. So I pass it as argument to foo::fn(int)

[xemul]

could you please re-read my comment? and probably the linked godbolt example, if you haven't. i commented out the non-const fn, and the sample compiles just fine.

I don't see how it's related. My code is

   srv.invoke_on_all([a] (checker& s) { return s.fn(a); });

srv is non-const, checker& s argument is non-const either, s.fn(a) calls for non-const fn() (if I remove const overload, it compiles just fine)

[xemul]

Maybe this would explain my intent better. I don't try to test if the lambda calls const fn or non const fn depending on whether it's mutable or not. My point is that invoke_on_other() doesn't work with mutable lambdas at all, it just fails to compile like this

/home/xemul/src/seastar/include/seastar/core/future.hh:2074:11: error: no type named ‘type’ in ‘struct std::invoke_result<const invoke_on_modifiers::do_run_test_case() const::<lambda(invoke_on_modifiers::do_run_test_case() const::checker&)>&, invoke_on_modifiers::do_run_test_case() const::checker&>’
 2074 |     using futurator = futurize<std::invoke_result_t<Func, Args&&...>>;
      |           ^~~~~~~~~

[xemul]

I updated the PR with less code in the test. Please, check, hope it demonstrates the problem in a cleaner way

[tchaikov]

what makes it important to capture something in this context?

Because lambda without capture, even being mutable, compiles just fine without the fix. I do need non-empty capture here. And if I use [a] capture, but don't do anything with a inside lambda, compiler warns me about unused lambda capture. So I pass it as argument to foo::fn(int)

thanks. TIL.

[tchaikov]

i'd suggest add a test case which pass a const & to the lambda, like

srv.invoke_on_all([a] (const checker& s) { return s.fn(a); }).get();

[xemul]

It's below, and it's commented out, because it doesn't work currently: #2278

[tchaikov]

it's not below. could you please read the comment again? please.

[xemul]

OK, I missed that you suggest to have const checker&, but not const_cast<const ...>(srv) part. What for?

[tchaikov]

hmm, just for the sake of completeness. but it's not important.

[avikivity]

How can mutable lambdas work? All shards will try to mutate them concurrently.

[xemul]

How can mutable lambdas work? All shards will try to mutate them concurrently.

Code can try to std::move() an argument from it, e.g. scylla/transport/server.cc

    return parallel_for_each(cpus.begin(), cpus.end(), [this, query, cpu_id, &client_state] (unsigned int c) mutable {
        if (c != cpu_id) {
            return smp::submit_to(c, [this, query, &client_state] () mutable {
                return _server._query_processor.local().prepare(std::move(query), client_state, false).discard_result();
            });
        } else {
            return make_ready_future<>();
        }
    })

this call to parallel_for_each(cpus, ...) is in fact _server._query_processor.invoke_on_others(...) one, which cannot compile

[avikivity]

How can mutable lambdas work? All shards will try to mutate them concurrently.

Code can try to std::move() an argument from it, e.g. scylla/transport/server.cc

    return parallel_for_each(cpus.begin(), cpus.end(), [this, query, cpu_id, &client_state] (unsigned int c) mutable {
        if (c != cpu_id) {
            return smp::submit_to(c, [this, query, &client_state] () mutable {
                return _server._query_processor.local().prepare(std::move(query), client_state, false).discard_result();
            });
        } else {
            return make_ready_future<>();
        }
    })

this call to parallel_for_each(cpus, ...) is in fact _server._query_processor.invoke_on_others(...) one, which cannot compile

How can this work? The first iteration will clear query and the second one will work from an empty variable.

[xemul]

How can this work? The first iteration will clear query and the second one will work from an empty variable.

The inner smp::submit_to() copies the lambda capture block including query string.
So does the invoke_on_(all|others)(), they both try to copy it.

[avikivity]

I see. It's confusing, and I feel there is wrongness here. If you pass something mutable, you expect to see the results of the mutation. On the other hand, we do this everywhere, and since it's a lambda, no one can inspect the result of the mutation.

[tchaikov]

lgtm

[xemul]

@avikivity , what's you decision here -- is it wrong to have mutable lambdas in invoke_on_...'s or is it worth it (so this PR needs to be merged)?

[avikivity]

@avikivity , what's you decision here -- is it wrong to have mutable lambdas in invoke_on_...'s or is it worth it (so this PR needs to be merged)?

I think it's even documented that invoke_on works on a copy.