Skip to content

Commit

Permalink
Address PR comments
Browse files Browse the repository at this point in the history
  • Loading branch information
RobinTF committed Feb 2, 2024
1 parent 768dd65 commit 40e1b43
Show file tree
Hide file tree
Showing 4 changed files with 10 additions and 13 deletions.
2 changes: 1 addition & 1 deletion src/engine/Server.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -493,7 +493,7 @@ class QueryAlreadyInUseError : public std::runtime_error {

ad_utility::websocket::OwningQueryId Server::getQueryId(
const ad_utility::httpUtils::HttpRequest auto& request,
const std::string& query) {
std::string_view query) {
using ad_utility::websocket::OwningQueryId;
std::string_view queryIdHeader = request.base()["Query-Id"];
if (queryIdHeader.empty()) {
Expand Down
2 changes: 1 addition & 1 deletion src/engine/Server.h
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ class Server {
/// on destruction.
ad_utility::websocket::OwningQueryId getQueryId(
const ad_utility::httpUtils::HttpRequest auto& request,
const std::string& query);
std::string_view query);

/// Schedule a task to trigger the timeout after the `timeLimit`.
/// The returned callback can be used to prevent this task from executing
Expand Down
18 changes: 8 additions & 10 deletions src/util/http/websocket/QueryId.h
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,11 @@ static_assert(!std::is_copy_assignable_v<OwningQueryId>);
/// A factory class to create unique query ids within each individual instance.
class QueryRegistry {
struct CancellationHandleWithQuery {
SharedCancellationHandle cancellationHandle_;
SharedCancellationHandle cancellationHandle_ =
std::make_shared<CancellationHandle<>>();
std::string query_;
explicit CancellationHandleWithQuery(std::string_view query)
: query_{query} {}
};
using SynchronizedType = ad_utility::Synchronized<
ad_utility::HashMap<QueryId, CancellationHandleWithQuery>>;
Expand All @@ -88,18 +91,14 @@ class QueryRegistry {

/// Tries to create a new unique OwningQueryId object from the given string.
/// \param id The id representation of the potential candidate.
/// \param query The string representation of the associated SPARQL query.
/// \return A std::optional<OwningQueryId> object wrapping the passed string
/// if it was not present in the registry before. An empty
/// std::optional if the id already existed before.
std::optional<OwningQueryId> uniqueIdFromString(std::string id,
const std::string& query) {
std::string_view query) {
auto queryId = QueryId::idFromString(std::move(id));
bool success =
registry_->wlock()
->emplace(queryId,
CancellationHandleWithQuery{
std::make_shared<CancellationHandle<>>(), query})
.second;
bool success = registry_->wlock()->try_emplace(queryId, query).second;
if (success) {
// Avoid undefined behavior when the registry is no longer alive at the
// time the `OwningQueryId` is destroyed.
Expand All @@ -119,7 +118,7 @@ class QueryRegistry {

/// Generates a unique pseudo-random OwningQueryId object for this registry
/// and associates it with the given query.
OwningQueryId uniqueId(const std::string& query) {
OwningQueryId uniqueId(std::string_view query) {
static thread_local std::mt19937 generator(std::random_device{}());
std::uniform_int_distribution<uint64_t> distrib{};
std::optional<OwningQueryId> result;
Expand All @@ -133,7 +132,6 @@ class QueryRegistry {
/// of all currently registered queries.
ad_utility::HashMap<QueryId, std::string> getActiveQueries() const {
return registry_->withReadLock([](const auto& map) {
// TODO<C++23> Use `ranges::to` to transform map keys into map
ad_utility::HashMap<QueryId, std::string> result;
result.reserve(map.size());
for (const auto& entry : map) {
Expand Down
1 change: 0 additions & 1 deletion test/QueryIdTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ using ad_utility::websocket::OwningQueryId;
using ad_utility::websocket::QueryId;
using ad_utility::websocket::QueryRegistry;
using ::testing::ContainerEq;
using ::testing::ElementsAre;
using ::testing::IsEmpty;

TEST(QueryId, checkIdEqualityRelation) {
Expand Down

0 comments on commit 40e1b43

Please sign in to comment.