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

Fix miscounting childrenNum in node16's shrink #28

Merged
merged 1 commit into from
Oct 30, 2024
Merged

Conversation

udzura
Copy link
Contributor

@udzura udzura commented Apr 9, 2024

When keys are added and deleted in order as shown below:

	keys := []Key{
		Key("test/a1"),
		Key("test/a2"),
		Key("test/a3"),
		Key("test/a4"),
		// This should become zeroChild
		Key("test/a"),
	}

	tree := newTree()
	for _, w := range keys {
		tree.Insert(w, w)
	}

	for _, w := range keys {
		// Fail!!!
		tree.Delete(w)
	}

This Delete will fail:

--- FAIL: TestTreeInsertAndDeleteConcerningZeroChild (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
        panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x8 pc=0x1046a50ac]

goroutine 92 [running]:
testing.tRunner.func1.2({0x10473d020, 0x10489e010})
        /opt/homebrew/Cellar/go/1.21.1/libexec/src/testing/testing.go:1545 +0x1c8
testing.tRunner.func1()
        /opt/homebrew/Cellar/go/1.21.1/libexec/src/testing/testing.go:1548 +0x360
panic({0x10473d020?, 0x10489e010?})
        /opt/homebrew/Cellar/go/1.21.1/libexec/src/runtime/panic.go:914 +0x218
github.com/plar/go-adaptive-radix-tree.(*artNode).isLeaf(...)
        /opt/go/github.com/plar/go-adaptive-radix-tree/node.go:108
github.com/plar/go-adaptive-radix-tree.(*artNode).shrink(0x14001be5760)
        /opt/go/github.com/plar/go-adaptive-radix-tree/node.go:656 +0x4c
github.com/plar/go-adaptive-radix-tree.(*artNode).deleteChild(0x14001be5760, 0x90?, 0x0?)
        /opt/go/github.com/plar/go-adaptive-radix-tree/node.go:563 +0xcc
github.com/plar/go-adaptive-radix-tree.(*tree).recursiveDelete(0x140000e5ec8, 0x1046b5cfc?, {0x140000b0090, 0x6, 0x6}, 0x0?)
        /opt/go/github.com/plar/go-adaptive-radix-tree/tree.go:218 +0x1f0
github.com/plar/go-adaptive-radix-tree.(*tree).Delete(0x140000e5ec8, {0x140000b0090?, 0x10473e000?, 0x14001c15320?})
        /opt/go/github.com/plar/go-adaptive-radix-tree/tree.go:25 +0x40
github.com/plar/go-adaptive-radix-tree.TestTreeInsertAndDeleteConcerningZeroChild(0x0?)
        /opt/go/github.com/plar/go-adaptive-radix-tree/tree_test.go:1005 +0x208
testing.tRunner(0x140001561a0, 0x10476f3f0)
        /opt/homebrew/Cellar/go/1.21.1/libexec/src/testing/testing.go:1595 +0xe8
created by testing.(*T).Run in goroutine 1
        /opt/homebrew/Cellar/go/1.21.1/libexec/src/testing/testing.go:1648 +0x33c

This is because of miscounting of childrenNum, so this pull request addresses the issue with childrenNum.

Specifically, when node16 is shrunk to node4, the numChildren value is incorrectly fixed to 4, even though the target node may have between 1 and 3 children (see the example above). This has been corrected when we ensure the artNode is not nil in iteration.

@plar plar merged commit af00541 into plar:master Oct 30, 2024
@plar
Copy link
Owner

plar commented Oct 30, 2024

@udzura thanks for the fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants