Moving and calling a OnceCallback from a RepeatingCallback

[Matthew Jones]

Hey all,

I have a system that uses OnceCallbacks to return information asynchronously to callers. I'm adding a fallback option which uses an api that demands a RepeatingCallback. The way it is used, the RepeatingCallback will only ever be called once. I found a solution using base::Passed, but this seems to be deprecated (and not advised in general). Is there a way for me to std::move my OnceCallback into a RepeatingCallback and ensure I call it once without writing a custom wrapper?

Thanks in advance,

Matt

[Peter Kasting]

Is there a way to adjust the API to support Once callbacks?

PK

--
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/CAM7y2MOu6-KoOYXHYaNXAnOP28j%3DLihrD5bMy3DmapGkghCh%2Bw%40mail.gmail.com.

[Jeremy Roman]

base::internal::OnceCallbackHolder does this (but you can't use it directly, only via SplitOnceCallback). I don't think there's a general purpose function because it does turn what we can usually detect at compile time into a runtime crash (though perhaps in your case there's no avoiding it; I don't know).

--

[Matthew Jones]

To close the loop here: I was able to solve this using `base::OwnedRef(std::move(callback))` and having the callback parameter be a non-const ref.

[Matthew Jones]

I have an open discussion with the team, but a quick look seems like it would be a pretty significant change. I imagine they'd try something similar but internally to reduce the overhead.

[Joe Mason]

The big question here is what should happen in this code if the RepeatingCallback is called multiple times. It's possible that's expected, and it should just do nothing, in which case you'd want to BindOnce a lambda that invokes your OnceCallback the first time it's called, and then does nothing afterward.

On Fri, Mar 8, 2024 at 1:27 PM Matthew Jones <mdj...@chromium.org> wrote:

--

[Matthew Jones]

Yeah, so I'm most of the way to that suggestion with the slight variation of putting a CHECK in to ensure the callback is valid (which should only be a single time). If my expectation is broken, I'll definitely hear about it.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAH%3DT95QLhFgnEd4HTc2Gc%2BKe7477cuLw%3DcnYMYpJCqpgA_jB3w%40mail.gmail.com.

[Daniel Cheng]

Please don't keep the code in this state.

I've spent a lot of time getting rid of code that uses base::Passed() to cram a move-only object into a repeating callback (crbug.com/40840557), and using a bespoke way to workaround this and make a RepeatingCallback that is only runnable once is not something we should leave in the codebase.

Daniel

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAM7y2MOAMBw1bFpL4_rnvXZm7t%2BcbdoMqp533KZ23FtAfnAOrw%40mail.gmail.com.

[David Benjamin]

+1. If we care that the callback is only called once, it should be a OnceCallback. That's the whole point of the OnceCallback type.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF3XrKoyGg2zgaKB5TtXEBf7RZ%3DwcBc%3D4vdqoA6nEY5qxpL4dA%40mail.gmail.com.

[Peter Kasting]

On Fri, Mar 8, 2024 at 10:52 AM Matthew Jones <mdj...@chromium.org> wrote:

I have an open discussion with the team, but a quick look seems like it would be a pretty significant change.

If there are fixable reasons this would be "a pretty significant change", I'd love to learn if we can improve something. E.g. need different base:: APIs, team should have used a type alias, we're trying to do something fundamentally different with this team's API than it was designed for, the API should never have used Repeating to begin with, etc.

PK 

[Matthew Jones]

The backing tech that we use (optimization guide) for our api (commerce) is designed to fetch information for a list of urls. In the specific instance where this arises, we only ever provide a single url and therefore expect a single response. I should also note that this addition is a supplemental fallback option for an existing OnceCallback api. We could require a RepeatingCallback to our commerce api, but that doesn't seem right given its intent to only ever be called once. I'd be interested to know if there's a way to represent this without requiring optimization guide to implement a OnceCallback version of what we're using.

To summarize I have something similar to the following:

• GetInfoForUrl(const GURL& url, OnceCallback callback)
• GetInfo(const GURL& url, OnceCallback callback)
• The API that's primarily used most of the time
• GetInfoOnDemand(std::vector<const GURL>, RepeatingCallback callback)
• The fallback option that makes a net request if the above can't provide info

- Matt

[Andrew Rayskiy]

I'm sincerely surprised nobody has mentioned BarrierCallback with num_callbacks = 1... :D

[Peter Kasting]

Let me make sure I got that:

• You provide callers an API that takes a GURL and a OnceCallback to call (either sync or async) when info is available.
• In turn you implement via APIs that provide both:
• A single-URL, OnceCallback version that might not always succeed
• A vector-of-URLs, RepeatingCallback version that calls its callback once per URL, which should only be used as fallback

Can you have multiple in-flight requests from your callers? (e.g. someone asks for info, you fall back to a slow path, meanwhile someone else asks for more info)

Can you potentially be destroyed while you have outstanding requests? If so, how does cancellation of any outstanding underlying requests work?

The design I'd reach for depending on the above would likely be some kind of lambda or member to provide in the vector case, which expects the response to correspond to a valid, unsatisfied client call, and calls its associated callback accordingly.

PK

[Matthew Jones]

> I'm sincerely surprised nobody has mentioned BarrierCallback with num_callbacks = 1... :D

I think this would work, but I'm getting the sense that it might not be the desired option from the rest of the conversation.

> Let me make sure I got that:...

Yeah, it sounds like you've got it correctly.

> Can you have multiple in-flight requests from your callers?

It's possible though I'm not sure how frequently this would happen - it's likely to be rare based on usage by our features.

> Can you potentially be destroyed while you have outstanding requests?

Yes, but I believe this is addressed by the use of weak pointers in our callback code.

IIUC, the suggestion would be to maintain a set of callbacks that are associated with each url we query for. If that's the case, I was trying to avoid the state management that comes with that, it could also get tricky if multiple clients request for the same url.

- Matt

--

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/CAAHOzFC8rfkrykXLA6yDODxQUNTCN_5_1MuQpPKcsB2jWM9Btg%40mail.gmail.com.

[Peter Kasting]

On Tue, Mar 19, 2024 at 1:18 PM Matthew Jones <mdj...@chromium.org> wrote:

> Can you have multiple in-flight requests from your callers?

It's possible though I'm not sure how frequently this would happen - it's likely to be rare based on usage by our features.

OK. That suggests to me that we don't need to be paranoid about CPU/memory overhead of any tracking we do.

IIUC, the suggestion would be to maintain a set of callbacks that are associated with each url we query for. If that's the case, I was trying to avoid the state management that comes with that, it could also get tricky if multiple clients request for the same url.

Yes, you'd need something like a multimap<url, OnceCallback>. I would hope managing it would be straightforward, but I don't know what sort of state you need to deal with.

PK

[Joe Mason]

I thought I had, but I guess I decided it didn't fit here... The problem is BarrierCallback<void(T)> ends up calling a OnceCallback<vector<T>> so you'd get a vector of length 1, and still need a conversion step. Which isn't any less complex than using a RepeatingCallback directly.

--

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/82117784-1443-4fb6-adbd-f9d4feccc024n%40chromium.org.

[Joe Mason]

Actually, it sounds like BarrierCallback would be very natural for your situation, because it fits the GetInfoOnDemand semantics:

In the general case. A caller passing a vector of URLs of length N, and the response gets called once for each URL. So a BarrierCallback will get back a vector of length N, with responses for all the URL's, to deal with all at once. It's trading off responsiveness for coding simplicity, by waiting for all the responses to arrive instead of dealing with each response as it comes.

In your specific case, you happen to always be passing 1 url, so you can CHECK that the vector you get out of the response callback has length 1, and then pass its contents to your regular callback. 

If you view the BarrierCallback as a general adapter for GetInfoOnDemand, it's obvious how it fits the API so won't need a complicated discussion of OnceCallback vs RepeatingCallback semantics. And the layer that calls BarrierCallback with N=1 url, because you only have 1 url to query, is also obvious.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAM7y2MN49pqb2-uKXxHqGRh3xRrGf7wDMesfBV0%3D7XWYuLvaDg%40mail.gmail.com.

[Matthew Jones]

I do like the BarrierCallback idea, so if it's not particularly offensive to anyone on this thread I think I'll update my code to do that.