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

[Brave News]: Build feed off the main thread #21833

Merged
merged 18 commits into from
Feb 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions components/api_request_helper/api_request_helper.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ class APIRequestResult {
const std::string& body() const { return body_; }
// `base::Value` of sanitized json response.
const base::Value& value_body() const { return value_body_; }
base::Value& value_body() { return value_body_; }
// HTTP response headers.
const base::flat_map<std::string, std::string>& headers() const {
return headers_;
Expand Down
3 changes: 2 additions & 1 deletion components/brave_news/browser/DEPS
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
include_rules = [
"+net",
"+content/public/browser/browser_thread.h",
"+services/network/public",
"+services/network/public/mojom",
"+third_party/re2",
]
]
117 changes: 79 additions & 38 deletions components/brave_news/browser/feed_fetcher.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@

#include <cstddef>
#include <iterator>
#include <memory>
#include <string>
#include <tuple>
#include <unordered_set>
#include <utility>
#include <vector>

Expand All @@ -17,8 +19,9 @@
#include "base/functional/callback_forward.h"
#include "base/location.h"
#include "base/memory/weak_ptr.h"
#include "base/one_shot_event.h"
#include "base/ranges/algorithm.h"
#include "base/task/thread_pool.h"
#include "base/values.h"
#include "brave/components/api_request_helper/api_request_helper.h"
#include "brave/components/brave_news/browser/channels_controller.h"
#include "brave/components/brave_news/browser/combined_feed_parsing.h"
Expand Down Expand Up @@ -56,6 +59,51 @@ FeedFetcher::FeedSourceResult::~FeedSourceResult() = default;
FeedFetcher::FeedSourceResult::FeedSourceResult(
FeedFetcher::FeedSourceResult&&) = default;

// static
std::tuple<FeedItems, ETags> FeedFetcher::CombineFeedSourceResults(
std::vector<FeedSourceResult> results) {
std::size_t total_size = 0;
for (const auto& result : results) {
total_size += result.items.size();
}
VLOG(1) << "All feed item fetches done with item count: " << total_size;

ETags etags;
FeedItems feed;
feed.reserve(total_size);

// We want to deduplicate the feed, as the feeds for different
// regions **may** have overlap.
std::unordered_set<std::string> seen;

// reserve |total_size| space in |seen|. This is more than we'll
// likely need but should be in the correct ballpark.
seen.reserve(total_size);

for (auto& result : results) {
etags[result.key] = result.etag;
for (auto& item : result.items) {
GURL url;
if (item->is_article()) {
url = item->get_article()->data->url;
} else if (item->is_promoted_article()) {
url = item->get_promoted_article()->data->url;
}

// Skip this, we've already seen it.
auto spec = url.spec();
if (!url.is_empty() && seen.contains(spec)) {
continue;
}
seen.insert(std::move(spec));

feed.push_back(std::move(item));
}
}

return std::make_tuple(std::move(feed), std::move(etags));
}

FeedFetcher::FeedFetcher(
PublishersController& publishers_controller,
ChannelsController& channels_controller,
Expand Down Expand Up @@ -152,47 +200,40 @@ void FeedFetcher::OnFetchFeedFetchedFeed(
return;
}

std::move(callback).Run({locale, etag, ParseFeedItems(result.value_body())});
base::ThreadPool::PostTaskAndReplyWithResult(
FROM_HERE,
base::BindOnce(&ParseFeedItems, std::move(result.value_body())),
base::BindOnce(
[](base::WeakPtr<FeedFetcher> fetcher, std::string locale,
std::string etag, FetchFeedSourceCallback callback,
std::vector<mojom::FeedItemPtr> items) {
// If the fetcher was destroyed, don't run the callback.
if (!fetcher) {
return;
}
std::move(callback).Run(
{std::move(locale), std::move(etag), std::move(items)});
},
weak_ptr_factory_.GetWeakPtr(), std::move(locale), std::move(etag),
std::move(callback)));
}

void FeedFetcher::OnFetchFeedFetchedAll(FetchFeedCallback callback,
Publishers publishers,
std::vector<FeedSourceResult> results) {
std::size_t total_size = 0;
for (const auto& result : results) {
total_size += result.items.size();
}
VLOG(1) << "All feed item fetches done with item count: " << total_size;

ETags etags;
FeedItems feed;
feed.reserve(total_size);

// We want to deduplicate the feed, as the feeds for different regions **may**
// have overlap.
base::flat_set<GURL> seen;

for (auto& result : results) {
etags[result.key] = result.etag;
for (auto& item : result.items) {
GURL url;
if (item->is_article()) {
url = item->get_article()->data->url;
} else if (item->is_promoted_article()) {
url = item->get_promoted_article()->data->url;
}

// Skip this, we've already seen it.
if (!url.is_empty() && seen.contains(url)) {
continue;
}
seen.insert(url);

feed.push_back(std::move(item));
}
}

std::move(callback).Run(std::move(feed), std::move(etags));
base::ThreadPool::PostTaskAndReplyWithResult(
FROM_HERE, base::BindOnce(&CombineFeedSourceResults, std::move(results)),
base::BindOnce(
[](base::WeakPtr<FeedFetcher> fetcher, FetchFeedCallback callback,
std::tuple<FeedItems, ETags> result) {
// If we've been destroyed, don't run the callback.
if (!fetcher) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking fetcher makes sense. Although, the lifetime of the callback could be different.
Frankly, I can't 100% sure that we don't add new crashes because posting the callback.

The code use a very callback-based approach. It's really hard to surely find all the entry points and make a conclusion that it's still safe..

Copy link
Collaborator

@atuchin-m atuchin-m Feb 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im other words, we changed the public contract of the class. Before we run all the callbacks sync, now we could postpone. A callback could be bound not only to FeedFetcher but to a mojo-related interface (to send the answer) or something else. Better to double check all the callers. Or even make renaming DoSomething(..) => DoSomethingAsync(..)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we have changed the the public contract of the class - the callback was never invoked synchronously, only ever after all the combined feeds and direct feeds are fetched (quite apart from the network request, JSON parsing of the combined feed potentially happens in another process).

I added the check that fetcher wasn't destroyed because it was the only place I could think of where the contract was changing (previously the callback wouldn't be invoked if the FeedFetcher was destroyed, and the WeakPtr check maintains that guarantee).

return;
}
std::move(callback).Run(std::move(std::get<0>(result)),
std::move(std::get<1>(result)));
},
weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
}

void FeedFetcher::IsUpdateAvailable(ETags etags,
Expand Down
3 changes: 3 additions & 0 deletions components/brave_news/browser/feed_fetcher.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

#include <memory>
#include <string>
#include <tuple>
#include <vector>

#include "base/containers/flat_map.h"
Expand Down Expand Up @@ -55,6 +56,8 @@ class FeedFetcher {
};
using FetchFeedSourceCallback =
base::OnceCallback<void(FeedSourceResult items)>;
static std::tuple<FeedItems, ETags> CombineFeedSourceResults(
std::vector<FeedSourceResult> results);

// Steps for |FetchFeed|
void OnFetchFeedFetchedPublishers(FetchFeedCallback callback,
Expand Down
Loading
Loading