Replacement for base::Passed?

[Andrew Rayskiy]

base::Passed is a legacy helper for capturing move-only types with base::BindRepeating under the assumption that the resulting callback will not be invoked more than once.

Judging by the examples in the codebase, the common scenario is to wrap a move-only type in a RepeatedCallback and pass this callback onward, which raises a question: maybe it makes sense to introduce an adapter similar to base::BarrierClosure that will turn a OnceCallback into a RepeatedCallback with a once-invokation check?

Say, instead of
```
std::unique_ptr<base::Value> value;
base::RepeatedCallback<...> = base::BindRepeating(&PrintValue, base::Passed(&value));
```
 
We'll have
```
std::unique_ptr<base::Value> value;
base::RepeatedCallback<...> = base::InvokeOnceAdapter(base::BindOnce(&PrintValue, std::move(value));
```

I believe that ensuring that the callback is invoked exactly once on the callback level is much clearer than on the argument level. Moreover, a OnceCallback -> RepeatedCallback adapter might be useful in other scenarios too (or maybe there's one already that I don't know about?)

Let me know your thoughts!

[K. Moon]

There's a base::BarrierCallback that takes a configurable number of invocations before the "done" callback is invoked. Is that what you had in mind?

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/e438ba58-f551-43c8-93e3-18636383ca01n%40chromium.org.

[Daniel Cheng]

We previously had a shim for adapting a OnceCallback into a RepeatingCallback that we used when migrating things from base::Callback to base::OnceCallback or base::RepeatingCallback.

In that case, I'm wondering why we can't just use base::OnceCallback directly to make it clear that it is a one-shot callback?

Daniel

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACwGi-6YR2s9-caOjugw-dx2%2BkNCzuuY%3DWPpWEyyvvTxHhzGtQ%40mail.gmail.com.

[dan...@chromium.org]

If you need multiple copies of a OnceCallback - and you will call one of them only - you can use SplitOnceCallback. That was the primary use of converting a OnceCallback to a RepeatingCallback in the past - as a factory for multiple OnceCallbacks.

If you want a RepeatingCallback that calls a OnceCallback once, maybe just wrap it in a RepeatingCallback, but escaping the type system that way sounds less than ideal and I'd think about how to actually use a OnceCallback.

--

[Andrew Rayskiy]

base::BarrierCallback is a bit different -- its purpose is to accumulate the vector of parameters for all callback invocations and then run the underlying OnceCallback passing this vector as an argument, so it's limited to void(T) signatures.

On Thursday, June 2, 2022 at 8:56:49 PM UTC+2 km...@chromium.org wrote:

There's a base::BarrierCallback that takes a configurable number of invocations before the "done" callback is invoked. Is that what you had in mind?

Moving to OnceCallback or using SplitOnceCallback is great (some of the existing cases can definitely be solved this way), but how would it work in case of an unknown amount of receivers? Say, something like this (there's one more BindRepeating down the call chain to make things even more complicated).

[dan...@chromium.org]

On Thu, Jun 2, 2022 at 4:37 PM 'Andrew Rayskiy' via cxx <c...@chromium.org> wrote:

base::BarrierCallback is a bit different -- its purpose is to accumulate the vector of parameters for all callback invocations and then run the underlying OnceCallback passing this vector as an argument, so it's limited to void(T) signatures.

I think BarrierClosure is what was being referred to: https://source.chromium.org/chromium/chromium/src/+/main:base/barrier_closure.h;drc=9f9d8e3255e8b16110602c7c8826f78baab49d89;l=23

We have both and they differ in more ways than do Closure and Callback.

 

On Thursday, June 2, 2022 at 8:56:49 PM UTC+2 km...@chromium.org wrote:

There's a base::BarrierCallback that takes a configurable number of invocations before the "done" callback is invoked. Is that what you had in mind?

Moving to OnceCallback or using SplitOnceCallback is great (some of the existing cases can definitely be solved this way), but how would it work in case of an unknown amount of receivers? Say, something like this (there's one more BindRepeating down the call chain to make things even more complicated).

FWIW you can split more than once, I'm not totally sure if that helps. This link is to passing a OnceCallback in an if/else.

 

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/587d929d-0296-48d9-b49a-b2b93d22a026n%40chromium.org.

[Andrew Rayskiy]

BarrierClosure is also a bit different since it only works for void() signatures, or am I missing something? I'm basically looking for a BarrierClosure that is compatible with void(Args...).

The link is not about if/else -- I was trying to show an example of a function (CloseAllBrowsersWithProfile in this case) that takes a RepeatingCallback with base::Passed (line 1170) and broadcasts it to multiple receivers (only one of which is supposed to invoke it). This particular callback ends up here and cannot be replaced with a OnceCallback according to the description. 

[K. Moon]

I think the straightforward way to do this would be to write your own base::RepeatingCallback that invokes a base::BarrierClosure. I'm guessing this scenario is uncommon enough that there isn't a general utility for it.

[dan...@chromium.org]

On Thu, Jun 2, 2022 at 5:23 PM Andrew Rayskiy <green...@google.com> wrote:

BarrierClosure is also a bit different since it only works for void() signatures, or am I missing something? I'm basically looking for a BarrierClosure that is compatible with void(Args...).

I was expecting that you would need to bind those arguments into the BarrierClosure, so you'd have a void() signature in what you hand to the BarrierClosure (though not necessarily in the fn being run)? Or are they being computed later? I'm not sure how that would look.

But reading your problem statement again I'm not sure that BarrierAnything is what you're looking for. It sounds like you want the _first_ run to do something, and the rest to not?

 

The link is not about if/else -- I was trying to show an example of a function (CloseAllBrowsersWithProfile in this case) that takes a RepeatingCallback with base::Passed (line 1170) and broadcasts it to multiple receivers (only one of which is supposed to invoke it). This particular callback ends up here and cannot be replaced with a OnceCallback according to the description. 

In this case you have a function that can only be called once, and a callback that can be invoked twice. What do you want to happen when the 2nd call happens? It's possible to write a function that makes that decision (crash? drop it?) bound in a RepeatingCallback with the OnceCallback it runs.

For example, this can be called N times but only runs `cb` once:
```

auto r = base::BindRepeating([](OnceClosure& cb) {
  if (cb) { std::move(cb).Run(); }

}, base::Owned(std::make_unique<OnceClosure>(std::move(once_callback))));

```

It's a bit ugly cuz Owned only takes unique_ptr, perhaps that can be improved. Did I correctly understand your idea?

[Andrew Rayskiy]

My idea was to have something like this (pretty much like you wrote, but with some syntax sugar).

I don't own the example that we're currently discussing -- it's something that I found on codesearch :) My point was that simply replacing base::RepeatingCallback + base::Passed with base::OnceCallback that gets split every time is dangerous (since there might be other scenarios/settings for the same function where the callback can be invoked multiple times, who knows?)

[dan...@chromium.org]

On Thu, Jun 2, 2022 at 5:58 PM Andrew Rayskiy <green...@google.com> wrote:

My idea was to have something like this (pretty much like you wrote, but with some syntax sugar).

Makes sense. I think the approach feels like a lot of template complexity for what it's doing. It looks like this would crash on a nullptr on the 2nd attempt to call the OnceCallback.

I don't own the example that we're currently discussing -- it's something that I found on codesearch :) My point was that simply replacing base::RepeatingCallback + base::Passed with base::OnceCallback that gets split every time is dangerous (since there might be other scenarios/settings for the same function where the callback can be invoked multiple times, who knows?)

That depends what you mean by dangerous. If multiple callbacks from a SplitOnceCallback are called, the 2nd will terminate the program.

[Andrew Rayskiy]

Ouch, I just realized that sometimes it's also necessary to pass in additional unbound args :) I created a small CL to illustrate what I mean -- the function in line 1152 (the rest is git cl format) takes two parameters but only one of them is bound initially.

[Daniel Cheng]

We should not provide a built-in way for wrapping a OnceCallback in a RepeatingCallback.

The linked-to example is unusual and difficult to understand, and it's hard to follow how the mixed usage of base::OnceCallback and base::RepeatingCallback is safe here. For example, this thread previously highlighted a comment for the callback type that specifically mentions that this type can't be a OnceCallback because it can be run more than once. But the different callbacks pass through so many different layers that it's difficult to see how each execution path is constrained in a way that guarantees that any OnceCallbacks involved are, in fact, only run once. I don't think an adapter would help improve clarity here.

I wonder if it'd be possible to change TryToCloseBrowserList() so that each call to TryToCloseWindow() returns enough info for its caller to run the appropriate callback directly instead. This would eliminate the need to pass the callback multiple times, and should allow us to use a OnceCallback. For the sake of completeness, a less nice way to do this would be to pass a base::OnceCallback by mutable reference, but we shouldn't do that if we can avoid it.

Finally, if we only care about getting rid of base::Passed(), we can also still do that without introducing any new types. It's quite ugly but the basic idea is to use base::Owned() like another part of this code already does. But I'd strongly prefer to see if there are simple ways to rearrange things so we don't need to fight the type system quite so much.

Daniel

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/7d567b8d-4a04-4a94-a10a-df2641e52d31n%40chromium.org.