Improvements to event observer management

[obycode]

I am relaying this suggestion from a user:

Currently, sending an event to an observer is a blocking action, meaning that the observer can block the stacks-node. This is intended behavior, to ensure that the observer receives all events, but it could be handled better without blocking the core functionality of the node. The proposal is to make more use of the event_observers.sqlite DB. The node could write the event to the DB, then signal to another thread that the new event is ready to be sent. This other thread may be blocked by the observer, but the node could continue with its normal activities after writing the event into the DB.

[jcnelson]

One caveat is that the event observer could fall behind the node, and cause the event_observers.sqlite DB to grow unbounded. How do you imagine that we'd address this?

[benjamin-stacks]

One caveat is that the event observer could fall behind the node, and cause the event_observers.sqlite DB to grow unbounded. How do you imagine that we'd address this?

Couple of thoughts, in random order:

• The status quo is that the node just stops working altogether. Isn't that worse?
• If this is really a practical concern, we could put an upper limit on the number of pending events before we start blocking again (or even crashing? I'm not yet familiar enough with everything to have an intuition on whether that would be appropriate).
• My assumption (which may be totally wrong, please correct me!) is that event DB growth would be O(chainstate growth) with a reasonable constant, and thus acceptable in practice
• We could make the process_pending_payloads call blocking. That way, at least on node restart, the owner gets the message "fix your observer or unregister it now"
• A common pattern for webhooks (which these ultimately are) is that after a certain number of failures, the caller stops sending them. This isn't 100% the same thing because the same entity probably controls receiver and sender, but it might still be an option to consider.

[brice-stacks]

I'd be happy with just an error log if the database grows too large. If that's not noticeable enough, we could stall the node at some point, but I'm not sure if that's necessary.

[benjamin-stacks]

Okay, here's my current thinking. Although everytime I write something down, I realize another edge case 😬

• make EventObserver into just a dumb struct without a lot of functionality (I think that used to be the case once?) -- just a copy of the observer config
• EventDispatcher is where all the magic happens
• EventDispatcher::new spins up a single extra thread and stores a mechanism to wake up that subthread
• (event dispatchers get cloned in a couple of places, but there is only a single subthread)
• (want to avoid message passing because an unbounded channel to a blocked subthread would leak memory)
• (there should probably be a reference counting-based mechanism that tears down the subthread when the final EventDispatcher is dropped -- probably only relevant in tests)
• (TODO: figure out how to handle multiple event dispatchers that would be talking to the same DB -- some sort of unique identifier on DB records?)
• The subthread grabs a payload from the DB (oldest first), makes the request, deletes on success.
• disable_retry is added as a DB column so the subthread can honor it
• (could add a time stamp to the DB; the subthread could figure out how long a message has been pending and issue a warning if that delay gets long)
• If there's no DB, HTTP requests continue on the main thread (I think this only happens in tests?)
• process_pending_replies on startup is still blocking

With this, a single broken event subscriber will still block all other events from being delivered, because there's just a single thread. I think that makes sense because it feels like there's an expectation the events are delivered in order. It might also make it more obvious to the operator that something's wrong.

Let me know if you have any thoughts; I'll start with trying to implement something on Monday.

[benjamin-stacks]

@brice-stacks @jcnelson This commit is what I went with (there's a couple of other commits in the PR, and there will be some additional testing code, but that commit is the meat).

Here's a couple of considerations I made after the discussion here:

Having the queue go theoretically unbounded isn't only a space / memory concern, as Jude pointed out, but it's also pretty pointless in my opinion. The purpose of this change is to allow the node to continue working while (potentially slow) requests are made, but if all the requests are so slow (or never succeed) that the event dispatcher keeps running far behind, something needs fixing.

So I decided to have a bounded queue after all (implemented through sync_channel). If that limit is reached, event delivery will start blocking again. I picked a limit of 1,000, but that's pretty arbitrary -- no idea what number makes sense, happy to hear thoughts.

This also fixes a couple other concerns I listed above (e.g. multiple event dispatchers is no longer a problem, since specific IDs are passed through the channel).

As for the "no DB" case, that's no longer a problem. I talked to Brice and we agreed that use case can be dropped, as it was only used in tests anyway. See #6785.

[jcnelson]

The purpose of this change is to allow the node to continue working while (potentially slow) requests are made, but if all the requests are so slow (or never succeed) that the event dispatcher keeps running far behind, something needs fixing.

So, the event dispatcher system was originally designed to allow an application to do its own application-specific processing on a block atomically with respect the block being processed. That is, the desired execution flow would be:

[node]                [app]
  |                     |
process block N         |
  |                   process block N
  |                     |
process block N+1       |
  |                   process block N+1
  |                     |
process block N+2       |
  |                   process block N+2

Block N+1 would not be processed until the application has had a chance to process block N.

Adding a bounded event queue with an app-configured bound would relax this constraint, so that the node would be able to process a burst of at most K blocks ahead of the application. Importantly, this is not a breaking change per se -- the prior behavior would continue to be supported as a special case when K = 1.

We believed that K = 1 (the original blocking behavior) was a useful behavior because (1) the main consumer of this API was the block explorer, which (at the time) needed its view of the blocks' events to be consistent with the node's view of the blocks, and (2) other consumers that wanted K > 1 could do so on their own -- the application could spawn a thread to eagerly accept and store block events from the node, and spawn one or more other threads to process those events asynchronously. Then, the application could implement its own policies for supporting node bursts. But ultimately, the goal was to allow the application to control the rate at which the node processed blocks, so it could guarantee that application-specific block processing would occur before the node continued to process more blocks (or bursts of blocks).

The issue opened by @obycode, as relayed to him by a user, was to ask for the node to supply the boilerplate code necessary to handle the case where K > 1. This is useful (and I'm glad you've taken this on!) because it simplifies the design of applications that want the node to process bursts of blocks -- the node will now buffer up events for the application, and the application can now consume them at its own pace without stalling the node.

So, given the above context:

• I think K = 1000 is a fine default, but I think it will be necessary to make this a configuration option. We do not know (and should not impose) a burst limit on the application, and we do not know if the application wants the node to burst head at all. If the application does not want bursting, it can set K = 1. If the application does not need to atomically process blocks with respect to the node, then it can set K to be an arbitrarily large value (e.g. u64::MAX).

• I think the node will need to stall block processing instead of silently dropping events if it bursts too far ahead of the application. I agree that a slow application is a problem, but dropping events silently is I think more surprising (and more damaging) than stalling block processing. The application wants to process potentially all of the block events, and if we silently drop arbitrary events (especially due to the application falling behind), we may easily corrupt the application's state and do so in ways that are hard to reliably reproduce. The application could potentially recover by identifying gaps in the event stream and replaying missed blocks and to get the missed events, but that significantly complicates the application design (which runs afoul with the intent of this PR, which is to simplify application design). Block events are not like webhooks -- in many cases, applications that consume block events want all of the events for e.g. a set of smart contracts, since they produce a materialized view of the smart contracts' state for users to consume.

[benjamin-stacks]

I think the node will need to stall block processing instead of silently dropping events if it bursts too far ahead of the application.

Agree, and that's what my approach is doing. No events are silently dropped (unless potentially when an observer is configured to disable_retries). If the queue is full, execution stalls. We get that behavior for free from sync_channel:

When the internal buffer becomes full, future sends will block waiting for the buffer to open up.

I'll make the buffer limit a configuration option.

Thanks for the detailed writeup!