Implement batched messages for Digiline Chests

[andriyndev]

This patch implements wrapping signals, which were send in a very short interval (<= 0.1 seconds), into batches. This is needed to prevent Lua Controller from burning when using batch move (performed by selecting an item, and clicking on an item of the same type with Shift pressed). This causes the chest to send Digiline signals for each added stack within milliseconds that will cause the connected Lua controller to burn, and since Digiline chests can be accessed by any player, this gives any player possibility to burn any Lua controller connected to a Digiline chest.
This patch implements sending several Digiline chest events in a single Digiline signal if the previous Digiline signal was sent or included into the pending batch less than 0.1 seconds ago. Thus, in case of using batch move, the first chest event is sent as a separate Digiline signal (since we cannot know in advance about the upcoming events), and the next events are included into a batch.

[andriyndev]

I didn't edit README.inventory since it's already outdated

[SmallJoker]

1. How exactly can I reproduce the issue in current master? Would you please be so nice to share a screenshot, and Lua Controller code if there any any necessary?
2. Would you please be so nice to at least update the README a little bit? I think there's barely anyone who is currently familiar with the code, let alone with what you just implemented.

[andriyndev]

How exactly can I reproduce the issue in current master? Would you please be so nice to share a screenshot, and Lua Controller code if there any any necessary?

I described how to reproduce the issue in comment. But here is also a video with demonstration of the bug: https://youtu.be/lDgaIYBr0ZA

Would you please be so nice to at least update the README a little bit? I think there's barely anyone who is currently familiar with the code, let alone with what you just implemented.

This patch adds a new digiline signal for the chest in the format {action = "batch", signals = {<signals in the batch>}}. I used the name action for uniformity with other signals, and <signals in the batch> is just an array of the batched signals in their format. However the final format of the batch signal is a matter of discussion as well as other ways of fixing the bug. Regarding updating README a little bit, I see no sense in it because it currently doesn't mention Digiline chests at all, while README.inventory, which describes the Digiline chests, already contains outdated information about the format of the signals, which doesn't correspond to the current reality. As an option - suspend the patch until README.inventory is updated, and then I'll happily update README.inventory according to my changes.

[andriyndev]

I think I also need to restrict the maximum size of a batch to avoid its growth up to infinity under particular conditions.

UPD. Done

[andriyndev]

To do: handle the situation of multiple channels
UPD. Decided to leave as is for now because multiple channel means that the channel of the chest itself was changed which is extremely unlikely, and sending all the signals to the new channel is likely not an error. As another option - send the batch right on changing the channel name if it's not empty.

[SmallJoker]

This and last_message_time_for_chest needs periodic cleanups (e.g. every 30 seconds) in cases where mapblocks are unloaded from memory without sending the message(s).

[andriyndev]

Sorry for late response. I'm not sure that it's the best solution because this means that the message will be lost that might be crucial in some cases and disrupt the logic of the factory. Also, I don't know how likely the situation is when the map region with a digiline chest is unloaded within 0.1 seconds after placing something to there in a batch (it might happen in case of delivering items to there by tubes in large amounts though). But even when it happens, I'm not sure how crucial is that we'll have some extra records in the structure for a while (maybe even until the server is shutting down). Another option would be to create such a timer, and if it detects such a situation, load the area and flush the signal. But in this case, as far as I understand, it'll activate the factory again that might send more items to the chest and trigger another batched message, and we're stuck in a vicious circle. Some players could theoretically even use it to keep their factories running when offline. I currently haven't come up with a solution of this dilemma.

[SmallJoker]

Mapblocks can be unloaded from one server step to the next. This case won't occur often, but it can accumulate over time (unless the same mapblocks are loaded again).

Option 1: Clearing old entries periodically

-- TODO: change to 120 seconds if confirmed working
local cleanup_interval = 30
local function cleanup_batches_from_unloaded_blocks()
	local threshold_time = minetest.get_us_time() - 1E6 * (cleanup_interval + interval_to_batch)
	local pos_to_remove = {}

	for pos_hash, prev_time in pairs(last_message_time_for_chest) do
		if prev_time < threshold_time then
			-- Expired
			table.insert(pos_to_remove, pos_hash) 
		end
	end

	-- Remove all data assigned to this position
	for _, pos_hash in ipairs(pos_to_remove) do
		batched_messages[pos_hash] = nil
		last_message_time_for_chest[pos_hash] = nil
	end

	minetest.after(cleanup_interval, cleanup_batches_from_unloaded_blocks)
end

-- Short time to discover code issues early
minetest.after(5, cleanup_batches_from_unloaded_blocks)

Option 2: node name validity check

1. get rid of node timers
2. iterate through batched_messages periodically (minetest.register_globalstep)
3. process the entry or remove it, depending on whether minetest.get_node(pos) returns the digiline node name.

---

Also: might there be nodes that switch between two states (e.g. active/inactive) ? AFAIK when using minetest.swap_node or equivalent, the node timer is removed but no on _destruct called. Judging from the C++ code, node timers appear to be untouched by swap node.

[andriyndev]

It seems I've come up with the perfect solution: use batched messages only for inventory actions triggered by a user. In this case it will be very unlikely that the chest is unloaded after 0.1 seconds after interaction with a user. But even if it happens, it shouldn't be a problem to load the map region again, and flush the batched messages. Regarding actions triggered by tubes, it will be the chest's owner's responsibility to ensure that the incoming (or outcoming) items won't burn the controller.

[andriyndev]

And yes, use minetest.after() instead of node timers to trigger it, so that it'll be triggered even if the node is unloaded

[andriyndev]

Done

[andriyndev]

We use last_message_time_for_chest to store time, at which the last message was sent, for each chest. A possible problem is that it's cleared only when its batch is sent, and if a chest doesn't send batched signals but sends other ones, its related value will always be present in last_message_time_for_chest. I don't know whether it's bad but in any case creating a separate timer for this purpose would be an overhead (IMHO), and I don't see other elegant solution except of iterating over the whole table on some regular events. At the same time, clearing it right after sending a batch might be also a bad idea since in this case if we need to send another message right after sending the batch, it won't be included into the next one.

[SmallJoker]

At the same time, clearing [the time] right after sending a batch might be also a bad idea since in this case if we need to send another message right after sending the batch, it won't be included into the next one.

I see. You'd generally want to send single messages immediately and batch afterwards to avoid delays. I currently do not know of any solution that wouldn't involve a periodic "garbage collector" by iterating. You could add a TODO comment in the code for now if you think that it does not pose an issue. Unfortunately I do not know either how much of a problem this can be.

[SmallJoker]

job:cancel() was added in 5.4.0. Increase the required version in README.md accordingly.

[andriyndev]

Ok, got it. However I'm not sure what's the best: increase the supported version or make a workaround and not use job:cancel()

[andriyndev]

I see. You'd generally want to send single messages immediately and batch afterwards to avoid delays. I currently do not know of any solution that wouldn't involve a periodic "garbage collector" by iterating. You could add a TODO comment in the code for now if you think that it does not pose an issue. Unfortunately I do not know either how much of a problem this can be.

I think I'll try to add a timer which will perform garbage collection in some periods of time such as every 1 minute or 1 hour, and it will start on insertion if there are no active timers, or restart if the table is not empty at the moment of expiration.

[andriyndev]

By the way, I call minetest.load_area(pos) if the area at pos is unloaded. But I have doubts regarding whether calling it once will be reliable.