8000 Fix RPC Webhook queue dropping by WietseWind · Pull Request #5163 · XRPLF/rippled · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix RPC Webhook queue dropping #5163

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
merged 7 commits into from
Apr 10, 2025
Merged

Fix RPC Webhook queue dropping #5163

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
15a7863
Fix RPC Webhook queue dropping
WietseWind Oct 24, 2024
2252d4f
Remove comments
WietseWind Dec 16, 2024
4c4527b
Merge pull request #1 from WietseWind/fix/rpc-webhook-queue-rmcomment
WietseWind Dec 16, 2024
9bc8719
Merge branch 'develop' into fix/rpc-webhook-queue
mvadari Feb 4, 2025
0f9b3e1
Merge branch 'develop' into fix/rpc-webhook-queue
ximinez Mar 20, 2025
b6a7c65
Merge branch 'develop' into fix/rpc-webhook-queue
bthomee Apr 9, 2025
d9ce153
Merge branch 'develop' into fix/rpc-webhook-queue
bthomee Apr 10, 2025
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
4 changes: 2 additions & 2 deletions src/xrpld/net/detail/RPCCall.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1663,7 +1663,7 @@ fromNetwork(
constexpr auto RPC_REPLY_MAX_BYTES = megabytes(256);

using namespace std::chrono_literals;
auto constexpr RPC_NOTIFY = 10min;
auto constexpr RPC_WEBHOOK_TIMEOUT = 30s;

HTTPClient::request(
bSSL,
Expand All @@ -1680,7 +1680,7 @@ fromNetwork(
std::placeholders::_2,
j),
RPC_REPLY_MAX_BYTES,
RPC_NOTIFY,
RPC_WEBHOOK_TIMEOUT,
std::bind(
&RPCCallImp::onResponse,
callbackFuncP,
Expand Down
9 changes: 0 additions & 9 deletions src/xrpld/net/detail/RPCSub.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,13 +80,6 @@ class RPCSubImp : public RPCSub
{
std::lock_guard sl(mLock);

if (mDeque.size() >= eventQueueMax)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unbound queues are often trouble. In this particular case, because it's protected by the ADMIN role, perhaps this is acceptable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep exactly my thinking. It's an admin command. If -as an admin- you want to dump a ton in the queue, be our guest?

Copy link
Collaborator

Choose a reason for hiding this comment

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

What about increasing eventQueueMax to a more reasonable number instead of removing this block?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mvadari What is reasonable? Theoretically I'd want to subscribe to all transactions, and theoretically that's 1000? 1500? 2000? 3000? per ledger? Why would we limit that?

If you don't want that or don't have the resources to process it, don't subscribe to it on the admin command. If you do, why put a limit in place? And then what would be a reasonable limit?

It is an admin command. Don't do stupid things. If you're an admin, there are a million other things not to screw up. On an OS level. Service level. Security level. Making rippled send a lot of webhooks is the least of your worries.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Bump @WietseWind

Copy link
Collaborator
@godexsoft godexsoft Mar 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This is still an issue imo. Let's at least agree why this is ok or address it in some way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO, there is no reasonable limit given the ledger's potential growth. Assuming this is only relevant for admins, any potential issues are mitigated. It's not like anybody is going to be running this on a UNL validator.

I would like to see some analysis that shows that it's only relevant for admins.

Either way, ledgers are getting bigger, so a single hard-coded value ain't going to cut it. Another potential solution would be instead of a hard-coded eventQueueMax, we have a static eventQueueMax variable initialized to some reasonably large value, and a public static function that can set it. Any time something happens that might cause a large number of messages to be sent to subscribers (e.g. closing a ledger), call the setter with the number of expected messages (e.g. the ledger size). If the new value is larger, it will update eventQueueMax, and maybe add some hard-coded padding. That way the limit may grow crazy big as ledgers grow crazy big, but it will always be big enough, and the queue will not be unbound.

{
// Drop the previous event.
JLOG(j_.warn()) << "RPCCall::fromNetwork drop";
mDeque.pop_back();
}

auto jm = broadcast ? j_.debug() : j_.info();
JLOG(jm) << "RPCCall::fromNetwork push: " << jvObj;

Expand Down Expand Up @@ -184,8 +177,6 @@ class RPCSubImp : public RPCSub
}

private:
enum { eventQueueMax = 32 };

boost::asio::io_service& m_io_service;
JobQueue& m_jobQueue;

Expand Down
Loading
0