Skip to content

[k2] fix TlRpcError::try_fetch #1518

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.

Sign up for GitHub

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

Open
Shamzik wants to merge 4 commits into
base: master
Choose a base branch
from
+213 −95
Open

[k2] fix TlRpcError::try_fetch #1518

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
667de14
fix TlRpcError::try_fetch
Shamzik Feb 9, 2026
9ee7384
rewrite whole TlRpcError::try_fetch
Shamzik Feb 10, 2026
1954ab8
tl::ReqResult & tl::RpcReqResult
Shamzik Feb 10, 2026
d25f514
fix includes ordering
Shamzik Feb 10, 2026
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
69 changes: 35 additions & 34 deletions common/tl/constants/common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,40 +6,41 @@

/* Autogenerated from common.tl and left only used constants */
#pragma once
inline constexpr uint32_t TL__ = 0x840e0eccU;
Copy link
Contributor

@apolyakov apolyakov Feb 10, 2026

Choose a reason for hiding this comment

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

Let's not include these changes as they are not mandatory

inline constexpr uint32_t TL_BOOL_FALSE = 0xbc799737U;
inline constexpr uint32_t TL_BOOL_STAT = 0x92cbcbfaU;
inline constexpr uint32_t TL_BOOL_TRUE = 0x997275b5U;
inline constexpr uint32_t TL_DICTIONARY = 0x1f4c618fU;
inline constexpr uint32_t TL_DICTIONARY_FIELD = 0x239c1b62U;
inline constexpr uint32_t TL_DOUBLE = 0x2210c154U;
inline constexpr uint32_t TL_FLOAT = 0x824dab22U;
inline constexpr uint32_t TL_INT = 0xa8509bdaU;
inline constexpr uint32_t TL_INT_KEY_DICTIONARY = 0x07bafc42U;
inline constexpr uint32_t TL_INT_KEY_DICTIONARY_FIELD = 0x721ea8b9U;
inline constexpr uint32_t TL_LEFT = 0x0a29cd5dU;
inline constexpr uint32_t TL_LONG = 0x22076cbaU;
inline constexpr uint32_t TL_LONG_KEY_DICTIONARY = 0xb424d8f1U;
inline constexpr uint32_t TL_REQ_RESULT_HEADER = 0x8cc84ce1U;
inline constexpr uint32_t TL_MAYBE_FALSE = 0x27930a7bU;
inline constexpr uint32_t TL_MAYBE_TRUE = 0x3f9c8ef8U;
inline constexpr uint32_t TL_RPC_DEST_ACTOR = 0x7568aabdU;
inline constexpr uint32_t TL_RPC_DEST_ACTOR_FLAGS = 0xf0a5acf7U;
inline constexpr uint32_t TL_RPC_DEST_FLAGS = 0xe352035eU;
inline constexpr uint32_t TL_RPC_INVOKE_REQ = 0x2374df3dU;
inline constexpr uint32_t TL_RPC_INVOKE_REQ_EXTRA = 0xf3ef81a9U;
inline constexpr uint32_t TL_RPC_PING = 0x5730a2dfU;
inline constexpr uint32_t TL_RPC_PONG = 0x8430eaa7U;
inline constexpr uint32_t TL_RPC_REQ_ERROR = 0x7ae432f5U;
inline constexpr uint32_t TL_RPC_REQ_RESULT = 0x63aeda4eU;
inline constexpr uint32_t TL_RPC_REQ_RESULT_EXTRA = 0xc5011709U;
inline constexpr uint32_t TL_STAT = 0x9d56e6b2U;
inline constexpr uint32_t TL_STRING = 0xb5286e24U;
inline constexpr uint32_t TL_TRUE = 0x3fedd339U;
inline constexpr uint32_t TL_TUPLE = 0x9770768aU;
inline constexpr uint32_t TL_VECTOR = 0x1cb5c415U;
inline constexpr uint32_t TL_VECTOR_TOTAL = 0x10133f47U;
inline constexpr uint32_t TL_ZERO = 0x00000000U;
inline constexpr uint32_t TL__{0x840e0eccU};
inline constexpr uint32_t TL_BOOL_FALSE{0xbc799737U};
inline constexpr uint32_t TL_BOOL_STAT{0x92cbcbfaU};
inline constexpr uint32_t TL_BOOL_TRUE{0x997275b5U};
inline constexpr uint32_t TL_DICTIONARY{0x1f4c618fU};
inline constexpr uint32_t TL_DICTIONARY_FIELD{0x239c1b62U};
inline constexpr uint32_t TL_DOUBLE{0x2210c154U};
inline constexpr uint32_t TL_FLOAT{0x824dab22U};
inline constexpr uint32_t TL_INT{0xa8509bdaU};
inline constexpr uint32_t TL_INT_KEY_DICTIONARY{0x07bafc42U};
inline constexpr uint32_t TL_INT_KEY_DICTIONARY_FIELD{0x721ea8b9U};
inline constexpr uint32_t TL_LEFT{0x0a29cd5dU};
inline constexpr uint32_t TL_LONG{0x22076cbaU};
inline constexpr uint32_t TL_LONG_KEY_DICTIONARY{0xb424d8f1U};
inline constexpr uint32_t TL_REQ_ERROR{0xb527877dU};
inline constexpr uint32_t TL_REQ_RESULT_HEADER{0x8cc84ce1U};
inline constexpr uint32_t TL_MAYBE_FALSE{0x27930a7bU};
inline constexpr uint32_t TL_MAYBE_TRUE{0x3f9c8ef8U};
inline constexpr uint32_t TL_RPC_DEST_ACTOR{0x7568aabdU};
inline constexpr uint32_t TL_RPC_DEST_ACTOR_FLAGS{0xf0a5acf7U};
inline constexpr uint32_t TL_RPC_DEST_FLAGS{0xe352035eU};
inline constexpr uint32_t TL_RPC_INVOKE_REQ{0x2374df3dU};
inline constexpr uint32_t TL_RPC_INVOKE_REQ_EXTRA{0xf3ef81a9U};
inline constexpr uint32_t TL_RPC_PING{0x5730a2dfU};
inline constexpr uint32_t TL_RPC_PONG{0x8430eaa7U};
inline constexpr uint32_t TL_RPC_REQ_ERROR{0x7ae432f5U};
inline constexpr uint32_t TL_RPC_REQ_RESULT{0x63aeda4eU};
inline constexpr uint32_t TL_RPC_REQ_RESULT_EXTRA{0xc5011709U};
inline constexpr uint32_t TL_STAT{0x9d56e6b2U};
inline constexpr uint32_t TL_STRING{0xb5286e24U};
inline constexpr uint32_t TL_TRUE{0x3fedd339U};
inline constexpr uint32_t TL_TUPLE{0x9770768aU};
inline constexpr uint32_t TL_VECTOR{0x1cb5c415U};
inline constexpr uint32_t TL_VECTOR_TOTAL{0x10133f47U};
inline constexpr uint32_t TL_ZERO{0x00000000U};

namespace vk {
namespace tl {
Expand Down
75 changes: 17 additions & 58 deletions runtime-light/stdlib/rpc/rpc-tl-error.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,68 +4,27 @@

#include "runtime-light/stdlib/rpc/rpc-tl-error.h"

#include <tuple>
#include <variant>

#include "common/tl/constants/common.h"
#include "runtime-light/stdlib/diagnostics/exception-functions.h"
#include "runtime-light/stdlib/rpc/rpc-api.h"
#include "runtime-light/stdlib/rpc/rpc-tl-builtins.h"
#include "runtime-light/server/rpc/rpc-server-state.h"
#include "runtime-light/tl/tl-core.h"
#include "runtime-light/tl/tl-types.h"

bool TlRpcError::try_fetch() noexcept {
const auto backup_pos{tl_parse_save_pos()};
auto op{TRY_CALL(decltype(f$fetch_int()), bool, f$fetch_int())};
if (op == TL_REQ_RESULT_HEADER) {
fetch_and_skip_header();
op = TRY_CALL(decltype(f$fetch_int()), bool, f$fetch_int());
}
if (op != TL_RPC_REQ_ERROR) {
tl_parse_restore_pos(backup_pos);
auto fetcher{RpcServerInstanceState::get().tl_fetcher};
Copy link
Contributor

@apolyakov apolyakov Feb 10, 2026

Choose a reason for hiding this comment

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

It looks like copying the fetcher here is an error

const auto backup_pos{fetcher.pos()};
tl::ReqResult req_result;
Copy link
Contributor

@apolyakov apolyakov Feb 10, 2026

Choose a reason for hiding this comment

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

Same comment here about init

nit: it's better to hide variables in scope unless they are used at the function level

if (tl::ReqResult req_result{}; ...)

if (req_result.fetch(fetcher) && std::holds_alternative<tl::reqResultHeader>(req_result.value)) {
fetcher = tl::fetcher{std::get<tl::reqResultHeader>(req_result.value).result};
} else {
fetcher.reset(backup_pos);
}
tl::RpcReqResult rpc_req_result;
if (!rpc_req_result.fetch(fetcher) || !std::holds_alternative<tl::rpcReqError>(rpc_req_result.value)) {
return false;
}

std::ignore = TRY_CALL(decltype(f$fetch_long()), bool, f$fetch_long());
error_code = static_cast<int32_t>(TRY_CALL(decltype(f$fetch_int()), bool, f$fetch_int()));
error_msg = TRY_CALL(decltype(f$fetch_string()), bool, f$fetch_string());
auto& rpc_req_error{std::get<tl::rpcReqError>(rpc_req_result.value)};
error_code = rpc_req_error.error_code.value;
error_msg = {rpc_req_error.error.value.data(), static_cast<string::size_type>(rpc_req_error.error.value.size())};
return true;
}

void TlRpcError::fetch_and_skip_header() const noexcept {
const auto flags{static_cast<int32_t>(TRY_CALL(decltype(f$fetch_int()), void, f$fetch_int()))};

if (flags & vk::tl::common::rpc_req_result_extra_flags::binlog_pos) {
std::ignore = TRY_CALL(decltype(f$fetch_long()), void, f$fetch_long());
}
if (flags & vk::tl::common::rpc_req_result_extra_flags::binlog_time) {
std::ignore = TRY_CALL(decltype(f$fetch_long()), void, f$fetch_long());
}
if (flags & vk::tl::common::rpc_req_result_extra_flags::engine_pid) {
std::ignore = TRY_CALL(decltype(f$fetch_int()), void, f$fetch_int());
std::ignore = TRY_CALL(decltype(f$fetch_int()), void, f$fetch_int());
std::ignore = TRY_CALL(decltype(f$fetch_int()), void, f$fetch_int());
}
if (flags & vk::tl::common::rpc_req_result_extra_flags::request_size) {
std::ignore = TRY_CALL(decltype(f$fetch_int()), void, f$fetch_int());
}
if (flags & vk::tl::common::rpc_req_result_extra_flags::response_size) {
std::ignore = TRY_CALL(decltype(f$fetch_int()), void, f$fetch_int());
}
if (flags & vk::tl::common::rpc_req_result_extra_flags::failed_subqueries) {
std::ignore = TRY_CALL(decltype(f$fetch_int()), void, f$fetch_int());
}
if (flags & vk::tl::common::rpc_req_result_extra_flags::compression_version) {
std::ignore = TRY_CALL(decltype(f$fetch_int()), void, f$fetch_int());
}
if (flags & vk::tl::common::rpc_req_result_extra_flags::stats) {
const auto size{TRY_CALL(decltype(f$fetch_int()), void, f$fetch_int())};
for (auto i = 0; i < size; ++i) {
std::ignore = TRY_CALL(decltype(f$fetch_int()), void, f$fetch_int());
std::ignore = TRY_CALL(decltype(f$fetch_int()), void, f$fetch_int());
}
}
if (flags & vk::tl::common::rpc_req_result_extra_flags::epoch_number) {
std::ignore = TRY_CALL(decltype(f$fetch_long()), void, f$fetch_long());
}
if (flags & vk::tl::common::rpc_req_result_extra_flags::view_number) {
std::ignore = TRY_CALL(decltype(f$fetch_long()), void, f$fetch_long());
}
}
3 changes: 0 additions & 3 deletions runtime-light/stdlib/rpc/rpc-tl-error.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,6 @@ struct TlRpcError {
}

bool try_fetch() noexcept;

private:
void fetch_and_skip_header() const noexcept;
};

class RpcErrorFactory {
Expand Down
82 changes: 82 additions & 0 deletions runtime-light/tl/tl-types.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,58 @@ tl::mask rpcInvokeReqExtra::get_flags() const noexcept {
return flags;
}

bool rpcReqResultExtra::fetch(tl::fetcher& tlf, const tl::mask& flags) noexcept {
if (static_cast<bool>(flags.value & BINLOG_POS_FLAG)) {
Copy link
Contributor

@apolyakov apolyakov Feb 10, 2026

Choose a reason for hiding this comment

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

What do you think about following implementation?

  bool ok{true};
  if (ok && static_cast<bool>(flags.value & BINLOG_POS_FLAG)) {
    ok = ok && binlog_pos.emplace().fetch(tlf);
  }
   ...

P.S. we need to change types of rpcReqResultExtra's fields to optional types. Take a look at rpcInvokeReqExtra for an example

if (!binlog_pos.fetch(tlf)) [[unlikely]] {
return false;
}
}
if (static_cast<bool>(flags.value & BINLOG_TIME_FLAG)) {
if (!binlog_time.fetch(tlf)) [[unlikely]] {
return false;
}
}
if (static_cast<bool>(flags.value) & ENGINE_PID_FLAG) {
if (!engine_pid.fetch(tlf)) [[unlikely]] {
return false;
}
}
if (static_cast<bool>(flags.value & REQUEST_SIZE_FLAG)) {
kphp::log::assertion(static_cast<bool>(flags.value & RESPONSE_SIZE_FLAG));
if (!request_size.fetch(tlf)) [[unlikely]] {
return false;
}
if (!response_size.fetch(tlf)) [[unlikely]] {
return false;
}
}
if (static_cast<bool>(flags.value & FAILED_SUBQUERIES_FLAG)) {
if (!failed_subqueries.fetch(tlf)) [[unlikely]] {
return false;
}
}
if (static_cast<bool>(flags.value & COMPRESSION_VERSION_FLAG)) {
if (!compression_version.fetch(tlf)) [[unlikely]] {
return false;
}
}
if (static_cast<bool>(flags.value & STATS_FLAG)) {
if (!stats.fetch(tlf)) [[unlikely]] {
return false;
}
}
if (static_cast<bool>(flags.value & EPOCH_NUMBER_FLAG)) {
kphp::log::assertion(static_cast<bool>(flags.value & VIEW_NUMBER_FLAG));
if (!epoch_number.fetch(tlf)) [[unlikely]] {
return false;
}
if (!view_number.fetch(tlf)) [[unlikely]] {
return false;
}
}
return true;
}

void rpcReqResultExtra::store(tl::storer& tls, const tl::mask& flags) const noexcept {
if (static_cast<bool>(flags.value & BINLOG_POS_FLAG)) {
binlog_pos.store(tls);
Expand Down Expand Up @@ -278,6 +330,36 @@ size_t rpcReqResultExtra::footprint(const tl::mask& flags) const noexcept {
return footprint;
}

bool ReqResult::fetch(tl::fetcher& tlf) noexcept {
Copy link
Contributor

@apolyakov apolyakov Feb 10, 2026

Choose a reason for hiding this comment

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

Same comment here about different implementation approach

const auto backup_pos{tlf.pos()};
tl::magic magic;
if (!magic.fetch(tlf)) [[unlikely]] {
return false;
}
switch (magic.value) {
case TL_REQ_ERROR: {
tl::reqError req_error;
if (!req_error.fetch(tlf)) [[unlikely]] {
return false;
}
value = req_error;
break;
}
case TL_REQ_RESULT_HEADER: {
tl::reqResultHeader req_result_header;
if (!req_result_header.fetch(tlf)) [[unlikely]] {
return false;
}
value = req_result_header;
break;
}
default:
tlf.reset(backup_pos);
value = *tlf.fetch_bytes(tlf.remaining());
}
return true;
}

} // namespace tl

namespace tl2 {
Expand Down
79 changes: 79 additions & 0 deletions runtime-light/tl/tl-types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1153,6 +1153,7 @@ class rpcReqResultExtra final {
tl::i64 epoch_number{};
tl::i64 view_number{};

bool fetch(tl::fetcher& tlf, const tl::mask& flags) noexcept;
void store(tl::storer& tls, const tl::mask& flags) const noexcept;

size_t footprint(const tl::mask& flags) const noexcept;
Expand All @@ -1166,6 +1167,84 @@ struct RpcReqResultExtra final {
}
};

struct reqError final {
tl::i32 error_code{};
tl::string error{};

bool fetch(tl::fetcher& tlf) noexcept {
return error_code.fetch(tlf) && error.fetch(tlf);
}
};

struct reqResultHeader final {
tl::mask flags{};
tl::rpcReqResultExtra extra{};
std::span<const std::byte> result;

bool fetch(tl::fetcher& tlf) noexcept {
if (!flags.fetch(tlf) || !extra.fetch(tlf, flags)) [[unlikely]] {
return false;
}
result = *tlf.fetch_bytes(tlf.remaining());
Copy link
Contributor

@apolyakov apolyakov Feb 10, 2026

Choose a reason for hiding this comment

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

Let's not skip check for the result of fetch_bytes

return true;
}
};

struct ReqResult final {
std::variant<tl::reqError, tl::reqResultHeader, std::span<const std::byte>> value;

bool fetch(tl::fetcher& tlf) noexcept;
};

struct rpcReqResult final {
tl::ReqResult result{};

bool fetch(tl::fetcher& tlf) noexcept {
return tl::i64{}.fetch(tlf) && result.fetch(tlf);
Copy link
Contributor

@apolyakov apolyakov Feb 10, 2026

Choose a reason for hiding this comment

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

Why do you skip query_id?

}
};

struct rpcReqError final {
tl::i32 error_code{};
tl::string error{};

bool fetch(tl::fetcher& tlf) noexcept {
return tl::i64{}.fetch(tlf) && error_code.fetch(tlf) && error.fetch(tlf);
Copy link
Contributor

@apolyakov apolyakov Feb 10, 2026

Choose a reason for hiding this comment

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

Same for query_id here

}
};

struct RpcReqResult final {
std::variant<tl::rpcReqResult, tl::rpcReqError> value;

bool fetch(tl::fetcher& tlf) noexcept {
Copy link
Contributor

@apolyakov apolyakov Feb 10, 2026

Choose a reason for hiding this comment

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

What do you think about following implementation?

  bool fetch(tl::fetcher& tlf) noexcept {
    tl::magic magic{};
    bool ok{magic.fetch(tlf)};
    switch (magic.value) {
    case TL_RPC_REQ_RESULT:
      ok = ok && value.emplace<tl::rpcReqResult>().fetch(tlf);
      break;
    case TL_RPC_REQ_ERROR:
      ok = ok && value.emplace<tl::rpcReqError>().fetch(tlf);
      break;
    default:
      ok = false;
      break;
    }
    return ok;
  }
}

tl::magic magic;
if (!magic.fetch(tlf)) [[unlikely]] {
return false;
}
switch (magic.value) {
case TL_RPC_REQ_RESULT: {
tl::rpcReqResult rpc_req_result;
Copy link
Contributor

@apolyakov apolyakov Feb 10, 2026

Choose a reason for hiding this comment

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

Please, don't leave local variables of types tl:: uninitialized

if (!rpc_req_result.fetch(tlf)) [[unlikely]] {
return false;
}
value = rpc_req_result;
break;
}
case TL_RPC_REQ_ERROR: {
tl::rpcReqError rpc_req_error;
if (!rpc_req_error.fetch(tlf)) [[unlikely]] {
return false;
}
value = rpc_req_error;
break;
}
default:
return false;
}
return true;
}
};

struct k2RpcResponseError final {
tl::i32 error_code{};
tl::string error{};
Expand Down
Loading