Skip to content

Commit

Permalink
part: Fix part.Map's Delete
Browse files Browse the repository at this point in the history
The Map.Delete and Set.Delete methods used Tree.Delete which did a
Commit that closed all watch channels. On delete of the same element
from the original this lead to double-closing of a channel.

For part.Map and part.Set we do not want the watch channel functionality
and instead should use CommitOnly().

Signed-off-by: Jussi Maki <[email protected]>
  • Loading branch information
joamaki committed Jul 15, 2024
1 parent 563f42c commit dc95f9b
Show file tree
Hide file tree
Showing 4 changed files with 31 additions and 10 deletions.
5 changes: 3 additions & 2 deletions part/map.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,11 @@ func (m Map[K, V]) Set(key K, value V) Map[K, V] {
// without the element pointed to by the key (if found).
func (m Map[K, V]) Delete(key K) Map[K, V] {
if m.tree != nil {
_, _, tree := m.tree.Delete(m.bytesFromKey(key))
txn := m.tree.Txn()
txn.Delete(m.bytesFromKey(key))
// Map is a struct passed by value, so we can modify
// it without changing the caller's view of it.
m.tree = tree
m.tree = txn.CommitOnly()
}
return m
}
Expand Down
12 changes: 12 additions & 0 deletions part/map_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,11 @@ func TestStringMap(t *testing.T) {
"3_three": 3,
})

// Setting on a copy doeen't affect original
m.Set("4_four", 4)
_, ok = m.Get("4_four")
assert.False(t, ok, "Get non-existing")

// Getting a non-existing value still does the same.
v, ok = m.Get("nonexisting")
assert.False(t, ok, "Get non-existing")
Expand Down Expand Up @@ -98,10 +103,17 @@ func TestStringMap(t *testing.T) {

assert.Equal(t, 3, m.Len())

mOld := m
m = m.Delete(kvs[0].k)
_, ok = m.Get(kvs[0].k)
assert.False(t, ok, "Get after Delete")

_, ok = mOld.Get(kvs[0].k)
assert.True(t, ok, "Original modified by Delete")
mOld = mOld.Delete(kvs[0].k)
_, ok = mOld.Get(kvs[0].k)
assert.False(t, ok, "Get after Delete")

assert.Equal(t, 2, m.Len())
}

Expand Down
10 changes: 6 additions & 4 deletions part/set.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,9 @@ func (s Set[T]) Set(v T) Set[T] {
if s.tree == nil {
return NewSet(v)
}
_, _, tree := s.tree.Insert(s.toBytes(v), v)
s.tree = tree // As Set is passed by value we can just modify it.
txn := s.tree.Txn()
txn.Insert(s.toBytes(v), v)
s.tree = txn.CommitOnly() // As Set is passed by value we can just modify it.
return s
}

Expand All @@ -51,8 +52,9 @@ func (s Set[T]) Delete(v T) Set[T] {
if s.tree == nil {
return s
}
_, _, tree := s.tree.Delete(s.toBytes(v))
s.tree = tree
txn := s.tree.Txn()
txn.Delete(s.toBytes(v))
s.tree = txn.CommitOnly()
return s
}

Expand Down
14 changes: 10 additions & 4 deletions part/set_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,18 @@ func TestStringSet(t *testing.T) {

assert.Equal(t, 2, s3.Len())

s3 = s3.Delete("foo")
assert.False(t, s3.Has("foo"), "s3 has no foo")
s5 := s3.Delete("foo")
assert.True(t, s3.Has("foo"), "s3 has foo")
assert.False(t, s5.Has("foo"), "s3 has no foo")

// Deleting again does the same.
s5 = s3.Delete("foo")
assert.False(t, s5.Has("foo"), "s3 has no foo")

assert.Equal(t, 1, s3.Len())
assert.Equal(t, 2, s3.Len())
assert.Equal(t, 1, s5.Len())

xs := s3.Slice()
xs := s5.Slice()
assert.Len(t, xs, 1)
assert.Equal(t, "bar", xs[0])
}
Expand Down

0 comments on commit dc95f9b

Please sign in to comment.