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.

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

Don't throw resize lock exception in destructor when removing nodemeta inventory #13894

Merged
merged 1 commit into from Oct 22, 2023

Conversation

Desour
Copy link
Member

@Desour Desour commented Oct 16, 2023

If you remove an inventory in meta in one of its inv callbacks, minetest currently crashes due to a resizelock exception in a destructor.
This PR triggers the exception before calling the destructor, so there will only be a lua error.
(The nodemeta will be left half-deleted. Only has the inventory will remain remain, not meta fields or privateness info.)

Fixes #13785 (comment).

To do

This PR is a Ready for Review.

How to test

minetest.register_node("test_inv_callbacks:node", {
	description = "inv callback node",
	tiles = {"default_cobble.png^heart.png"},
	groups = {choppy = 3, oddly_breakable_by_hand = 3},

	on_construct = function(pos)
		minetest.log("on_construct")
		local meta = minetest.get_meta(pos)
		local inv = meta:get_inventory()
		inv:set_size("mylist", 5)
		meta:set_string("formspec", "size[8,10]"
			.."list[current_player;main;0,3;8,4;]"
			.."list[current_name;mylist;0,0;10,1;]")
	end,
	on_metadata_inventory_move = function(pos, from_list, from_index,
			to_list, to_index, count, player)
		minetest.log("on_metadata_inventory_move")
		core.remove_node(pos)
	end,
})

@Desour Desour added @ Script API Bugfix 🐛 PRs that fix a bug labels Oct 16, 2023
@Desour Desour added this to the 5.8.0 milestone Oct 16, 2023
Copy link
Member

@grorp grorp left a comment

Choose a reason for hiding this comment

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

Since the on_* inventory callbacks are called after the items are moved, not before or while they are moved, this shouldn't even result in a Lua error, should it?

minetest/doc/lua_api.md

Lines 9206 to 9211 in 6fdc7e0

on_metadata_inventory_move = function(pos, from_list, from_index, to_list, to_index, count, player),
on_metadata_inventory_put = function(pos, listname, index, stack, player),
on_metadata_inventory_take = function(pos, listname, index, stack, player),
-- Called after the actual action has happened, according to what was
-- allowed.
-- No return value.

Is it possible to already have the resize lock released at this point?

@Desour
Copy link
Member Author

Desour commented Oct 17, 2023

Is it possible to already have the resize lock released at this point?

In some cases, probably.
Currently the list_from is kept lock for the case where the move/put/take is a swap, to get the item for the 2nd callback.

Anyway, the crash also happens if the node is removed in an allow_ callback.

Copy link
Member

@grorp grorp left a comment

Choose a reason for hiding this comment

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

Works, but doesn't fully resolve #13785 yet.

@SmallJoker
Copy link
Member

@grorp Your theory does not hold true in the case where ItemStacks are limited in count due to allow_* callbacks. When shift-clicking (i.e. moves according to listring[]), this would split the stack, resulting in subsequent callbacks.

@SmallJoker SmallJoker merged commit 7e8831a into minetest:master Oct 22, 2023
13 checks passed
@Desour Desour deleted the fix_inv_crash_on_setnode branch October 22, 2023 15:46
kawogi pushed a commit to kawogi/minetest that referenced this pull request Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

InventoryList 'main' is currently in use and cannot be deleted or resized.
3 participants