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

Remove panic from crdt.RGATreeList #596

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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: 3 additions & 1 deletion api/converter/from_bytes.go
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,9 @@ func fromJSONArray(pbArr *api.JSONElement_JSONArray) (*crdt.Array, error) {
if err != nil {
return nil, err
}
elements.Add(elem)
if err = elements.Add(elem); err != nil {
return nil, err
}
}

createdAt, err := fromTimeTicket(pbArr.CreatedAt)
Expand Down
27 changes: 16 additions & 11 deletions pkg/document/crdt/array.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,8 @@ func (a *Array) Purge(elem Element) error {
}

// Add adds the given element at the last.
func (a *Array) Add(elem Element) *Array {
a.elements.Add(elem)
return a
func (a *Array) Add(elem Element) error {
return a.elements.Add(elem)
}

// Get returns the element of the given index.
Expand All @@ -59,7 +58,7 @@ func (a *Array) Get(idx int) (Element, error) {

// FindPrevCreatedAt returns the creation time of the previous element of the
// given element.
func (a *Array) FindPrevCreatedAt(createdAt *time.Ticket) *time.Ticket {
func (a *Array) FindPrevCreatedAt(createdAt *time.Ticket) (*time.Ticket, error) {
return a.elements.FindPrevCreatedAt(createdAt)
}

Expand All @@ -74,8 +73,8 @@ func (a *Array) Delete(idx int, deletedAt *time.Ticket) (Element, error) {

// MoveAfter moves the given `createdAt` element after the `prevCreatedAt`
// element.
func (a *Array) MoveAfter(prevCreatedAt, createdAt, executedAt *time.Ticket) {
a.elements.MoveAfter(prevCreatedAt, createdAt, executedAt)
func (a *Array) MoveAfter(prevCreatedAt, createdAt, executedAt *time.Ticket) error {
return a.elements.MoveAfter(prevCreatedAt, createdAt, executedAt)
}

// Elements returns an array of elements contained in this RGATreeList.
Expand Down Expand Up @@ -111,7 +110,9 @@ func (a *Array) DeepCopy() (Element, error) {
if err != nil {
return nil, err
}
elements.Add(copiedNode)
if err = elements.Add(copiedNode); err != nil {
return nil, err
}
}

array := NewArray(elements, a.createdAt)
Expand Down Expand Up @@ -160,13 +161,17 @@ func (a *Array) LastCreatedAt() *time.Ticket {
}

// InsertAfter inserts the given element after the given previous element.
func (a *Array) InsertAfter(prevCreatedAt *time.Ticket, element Element) {
a.elements.InsertAfter(prevCreatedAt, element)
func (a *Array) InsertAfter(prevCreatedAt *time.Ticket, element Element) error {
return a.elements.InsertAfter(prevCreatedAt, element)
}

// DeleteByCreatedAt deletes the given element.
func (a *Array) DeleteByCreatedAt(createdAt *time.Ticket, deletedAt *time.Ticket) Element {
return a.elements.DeleteByCreatedAt(createdAt, deletedAt).elem
func (a *Array) DeleteByCreatedAt(createdAt *time.Ticket, deletedAt *time.Ticket) (Element, error) {
node, err := a.elements.DeleteByCreatedAt(createdAt, deletedAt)
if err != nil {
return nil, err
}
return node.elem, nil
}

// Len returns length of this Array.
Expand Down
9 changes: 6 additions & 3 deletions pkg/document/crdt/array_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,14 @@ func TestArray(t *testing.T) {

a := crdt.NewArray(crdt.NewRGATreeList(), ctx.IssueTimeTicket())

a.Add(crdt.NewPrimitive("1", ctx.IssueTimeTicket()))
err := a.Add(crdt.NewPrimitive("1", ctx.IssueTimeTicket()))
assert.NoError(t, err)
assert.Equal(t, `["1"]`, a.Marshal())
a.Add(crdt.NewPrimitive("2", ctx.IssueTimeTicket()))
err = a.Add(crdt.NewPrimitive("2", ctx.IssueTimeTicket()))
assert.NoError(t, err)
assert.Equal(t, `["1","2"]`, a.Marshal())
a.Add(crdt.NewPrimitive("3", ctx.IssueTimeTicket()))
err = a.Add(crdt.NewPrimitive("3", ctx.IssueTimeTicket()))
assert.NoError(t, err)
assert.Equal(t, `["1","2","3"]`, a.Marshal())
})
}
2 changes: 1 addition & 1 deletion pkg/document/crdt/element.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ type Container interface {
Descendants(callback func(elem Element, parent Container) bool)

// DeleteByCreatedAt removes the given element from this container.
DeleteByCreatedAt(createdAt *time.Ticket, deletedAt *time.Ticket) Element
DeleteByCreatedAt(createdAt *time.Ticket, deletedAt *time.Ticket) (Element, error)
}

// GCElement represents Element which has GC.
Expand Down
8 changes: 4 additions & 4 deletions pkg/document/crdt/element_rht.go
Original file line number Diff line number Diff line change
Expand Up @@ -129,17 +129,17 @@ func (rht *ElementRHT) Delete(k string, deletedAt *time.Ticket) Element {
}

// DeleteByCreatedAt deletes the Element of the given creation time.
func (rht *ElementRHT) DeleteByCreatedAt(createdAt *time.Ticket, deletedAt *time.Ticket) Element {
func (rht *ElementRHT) DeleteByCreatedAt(createdAt *time.Ticket, deletedAt *time.Ticket) (Element, error) {
node, ok := rht.nodeMapByCreatedAt[createdAt.Key()]
if !ok {
return nil
return nil, fmt.Errorf("fail to find the given createdAt: %s", createdAt.Key())
hackerwins marked this conversation as resolved.
Show resolved Hide resolved
}

if !node.Remove(deletedAt) {
return nil
return nil, nil
}

return node.elem
return node.elem, nil
}

// Elements returns a map of elements because the map easy to use for loop.
Expand Down
2 changes: 1 addition & 1 deletion pkg/document/crdt/object.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ func (o *Object) Has(k string) bool {
}

// DeleteByCreatedAt deletes the element of the given creation time.
func (o *Object) DeleteByCreatedAt(createdAt *time.Ticket, deletedAt *time.Ticket) Element {
func (o *Object) DeleteByCreatedAt(createdAt *time.Ticket, deletedAt *time.Ticket) (Element, error) {
return o.memberNodes.DeleteByCreatedAt(createdAt, deletedAt)
}

Expand Down
48 changes: 28 additions & 20 deletions pkg/document/crdt/rga_tree_list.go
Original file line number Diff line number Diff line change
Expand Up @@ -149,8 +149,8 @@ func (a *RGATreeList) Marshal() string {
}

// Add adds the given element at the last.
func (a *RGATreeList) Add(elem Element) {
a.insertAfter(a.last.CreatedAt(), elem, elem.CreatedAt())
func (a *RGATreeList) Add(elem Element) error {
return a.insertAfter(a.last.CreatedAt(), elem, elem.CreatedAt())
}

// Nodes returns an array of elements contained in this RGATreeList.
Expand All @@ -175,8 +175,8 @@ func (a *RGATreeList) LastCreatedAt() *time.Ticket {
}

// InsertAfter inserts the given element after the given previous element.
func (a *RGATreeList) InsertAfter(prevCreatedAt *time.Ticket, elem Element) {
a.insertAfter(prevCreatedAt, elem, elem.CreatedAt())
func (a *RGATreeList) InsertAfter(prevCreatedAt *time.Ticket, elem Element) error {
return a.insertAfter(prevCreatedAt, elem, elem.CreatedAt())
}

// Get returns the element of the given index.
Expand Down Expand Up @@ -207,17 +207,17 @@ func (a *RGATreeList) Get(idx int) (*RGATreeListNode, error) {
}

// DeleteByCreatedAt deletes the given element.
func (a *RGATreeList) DeleteByCreatedAt(createdAt *time.Ticket, deletedAt *time.Ticket) *RGATreeListNode {
func (a *RGATreeList) DeleteByCreatedAt(createdAt *time.Ticket, deletedAt *time.Ticket) (*RGATreeListNode, error) {
node, ok := a.nodeMapByCreatedAt[createdAt.Key()]
if !ok {
panic("fail to find the given createdAt: " + createdAt.Key())
return nil, fmt.Errorf("fail to find the given createdAt: %s", createdAt.Key())
}

alreadyRemoved := node.isRemoved()
if node.elem.Remove(deletedAt) && !alreadyRemoved {
a.nodeMapByIndex.Splay(node.indexNode)
}
return node
return node, nil
}

// Len returns length of this RGATreeList.
Expand All @@ -237,35 +237,38 @@ func (a *RGATreeList) Delete(idx int, deletedAt *time.Ticket) (*RGATreeListNode,
if err != nil {
return nil, err
}
return a.DeleteByCreatedAt(target.CreatedAt(), deletedAt), nil
return a.DeleteByCreatedAt(target.CreatedAt(), deletedAt)
}

// MoveAfter moves the given `createdAt` element after the `prevCreatedAt`
// element.
func (a *RGATreeList) MoveAfter(prevCreatedAt, createdAt, executedAt *time.Ticket) {
func (a *RGATreeList) MoveAfter(prevCreatedAt, createdAt, executedAt *time.Ticket) error {
prevNode, ok := a.nodeMapByCreatedAt[prevCreatedAt.Key()]
if !ok {
panic("fail to find the given prevCreatedAt: " + prevCreatedAt.Key())
return fmt.Errorf("fail to find the given prevCreatedAt: %s", prevCreatedAt.Key())
}

node, ok := a.nodeMapByCreatedAt[createdAt.Key()]
if !ok {
panic("fail to find the given createdAt: " + createdAt.Key())
return fmt.Errorf("fail to find the given createdAt: %s", createdAt.Key())
}

if node.elem.MovedAt() == nil || executedAt.After(node.elem.MovedAt()) {
a.release(node)
a.insertAfter(prevNode.CreatedAt(), node.elem, executedAt)
if err := a.insertAfter(prevNode.CreatedAt(), node.elem, executedAt); err != nil {
return err
}
node.elem.SetMovedAt(executedAt)
}
return nil
}

// FindPrevCreatedAt returns the creation time of the previous element of the
// given element.
func (a *RGATreeList) FindPrevCreatedAt(createdAt *time.Ticket) *time.Ticket {
func (a *RGATreeList) FindPrevCreatedAt(createdAt *time.Ticket) (*time.Ticket, error) {
node, ok := a.nodeMapByCreatedAt[createdAt.Key()]
if !ok {
panic("fail to find the given prevCreatedAt: " + createdAt.Key())
return nil, fmt.Errorf("fail to find the given prevCreatedAt: %s", createdAt.Key())
}

for {
Expand All @@ -275,7 +278,7 @@ func (a *RGATreeList) FindPrevCreatedAt(createdAt *time.Ticket) *time.Ticket {
}
}

return node.CreatedAt()
return node.CreatedAt(), nil
}

// purge physically purge child element.
Expand All @@ -293,17 +296,17 @@ func (a *RGATreeList) purge(elem Element) error {
func (a *RGATreeList) findNextBeforeExecutedAt(
createdAt *time.Ticket,
executedAt *time.Ticket,
) *RGATreeListNode {
) (*RGATreeListNode, error) {
node, ok := a.nodeMapByCreatedAt[createdAt.Key()]
if !ok {
panic("fail to find the given createdAt: " + createdAt.Key())
return nil, fmt.Errorf("fail to find the given createdAt: %s", createdAt.Key())
}

for node.next != nil && node.next.PositionedAt().After(executedAt) {
node = node.next
}

return node
return node, nil
}

func (a *RGATreeList) release(node *RGATreeListNode) {
Expand All @@ -325,13 +328,18 @@ func (a *RGATreeList) insertAfter(
prevCreatedAt *time.Ticket,
value Element,
executedAt *time.Ticket,
) {
prevNode := a.findNextBeforeExecutedAt(prevCreatedAt, executedAt)
) error {
prevNode, err := a.findNextBeforeExecutedAt(prevCreatedAt, executedAt)
if err != nil {
return err
}

newNode := newRGATreeListNodeAfter(prevNode, value)
if prevNode == a.last {
a.last = newNode
}

a.nodeMapByIndex.InsertAfter(prevNode.indexNode, newNode.indexNode)
a.nodeMapByCreatedAt[value.CreatedAt().Key()] = newNode
return nil
}
25 changes: 16 additions & 9 deletions pkg/document/crdt/root_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,15 +36,18 @@ func TestRoot(t *testing.T) {
t.Run("garbage collection for array test", func(t *testing.T) {
root := helper.TestRoot()
ctx := helper.TextChangeContext(root)
array := crdt.NewArray(crdt.NewRGATreeList(), ctx.IssueTimeTicket())

array.Add(crdt.NewPrimitive(0, ctx.IssueTimeTicket()))
array.Add(crdt.NewPrimitive(1, ctx.IssueTimeTicket()))
array.Add(crdt.NewPrimitive(2, ctx.IssueTimeTicket()))
array := crdt.NewArray(crdt.NewRGATreeList(), ctx.IssueTimeTicket())
var err error
for _, v := range []int{0, 1, 2} {
err = array.Add(crdt.NewPrimitive(v, ctx.IssueTimeTicket()))
assert.NoError(t, err)
}
assert.Equal(t, "[0,1,2]", array.Marshal())

targetElement, _ := array.Get(1)
array.DeleteByCreatedAt(targetElement.CreatedAt(), ctx.IssueTimeTicket())
_, err = array.DeleteByCreatedAt(targetElement.CreatedAt(), ctx.IssueTimeTicket())
assert.NoError(t, err)
root.RegisterRemovedElementPair(array, targetElement)
assert.Equal(t, "[0,2]", array.Marshal())
assert.Equal(t, 1, root.GarbageLen())
Expand Down Expand Up @@ -182,10 +185,14 @@ func TestRoot(t *testing.T) {

obj := root.Object()
obj.Set("1", crdt.NewPrimitive(1, ctx.IssueTimeTicket()))
arr := crdt.NewArray(crdt.NewRGATreeList(), ctx.IssueTimeTicket()).
Add(crdt.NewPrimitive(1, ctx.IssueTimeTicket())).
Add(crdt.NewPrimitive(2, ctx.IssueTimeTicket())).
Add(crdt.NewPrimitive(3, ctx.IssueTimeTicket()))

arr := crdt.NewArray(crdt.NewRGATreeList(), ctx.IssueTimeTicket())
var err error
for _, v := range []int{1, 2, 3} {
err = arr.Add(crdt.NewPrimitive(v, ctx.IssueTimeTicket()))
assert.NoError(t, err)
}

obj.Set("2", arr)
obj.Set("3", crdt.NewPrimitive(3, ctx.IssueTimeTicket()))
assert.Equal(t, `{"1":1,"2":[1,2,3],"3":3}`, root.Object().Marshal())
Expand Down
13 changes: 10 additions & 3 deletions pkg/document/json/array.go
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,9 @@ func (p *Array) insertAfterInternal(
ticket,
))

p.InsertAfter(prevCreatedAt, value)
if err = p.InsertAfter(prevCreatedAt, value); err != nil {
panic(err)
Copy link
Member

Choose a reason for hiding this comment

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

Could you tell me why panic is called?
I think it's safer to handle errors using errors.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, are you going to phase it out?

Copy link
Contributor Author

@sejongk sejongk Aug 2, 2023

Choose a reason for hiding this comment

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

I thought it is okay to call panic in json according to #497 (comment).
It would be great to change the above logic to handle errors if possible.
I guess one of possible errors can be an error that occurs because of a failure of finding a node for the given createdAt.

func (a *RGATreeList) findNextBeforeExecutedAt(
	createdAt *time.Ticket,
	executedAt *time.Ticket,
) (*RGATreeListNode, error) {
	node, ok := a.nodeMapByCreatedAt[createdAt.Key()]
	if !ok {
		return nil, fmt.Errorf("fail to find the given createdAt: %s", createdAt.Key())
	}

	...
}

Would you mind giving me some guides or examples to safely handle errors like the above?

}
p.context.RegisterElement(value)

return elem
Expand All @@ -223,7 +225,10 @@ func (p *Array) insertAfterInternal(
func (p *Array) moveBeforeInternal(nextCreatedAt, createdAt *time.Ticket) {
ticket := p.context.IssueTimeTicket()

prevCreatedAt := p.FindPrevCreatedAt(nextCreatedAt)
prevCreatedAt, err := p.FindPrevCreatedAt(nextCreatedAt)
if err != nil {
panic(err)
}

p.context.Push(operations.NewMove(
p.Array.CreatedAt(),
Expand All @@ -232,5 +237,7 @@ func (p *Array) moveBeforeInternal(nextCreatedAt, createdAt *time.Ticket) {
ticket,
))

p.MoveAfter(prevCreatedAt, createdAt, ticket)
if err = p.MoveAfter(prevCreatedAt, createdAt, ticket); err != nil {
panic(err)
}
}
5 changes: 4 additions & 1 deletion pkg/document/operations/add.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,10 @@ func (o *Add) Execute(root *crdt.Root) error {
if err != nil {
return err
}
obj.InsertAfter(o.prevCreatedAt, value)

if err = obj.InsertAfter(o.prevCreatedAt, value); err != nil {
return err
}

root.RegisterElement(value)
return nil
Expand Down
4 changes: 1 addition & 3 deletions pkg/document/operations/move.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,7 @@ func (o *Move) Execute(root *crdt.Root) error {
return ErrNotApplicableDataType
}

obj.MoveAfter(o.prevCreatedAt, o.createdAt, o.executedAt)

return nil
return obj.MoveAfter(o.prevCreatedAt, o.createdAt, o.executedAt)
}

// CreatedAt returns the creation time of the target element.
Expand Down
5 changes: 4 additions & 1 deletion pkg/document/operations/remove.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,10 @@ func (o *Remove) Execute(root *crdt.Root) error {

switch parent := parentElem.(type) {
case crdt.Container:
elem := parent.DeleteByCreatedAt(o.createdAt, o.executedAt)
elem, err := parent.DeleteByCreatedAt(o.createdAt, o.executedAt)
if err != nil {
return err
}
if elem != nil {
root.RegisterRemovedElementPair(parent, elem)
}
Expand Down
Loading