Skip to content

JsonRpcConnection: Don't drop client from cache prematurely #10210

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

Merged
julianbrost merged 3 commits into from
Jan 9, 2025
Merged

JsonRpcConnection: Don't drop client from cache prematurely #10210

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3af7cfe
JsonRpcConnection: Don't drop client from cache prematurely
yhabteab Oct 31, 2024
41373ad
Log before & after an RPC client is disconnected
yhabteab Nov 25, 2024
1425641
Don't endlessly wait on writer coroutine on disconnect
yhabteab Nov 28, 2024
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
42 changes: 28 additions & 14 deletions lib/remote/jsonrpcconnection.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,28 +250,42 @@ void JsonRpcConnection::Disconnect()
if (!m_ShuttingDown.exchange(true)) {
JsonRpcConnection::Ptr keepAlive (this);
Comment thread
yhabteab marked this conversation as resolved.

IoEngine::SpawnCoroutine(m_IoStrand, [this, keepAlive](asio::yield_context yc) {
Log(LogWarning, "JsonRpcConnection")
<< "API client disconnected for identity '" << m_Identity << "'";

// We need to unregister the endpoint client as soon as possible not to confuse Icinga 2,
// given that Endpoint::GetConnected() is just performing a check that the endpoint's client
// cache is not empty, which could result in an already disconnected endpoint never trying to
// reconnect again. See #7444.
if (m_Endpoint) {
m_Endpoint->RemoveClient(this);
} else {
ApiListener::GetInstance()->RemoveAnonymousClient(this);
}
Log(LogNotice, "JsonRpcConnection")
<< "Disconnecting API client for identity '" << m_Identity << "'";

IoEngine::SpawnCoroutine(m_IoStrand, [this, keepAlive](asio::yield_context yc) {
m_OutgoingMessagesQueued.Set();

m_WriterDone.Wait(yc);
{
Timeout writerTimeout(
m_IoStrand,
boost::posix_time::seconds(5),
[this]() {
// The writer coroutine could not finish soon enough to unblock the waiter down blow,
// so we have to do this on our own, and the coroutine will be terminated forcibly when
// the ops on the underlying socket are cancelled.
boost::system::error_code ec;
m_Stream->lowest_layer().cancel(ec);
}
);

m_WriterDone.Wait(yc);
// We don't need to explicitly cancel the timer here; its destructor will handle it for us.
}

m_CheckLivenessTimer.cancel();
m_HeartbeatTimer.cancel();

m_Stream->GracefulDisconnect(m_IoStrand, yc);

Copy link
Copy Markdown
Member

@Al2Klimov Al2Klimov Dec 10, 2024

Choose a reason for hiding this comment

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

... i.e. it could be called while async_shutdown() here yields. This is obviously not what we want, so I'd like the same check here. This would make it a 99% copy of #10254 w/o even unit tests, so... 🤷‍♂️

Copy link
Copy Markdown
Member

@Al2Klimov Al2Klimov Dec 11, 2024

Choose a reason for hiding this comment

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

Alternatively to #10254 you can also use the current Timeout class.

Copy link
Copy Markdown
Member

@Al2Klimov Al2Klimov Jan 8, 2025

Choose a reason for hiding this comment

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

... with Defer!

Copy link
Copy Markdown
Member

@Al2Klimov Al2Klimov Jan 8, 2025

Choose a reason for hiding this comment

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

Last year I just wanted to express that #10254 is not strictly necessary, once also could just Defer-cancel the timeout like Julian did.

if (m_Endpoint) {
m_Endpoint->RemoveClient(this);
} else {
ApiListener::GetInstance()->RemoveAnonymousClient(this);
}

Log(LogWarning, "JsonRpcConnection")
<< "API client disconnected for identity '" << m_Identity << "'";
});
}
}
Expand Down