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.

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

Jump to bottom

JSON-RPC message handling blocks sending own messages #10268

Open
Al2Klimov opened this issue Dec 11, 2024 · 0 comments · May be fixed by #10273
Open

JSON-RPC message handling blocks sending own messages #10268

Al2Klimov opened this issue Dec 11, 2024 · 0 comments · May be fixed by #10273
Labels
area/distributed Distributed monitoring (master, satellites, clients) bug Something isn't working ref/NC

Comments

@Al2Klimov
Copy link
Member

Al2Klimov commented Dec 11, 2024 •
edited
Loading

Describe the bug

Everything per each JSON-RPC connection runs mutually exclusive.

icinga2/lib/remote/jsonrpcconnection.cpp

Lines 55 to 58 in e50eb52

IoEngine::SpawnCoroutine(m_IoStrand, [this, keepAlive](asio::yield_context yc) { HandleIncomingMessages(yc); });
IoEngine::SpawnCoroutine(m_IoStrand, [this, keepAlive](asio::yield_context yc) { WriteOutgoingMessages(yc); });
IoEngine::SpawnCoroutine(m_IoStrand, [this, keepAlive](asio::yield_context yc) { HandleAndWriteHeartbeats(yc); });
IoEngine::SpawnCoroutine(m_IoStrand, [this, keepAlive](asio::yield_context yc) { CheckLiveness(yc); });

Something, like I/O itself, is async. Something, like queueing I/O, is pretty quick. But message handling itself is the slowest part and blocks the entire strand.

To Reproduce

icinga2/lib/remote/jsonrpcconnection.cpp

Lines 99 to 104 in e50eb52

Dictionary::Ptr message = JsonRpc::DecodeMessage(jsonString);
if (String method = message->Get("method"); !method.IsEmpty()) {
rpcMethod = std::move(method);
}
MessageHandler(message);

Expected behavior

JsonRpcConnection should have a second strand. The following 33 lines should be a callback posted to the new strand.

icinga2/lib/remote/jsonrpcconnection.cpp

Lines 99 to 131 in e50eb52

Dictionary::Ptr message = JsonRpc::DecodeMessage(jsonString);
if (String method = message->Get("method"); !method.IsEmpty()) {
rpcMethod = std::move(method);
}
MessageHandler(message);
l_TaskStats.InsertValue(Utility::GetTime(), 1);
auto total = ch::steady_clock::now() - start;
Log msg(total >= ch::seconds(5) ? LogWarning : LogDebug, "JsonRpcConnection");
msg << "Processed JSON-RPC '" << rpcMethod << "' message for identity '" << m_Identity
<< "' (took total " << toMilliseconds(total) << "ms";
if (cpuBoundDuration >= ch::seconds(1)) {
msg << ", waited " << toMilliseconds(cpuBoundDuration) << "ms on semaphore";
}
msg << ").";
} catch (const std::exception& ex) {
auto total = ch::steady_clock::now() - start;
Log msg(m_ShuttingDown ? LogDebug : LogWarning, "JsonRpcConnection");
msg << "Error while processing JSON-RPC '" << rpcMethod << "' message for identity '"
<< m_Identity << "' (took total " << toMilliseconds(total) << "ms";
if (cpuBoundDuration >= ch::seconds(1)) {
msg << ", waited " << toMilliseconds(cpuBoundDuration) << "ms on semaphore";
}
msg << "): " << DiagnosticInformation(ex);
break;
}

As it's a strand AND only one thread posts at a time, this should keep messages handling serial, as the code expects.

But then handling wouldn't block I/O.

Your Environment

  • Version used (icinga2 --version): e50eb52

Additional context

ref/NC/820479
@Al2Klimov Al2Klimov added bug Something isn't working area/distributed Distributed monitoring (master, satellites, clients) ref/NC labels Dec 11, 2024
@Al2Klimov Al2Klimov linked a pull request Dec 13, 2024 that will close this issue
JsonRpcConnection#HandleIncomingMessages(): don't block #m_IoStrand with #MessageHandler() #10273
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area/distributed Distributed monitoring (master, satellites, clients) bug Something isn't working ref/NC
Projects
None yet
Milestone
No milestone
1 participant
@Al2Klimov