From 91d062a1fcbea8aae4b8db6b44c4e27091964b23 Mon Sep 17 00:00:00 2001 From: Ian Denhardt Date: Sat, 27 May 2023 15:00:35 -0400 Subject: [PATCH 01/11] CapTable: rename Get -> GetClient, Add -> AddClient I plan on adding *Snapshot versions and eventually storing snapshots internally. --- answer_test.go | 2 +- capability.go | 4 ++-- capnpc-go/templates/structCapabilityField | 2 +- capnpc-go/templates/structInterfaceField | 2 +- captable.go | 10 +++++----- captable_test.go | 2 +- internal/aircraftlib/aircraft.capnp.go | 8 ++++---- list.go | 2 +- localpromise.go | 2 +- message_test.go | 8 ++++---- pogs/insert.go | 4 ++-- pogs/pogs_test.go | 2 +- rpc/export.go | 2 +- rpc/internal/testcapnp/test.capnp.go | 8 ++++---- rpc/level0_test.go | 2 +- rpc/level1_test.go | 2 +- rpc/rpc.go | 6 +++--- rpc/transport/transport_test.go | 2 +- segment.go | 2 +- segment_test.go | 6 +++--- 20 files changed, 39 insertions(+), 39 deletions(-) diff --git a/answer_test.go b/answer_test.go index 6f3cea28..aff4d9ed 100644 --- a/answer_test.go +++ b/answer_test.go @@ -103,7 +103,7 @@ func TestPromiseFulfill(t *testing.T) { defer msg.Release() res, _ := NewStruct(seg, ObjectSize{PointerCount: 3}) - res.SetPtr(1, NewInterface(seg, msg.CapTable().Add(c.AddRef())).ToPtr()) + res.SetPtr(1, NewInterface(seg, msg.CapTable().AddClient(c.AddRef())).ToPtr()) p.Fulfill(res.ToPtr()) diff --git a/capability.go b/capability.go index 473827fa..48a501ee 100644 --- a/capability.go +++ b/capability.go @@ -84,7 +84,7 @@ func (i Interface) value(paddr address) rawPointer { // or nil if the pointer is invalid. func (i Interface) Client() (c Client) { if msg := i.Message(); msg != nil { - c = msg.CapTable().Get(i) + c = msg.CapTable().GetClient(i) } return @@ -746,7 +746,7 @@ func (c Client) Release() { } func (c Client) EncodeAsPtr(seg *Segment) Ptr { - capId := seg.Message().CapTable().Add(c) + capId := seg.Message().CapTable().AddClient(c) return NewInterface(seg, capId).ToPtr() } diff --git a/capnpc-go/templates/structCapabilityField b/capnpc-go/templates/structCapabilityField index 76e20d1c..908236d1 100644 --- a/capnpc-go/templates/structCapabilityField +++ b/capnpc-go/templates/structCapabilityField @@ -12,6 +12,6 @@ func (s {{.Node.Name}}) Set{{.Field.Name|title}}(c {{.FieldType}}) error { return capnp.Struct(s).SetPtr({{.Field.Slot.Offset}}, capnp.Ptr{}) } seg := s.Segment() - in := capnp.NewInterface(seg, seg.Message().CapTable().Add(c)) + in := capnp.NewInterface(seg, seg.Message().CapTable().AddClient(c)) return capnp.Struct(s).SetPtr({{.Field.Slot.Offset}}, in.ToPtr()) } diff --git a/capnpc-go/templates/structInterfaceField b/capnpc-go/templates/structInterfaceField index 7d24e66a..ad78f048 100644 --- a/capnpc-go/templates/structInterfaceField +++ b/capnpc-go/templates/structInterfaceField @@ -12,7 +12,7 @@ func (s {{.Node.Name}}) Set{{.Field.Name|title}}(v {{.FieldType}}) error { return capnp.Struct(s).SetPtr({{.Field.Slot.Offset}}, capnp.Ptr{}) } seg := s.Segment() - in := capnp.NewInterface(seg, seg.Message().CapTable().Add(capnp.Client(v))) + in := capnp.NewInterface(seg, seg.Message().CapTable().AddClient(capnp.Client(v))) return capnp.Struct(s).SetPtr({{.Field.Slot.Offset}}, in.ToPtr()) } diff --git a/captable.go b/captable.go index 95a9da6d..9fcd47be 100644 --- a/captable.go +++ b/captable.go @@ -37,10 +37,10 @@ func (ct CapTable) Contains(ifc Interface) bool { return ifc.IsValid() && ifc.Capability() < CapabilityID(ct.Len()) } -// Get the client corresponding to the supplied interface. It -// returns a null client if the interface's CapabilityID isn't +// GetClient gets the client corresponding to the supplied interface. +// It returns a null client if the interface's CapabilityID isn't // in the table. -func (ct CapTable) Get(ifc Interface) (c Client) { +func (ct CapTable) GetClient(ifc Interface) (c Client) { if ct.Contains(ifc) { c = ct.cs[ifc.Capability()] } @@ -55,10 +55,10 @@ func (ct CapTable) Set(id CapabilityID, c Client) { ct.cs[id] = c } -// Add appends a capability to the message's capability table and +// AddClient appends a capability to the message's capability table and // returns its ID. It "steals" c's reference: the Message will release // the client when calling Reset. -func (ct *CapTable) Add(c Client) CapabilityID { +func (ct *CapTable) AddClient(c Client) CapabilityID { ct.cs = append(ct.cs, c) return CapabilityID(ct.Len() - 1) } diff --git a/captable_test.go b/captable_test.go index d10bdb2b..1e6e6e18 100644 --- a/captable_test.go +++ b/captable_test.go @@ -15,7 +15,7 @@ func TestCapTable(t *testing.T) { assert.Zero(t, ct.Len(), "zero-value CapTable should be empty") - assert.Zero(t, ct.Add(capnp.Client{}), + assert.Zero(t, ct.AddClient(capnp.Client{}), "first entry should have CapabilityID(0)") assert.Equal(t, 1, ct.Len(), "should increase length after adding capability") diff --git a/internal/aircraftlib/aircraft.capnp.go b/internal/aircraftlib/aircraft.capnp.go index 22cdb9c2..4cc4c8df 100644 --- a/internal/aircraftlib/aircraft.capnp.go +++ b/internal/aircraftlib/aircraft.capnp.go @@ -2336,7 +2336,7 @@ func (s Z) SetEcho(v Echo) error { return capnp.Struct(s).SetPtr(0, capnp.Ptr{}) } seg := s.Segment() - in := capnp.NewInterface(seg, seg.Message().CapTable().Add(capnp.Client(v))) + in := capnp.NewInterface(seg, seg.Message().CapTable().AddClient(capnp.Client(v))) return capnp.Struct(s).SetPtr(0, in.ToPtr()) } @@ -2448,7 +2448,7 @@ func (s Z) SetAnyCapability(c capnp.Client) error { return capnp.Struct(s).SetPtr(0, capnp.Ptr{}) } seg := s.Segment() - in := capnp.NewInterface(seg, seg.Message().CapTable().Add(c)) + in := capnp.NewInterface(seg, seg.Message().CapTable().AddClient(c)) return capnp.Struct(s).SetPtr(0, in.ToPtr()) } @@ -5524,7 +5524,7 @@ func (s EchoBase) SetEcho(v Echo) error { return capnp.Struct(s).SetPtr(0, capnp.Ptr{}) } seg := s.Segment() - in := capnp.NewInterface(seg, seg.Message().CapTable().Add(capnp.Client(v))) + in := capnp.NewInterface(seg, seg.Message().CapTable().AddClient(capnp.Client(v))) return capnp.Struct(s).SetPtr(0, in.ToPtr()) } @@ -6471,7 +6471,7 @@ func (s Pipeliner_newPipeliner_Results) SetPipeliner(v Pipeliner) error { return capnp.Struct(s).SetPtr(1, capnp.Ptr{}) } seg := s.Segment() - in := capnp.NewInterface(seg, seg.Message().CapTable().Add(capnp.Client(v))) + in := capnp.NewInterface(seg, seg.Message().CapTable().AddClient(capnp.Client(v))) return capnp.Struct(s).SetPtr(1, in.ToPtr()) } diff --git a/list.go b/list.go index 0f6af2b9..aa25fa2f 100644 --- a/list.go +++ b/list.go @@ -1092,7 +1092,7 @@ func (c CapList[T]) At(i int) (T, error) { func (c CapList[T]) Set(i int, v T) error { pl := PointerList(c) seg := pl.Segment() - capId := seg.Message().CapTable().Add(Client(v)) + capId := seg.Message().CapTable().AddClient(Client(v)) return pl.Set(i, NewInterface(seg, capId).ToPtr()) } diff --git a/localpromise.go b/localpromise.go index a478c169..82ea4d28 100644 --- a/localpromise.go +++ b/localpromise.go @@ -46,7 +46,7 @@ func (lp localPromise) String() string { func (lp localPromise) Fulfill(c Client) { msg, seg := NewSingleSegmentMessage(nil) - capID := msg.CapTable().Add(c) + capID := msg.CapTable().AddClient(c) lp.aq.Fulfill(NewInterface(seg, capID).ToPtr()) } diff --git a/message_test.go b/message_test.go index b146bd06..9e4446bb 100644 --- a/message_test.go +++ b/message_test.go @@ -400,7 +400,7 @@ func TestAddCap(t *testing.T) { msg := &Message{Arena: SingleSegment(nil)} // Simple case: distinct non-nil clients. - id1 := msg.CapTable().Add(client1.AddRef()) + id1 := msg.CapTable().AddClient(client1.AddRef()) assert.Equal(t, CapabilityID(0), id1, "first capability ID should be 0") assert.Equal(t, 1, msg.CapTable().Len(), @@ -408,7 +408,7 @@ func TestAddCap(t *testing.T) { assert.True(t, msg.CapTable().At(0).IsSame(client1), "client does not match entry in cap table") - id2 := msg.CapTable().Add(client2.AddRef()) + id2 := msg.CapTable().AddClient(client2.AddRef()) assert.Equal(t, CapabilityID(1), id2, "second capability ID should be 1") assert.Equal(t, 2, msg.CapTable().Len(), @@ -417,7 +417,7 @@ func TestAddCap(t *testing.T) { "client does not match entry in cap table") // nil client - id3 := msg.CapTable().Add(Client{}) + id3 := msg.CapTable().AddClient(Client{}) assert.Equal(t, CapabilityID(2), id3, "third capability ID should be 2") assert.Equal(t, 3, msg.CapTable().Len(), @@ -426,7 +426,7 @@ func TestAddCap(t *testing.T) { "client does not match entry in cap table") // Add should not attempt to deduplicate. - id4 := msg.CapTable().Add(client1.AddRef()) + id4 := msg.CapTable().AddClient(client1.AddRef()) assert.Equal(t, CapabilityID(3), id4, "fourth capability ID should be 3") assert.Equal(t, 4, msg.CapTable().Len(), diff --git a/pogs/insert.go b/pogs/insert.go index efe5199b..727f429c 100644 --- a/pogs/insert.go +++ b/pogs/insert.go @@ -239,7 +239,7 @@ func (ins *inserter) insertField(s capnp.Struct, f schema.Field, val reflect.Val if !c.IsValid() { return s.SetPtr(off, capnp.Ptr{}) } - id := s.Message().CapTable().Add(c) + id := s.Message().CapTable().AddClient(c) return s.SetPtr(off, capnp.NewInterface(s.Segment(), id).ToPtr()) default: panic("unreachable") @@ -255,7 +255,7 @@ func capPtr(seg *capnp.Segment, val reflect.Value) capnp.Ptr { if !client.IsValid() { return capnp.Ptr{} } - cap := seg.Message().CapTable().Add(client) + cap := seg.Message().CapTable().AddClient(client) iface := capnp.NewInterface(seg, cap) return iface.ToPtr() } diff --git a/pogs/pogs_test.go b/pogs/pogs_test.go index 4d6e09f0..b6812848 100644 --- a/pogs/pogs_test.go +++ b/pogs/pogs_test.go @@ -199,7 +199,7 @@ func newTestList() capnp.List { func newTestInterface() capnp.Interface { msg, seg, _ := capnp.NewMessage(capnp.SingleSegment(nil)) - id := msg.CapTable().Add(capnp.ErrorClient(errors.New("boo"))) + id := msg.CapTable().AddClient(capnp.ErrorClient(errors.New("boo"))) return capnp.NewInterface(seg, id) } diff --git a/rpc/export.go b/rpc/export.go index 671a59a3..62c9aef7 100644 --- a/rpc/export.go +++ b/rpc/export.go @@ -334,7 +334,7 @@ func (c *lockedConn) findEmbargo(id embargoID) *embargo { func newEmbargo(client capnp.Client) *embargo { msg, seg := capnp.NewSingleSegmentMessage(nil) - capID := msg.CapTable().Add(client) + capID := msg.CapTable().AddClient(client) iface := capnp.NewInterface(seg, capID) return &embargo{ result: iface.ToPtr(), diff --git a/rpc/internal/testcapnp/test.capnp.go b/rpc/internal/testcapnp/test.capnp.go index 2b70a6c6..f8db98bb 100644 --- a/rpc/internal/testcapnp/test.capnp.go +++ b/rpc/internal/testcapnp/test.capnp.go @@ -410,7 +410,7 @@ func (s EmptyProvider_getEmpty_Results) SetEmpty(v Empty) error { return capnp.Struct(s).SetPtr(0, capnp.Ptr{}) } seg := s.Segment() - in := capnp.NewInterface(seg, seg.Message().CapTable().Add(capnp.Client(v))) + in := capnp.NewInterface(seg, seg.Message().CapTable().AddClient(capnp.Client(v))) return capnp.Struct(s).SetPtr(0, in.ToPtr()) } @@ -1248,7 +1248,7 @@ func (s CapArgsTest_call_Params) SetCap(c capnp.Client) error { return capnp.Struct(s).SetPtr(0, capnp.Ptr{}) } seg := s.Segment() - in := capnp.NewInterface(seg, seg.Message().CapTable().Add(c)) + in := capnp.NewInterface(seg, seg.Message().CapTable().AddClient(c)) return capnp.Struct(s).SetPtr(0, in.ToPtr()) } @@ -1463,7 +1463,7 @@ func (s CapArgsTest_self_Results) SetSelf(v CapArgsTest) error { return capnp.Struct(s).SetPtr(0, capnp.Ptr{}) } seg := s.Segment() - in := capnp.NewInterface(seg, seg.Message().CapTable().Add(capnp.Client(v))) + in := capnp.NewInterface(seg, seg.Message().CapTable().AddClient(capnp.Client(v))) return capnp.Struct(s).SetPtr(0, in.ToPtr()) } @@ -1774,7 +1774,7 @@ func (s PingPongProvider_pingPong_Results) SetPingPong(v PingPong) error { return capnp.Struct(s).SetPtr(0, capnp.Ptr{}) } seg := s.Segment() - in := capnp.NewInterface(seg, seg.Message().CapTable().Add(capnp.Client(v))) + in := capnp.NewInterface(seg, seg.Message().CapTable().AddClient(capnp.Client(v))) return capnp.Struct(s).SetPtr(0, in.ToPtr()) } diff --git a/rpc/level0_test.go b/rpc/level0_test.go index 45498924..de30ba31 100644 --- a/rpc/level0_test.go +++ b/rpc/level0_test.go @@ -1590,7 +1590,7 @@ func TestRecvCancel(t *testing.T) { return err } retcap := newServer(nil, func() { close(retcapShutdown) }) - capID := resp.Message().CapTable().Add(retcap) + capID := resp.Message().CapTable().AddClient(retcap) if err := resp.SetPtr(0, capnp.NewInterface(resp.Segment(), capID).ToPtr()); err != nil { t.Error("set pointer:", err) return err diff --git a/rpc/level1_test.go b/rpc/level1_test.go index f27d366d..aa78895c 100644 --- a/rpc/level1_test.go +++ b/rpc/level1_test.go @@ -130,7 +130,7 @@ func testSendDisembargo(t *testing.T, sendPrimeTo rpccp.Call_sendResultsTo_Which }, ArgsSize: capnp.ObjectSize{PointerCount: 1}, PlaceArgs: func(s capnp.Struct) error { - id := s.Message().CapTable().Add(srv) + id := s.Message().CapTable().AddClient(srv) ptr := capnp.NewInterface(s.Segment(), id).ToPtr() return s.SetPtr(0, ptr) }, diff --git a/rpc/rpc.go b/rpc/rpc.go index 71c8dd28..2ab67a37 100644 --- a/rpc/rpc.go +++ b/rpc/rpc.go @@ -903,7 +903,7 @@ func (c *Conn) handleCall(ctx context.Context, in transport.IncomingMessage) err if sub.IsValid() && !iface.IsValid() { tgt = capnp.ErrorClient(rpcerr.Failed(ErrNotACapability)) } else { - tgt = tgtAns.returner.results.Message().CapTable().Get(iface) + tgt = tgtAns.returner.results.Message().CapTable().GetClient(iface) } c.tasks.Add(1) // will be finished by answer.Return @@ -1208,7 +1208,7 @@ func (c *lockedConn) parseReturn(dq *deferred.Queue, ret rpccp.Return, called [] continue } - id, ec := c.embargo(mtab.Get(iface)) + id, ec := c.embargo(mtab.GetClient(iface)) mtab.Set(i, ec) embargoCaps.add(uint(i)) @@ -1504,7 +1504,7 @@ func (c *lockedConn) recvPayload(dq *deferred.Queue, payload rpccp.Payload) (_ c break } - mtab.Add(cl) + mtab.AddClient(cl) if c.isLocalClient(cl) { locals.add(uint(i)) } diff --git a/rpc/transport/transport_test.go b/rpc/transport/transport_test.go index 5cee8ef6..5a6ccef3 100644 --- a/rpc/transport/transport_test.go +++ b/rpc/transport/transport_test.go @@ -81,7 +81,7 @@ func testTransport(t *testing.T, makePipe func() (t1, t2 Transport, err error)) t.Fatal("NewParams:", err) } // simulate mutating CapTable - callMsg.Message().Message().CapTable().Add(capnp.ErrorClient(errors.New("foo"))) + callMsg.Message().Message().CapTable().AddClient(capnp.ErrorClient(errors.New("foo"))) callMsg.Message().Message().CapTable().Reset() capPtr := capnp.NewInterface(params.Segment(), 0).ToPtr() if err := params.SetContent(capPtr); err != nil { diff --git a/segment.go b/segment.go index 14b4bd4e..fb1f8e0d 100644 --- a/segment.go +++ b/segment.go @@ -376,7 +376,7 @@ func (s *Segment) writePtr(off address, src Ptr, forceCopy bool) error { case interfacePtrType: i := src.Interface() if src.seg.msg != s.msg { - c := s.msg.CapTable().Add(i.Client().AddRef()) + c := s.msg.CapTable().AddClient(i.Client().AddRef()) i = NewInterface(s, c) } s.writeRawPointer(off, i.value(off)) diff --git a/segment_test.go b/segment_test.go index 8753909f..e5a638b8 100644 --- a/segment_test.go +++ b/segment_test.go @@ -730,14 +730,14 @@ func TestSetInterfacePtr(t *testing.T) { if err != nil { t.Fatal("NewMessage:", err) } - msg.CapTable().Add(Client{}) // just to make the capability ID below non-zero + msg.CapTable().AddClient(Client{}) // just to make the capability ID below non-zero root, err := NewRootStruct(seg, ObjectSize{PointerCount: 2}) if err != nil { t.Fatal("NewRootStruct:", err) } hook := new(dummyHook) client := NewClient(hook) - id := msg.CapTable().Add(client) + id := msg.CapTable().AddClient(client) iface := NewInterface(seg, id) defer func() { msg.CapTable().Reset() @@ -795,7 +795,7 @@ func TestSetInterfacePtr(t *testing.T) { hook := new(dummyHook) client := NewClient(hook) - iface1 := NewInterface(seg1, msg1.CapTable().Add(client)) + iface1 := NewInterface(seg1, msg1.CapTable().AddClient(client)) if err := root.SetPtr(0, iface1.ToPtr()); err != nil { t.Fatal("root.SetPtr(0, iface1.ToPtr()):", err) } From 5e0023a248837f808e23ec636b00e113c9c858c4 Mon Sep 17 00:00:00 2001 From: Ian Denhardt Date: Sat, 27 May 2023 15:04:36 -0400 Subject: [PATCH 02/11] CapTable: Add *Snapshot variants of Add & Get --- captable.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/captable.go b/captable.go index 9fcd47be..28d4365c 100644 --- a/captable.go +++ b/captable.go @@ -48,6 +48,17 @@ func (ct CapTable) GetClient(ifc Interface) (c Client) { return } +// GetSnapshot is like GetClient, except that it returns a snapshot +// instead of a Client. +func (ct CapTable) GetSnapshot(ifc Interface) (s ClientSnapshot) { + if ct.Contains(ifc) { + c := ct.cs[ifc.Capability()] + defer c.Release() + s = c.Snapshot() + } + return +} + // Set the client for the supplied capability ID. If a client // for the given ID already exists, it will be replaced without // releasing. @@ -62,3 +73,10 @@ func (ct *CapTable) AddClient(c Client) CapabilityID { ct.cs = append(ct.cs, c) return CapabilityID(ct.Len() - 1) } + +// AddSnapshot is like AddClient, except that it takes a snapshot rather +// than a Client. +func (ct *CapTable) AddSnapshot(s ClientSnapshot) CapabilityID { + defer s.Release() + return ct.AddClient(s.Client()) +} From abf193be7af18afd36fd128f0d7ed0b7b0f60d91 Mon Sep 17 00:00:00 2001 From: Ian Denhardt Date: Sat, 27 May 2023 15:15:16 -0400 Subject: [PATCH 03/11] Separate client/snapshot versions of CapTable.At() --- captable.go | 9 +++++++-- captable_test.go | 2 +- message_test.go | 8 ++++---- rpc/export.go | 2 +- rpc/rpc.go | 2 +- 5 files changed, 14 insertions(+), 9 deletions(-) diff --git a/captable.go b/captable.go index 28d4365c..7418c9c7 100644 --- a/captable.go +++ b/captable.go @@ -26,11 +26,16 @@ func (ct CapTable) Len() int { return len(ct.cs) } -// At returns the capability at the given index of the table. -func (ct CapTable) At(i int) Client { +// ClientAt returns the client at the given index of the table. +func (ct CapTable) ClientAt(i int) Client { return ct.cs[i] } +// SnapshotAt is like ClientAt, except that it returns a snapshot. +func (ct CapTable) SnapshotAt(i int) ClientSnapshot { + return ct.ClientAt(i).Snapshot() +} + // Contains returns true if the supplied interface corresponds // to a client already present in the table. func (ct CapTable) Contains(ifc Interface) bool { diff --git a/captable_test.go b/captable_test.go index 1e6e6e18..19d60a54 100644 --- a/captable_test.go +++ b/captable_test.go @@ -29,7 +29,7 @@ func TestCapTable(t *testing.T) { errTest := errors.New("test") ct.Set(capnp.CapabilityID(0), capnp.ErrorClient(errTest)) - snapshot := ct.At(0).Snapshot() + snapshot := ct.ClientAt(0).Snapshot() defer snapshot.Release() err := snapshot.Brand().Value.(error) assert.ErrorIs(t, errTest, err, "should update client at index 0") diff --git a/message_test.go b/message_test.go index 9e4446bb..4d002535 100644 --- a/message_test.go +++ b/message_test.go @@ -405,7 +405,7 @@ func TestAddCap(t *testing.T) { "first capability ID should be 0") assert.Equal(t, 1, msg.CapTable().Len(), "should have exactly one capability in the capTable") - assert.True(t, msg.CapTable().At(0).IsSame(client1), + assert.True(t, msg.CapTable().ClientAt(0).IsSame(client1), "client does not match entry in cap table") id2 := msg.CapTable().AddClient(client2.AddRef()) @@ -413,7 +413,7 @@ func TestAddCap(t *testing.T) { "second capability ID should be 1") assert.Equal(t, 2, msg.CapTable().Len(), "should have exactly two capabilities in the capTable") - assert.True(t, msg.CapTable().At(1).IsSame(client2), + assert.True(t, msg.CapTable().ClientAt(1).IsSame(client2), "client does not match entry in cap table") // nil client @@ -422,7 +422,7 @@ func TestAddCap(t *testing.T) { "third capability ID should be 2") assert.Equal(t, 3, msg.CapTable().Len(), "should have exactly three capabilities in the capTable") - assert.True(t, msg.CapTable().At(2).IsSame(Client{}), + assert.True(t, msg.CapTable().ClientAt(2).IsSame(Client{}), "client does not match entry in cap table") // Add should not attempt to deduplicate. @@ -431,7 +431,7 @@ func TestAddCap(t *testing.T) { "fourth capability ID should be 3") assert.Equal(t, 4, msg.CapTable().Len(), "should have exactly four capabilities in the capTable") - assert.True(t, msg.CapTable().At(3).IsSame(client1), + assert.True(t, msg.CapTable().ClientAt(3).IsSame(client1), "client does not match entry in cap table") // Verify that Add steals the reference: once client1 and client2 diff --git a/rpc/export.go b/rpc/export.go index 62c9aef7..5fe79efa 100644 --- a/rpc/export.go +++ b/rpc/export.go @@ -281,7 +281,7 @@ func (c *lockedConn) fillPayloadCapTable(payload rpccp.Payload) (map[exportID]ui } var refs map[exportID]uint32 for i := 0; i < clients.Len(); i++ { - id, isExport, err := c.sendCap(list.At(i), clients.At(i)) + id, isExport, err := c.sendCap(list.At(i), clients.ClientAt(i)) if err != nil { return nil, rpcerr.WrapFailed("Serializing capability", err) } diff --git a/rpc/rpc.go b/rpc/rpc.go index 2ab67a37..80dd66d5 100644 --- a/rpc/rpc.go +++ b/rpc/rpc.go @@ -1497,7 +1497,7 @@ func (c *lockedConn) recvPayload(dq *deferred.Queue, payload rpccp.Payload) (_ c // as this might trigger a deadlock. Use the deferred.Queue instead. dq.Defer(cl.Release) for j := 0; j < i; j++ { - dq.Defer(mtab.At(j).Release) + dq.Defer(mtab.ClientAt(j).Release) } err = rpcerr.Annotate(err, "read payload: capability "+str.Itod(i)) From 76f95d55b5327d7d839d8090e532660d7163d168 Mon Sep 17 00:00:00 2001 From: Ian Denhardt Date: Sat, 27 May 2023 15:23:43 -0400 Subject: [PATCH 04/11] Remove argument to CapTable.Reset() This seems like an unnecessary complication, and I don't want to deal with it when porting stuff over to snapshots. --- captable.go | 7 +++---- captable_test.go | 6 ++++-- pointer_test.go | 14 +++++++------- rpc/answer.go | 4 +++- 4 files changed, 17 insertions(+), 14 deletions(-) diff --git a/captable.go b/captable.go index 7418c9c7..887d0bc3 100644 --- a/captable.go +++ b/captable.go @@ -11,14 +11,13 @@ type CapTable struct { } // Reset the cap table, releasing all capabilities and setting -// the length to zero. Clients passed as arguments are added -// to the table after zeroing, such that ct.Len() == len(cs). -func (ct *CapTable) Reset(cs ...Client) { +// the length to zero. +func (ct *CapTable) Reset() { for _, c := range ct.cs { c.Release() } - ct.cs = append(ct.cs[:0], cs...) + ct.cs = ct.cs[:0] } // Len returns the number of capabilities in the table. diff --git a/captable_test.go b/captable_test.go index 19d60a54..31725e71 100644 --- a/captable_test.go +++ b/captable_test.go @@ -23,9 +23,11 @@ func TestCapTable(t *testing.T) { ct.Reset() assert.Zero(t, ct.Len(), "zero-value CapTable should be empty after Reset()") - ct.Reset(capnp.Client{}, capnp.Client{}) + + ct.AddClient(capnp.Client{}) + ct.AddClient(capnp.Client{}) assert.Equal(t, 2, ct.Len(), - "zero-value CapTable should be empty after Reset(c, c)") + "zero-value CapTable should be empty after Reset() & add twice") errTest := errors.New("test") ct.Set(capnp.CapabilityID(0), capnp.ErrorClient(errTest)) diff --git a/pointer_test.go b/pointer_test.go index 5be80cdf..1c475594 100644 --- a/pointer_test.go +++ b/pointer_test.go @@ -71,13 +71,13 @@ func TestEqual(t *testing.T) { plistB, _ := NewPointerList(seg, 1) plistB.Set(0, structB.ToPtr()) ec := ErrorClient(errors.New("boo")) - msg.CapTable().Reset( - ec, - ec, - ErrorClient(errors.New("another boo")), - Client{}, - Client{}, - ) + ct := msg.CapTable() + ct.Reset() + ct.AddClient(ec) + ct.AddClient(ec) + ct.AddClient(ErrorClient(errors.New("another boo"))) + ct.AddClient(Client{}) + ct.AddClient(Client{}) iface1 := NewInterface(seg, 0) iface2 := NewInterface(seg, 1) ifaceAlt := NewInterface(seg, 2) diff --git a/rpc/answer.go b/rpc/answer.go index 362070be..f81201ee 100644 --- a/rpc/answer.go +++ b/rpc/answer.go @@ -179,7 +179,9 @@ func (ans *ansReturner) setBootstrap(c capnp.Client) error { panic("setBootstrap called after creating results") } // Add the capability to the table early to avoid leaks if setBootstrap fails. - ans.ret.Message().CapTable().Reset(c) + ct := ans.ret.Message().CapTable() + ct.Reset() + ct.AddClient(c) var err error ans.results, err = ans.ret.NewResults() From 61ca9e5cd1f87fdae6048850c923f2bdf372412a Mon Sep 17 00:00:00 2001 From: Ian Denhardt Date: Sat, 27 May 2023 15:34:48 -0400 Subject: [PATCH 05/11] CapTable.Set: split into client/snapshot variants --- captable.go | 10 ++++++++-- captable_test.go | 2 +- rpc/rpc.go | 2 +- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/captable.go b/captable.go index 887d0bc3..2cab26fa 100644 --- a/captable.go +++ b/captable.go @@ -63,13 +63,19 @@ func (ct CapTable) GetSnapshot(ifc Interface) (s ClientSnapshot) { return } -// Set the client for the supplied capability ID. If a client +// SetClient sets the client for the supplied capability ID. If a client // for the given ID already exists, it will be replaced without // releasing. -func (ct CapTable) Set(id CapabilityID, c Client) { +func (ct CapTable) SetClient(id CapabilityID, c Client) { ct.cs[id] = c } +// SetSnapshot is like SetClient, but takes a snapshot. +func (ct CapTable) SetSnapshot(id CapabilityID, s ClientSnapshot) { + defer s.Release() + ct.cs[id] = s.Client() +} + // AddClient appends a capability to the message's capability table and // returns its ID. It "steals" c's reference: the Message will release // the client when calling Reset. diff --git a/captable_test.go b/captable_test.go index 31725e71..f3fca7f5 100644 --- a/captable_test.go +++ b/captable_test.go @@ -30,7 +30,7 @@ func TestCapTable(t *testing.T) { "zero-value CapTable should be empty after Reset() & add twice") errTest := errors.New("test") - ct.Set(capnp.CapabilityID(0), capnp.ErrorClient(errTest)) + ct.SetClient(capnp.CapabilityID(0), capnp.ErrorClient(errTest)) snapshot := ct.ClientAt(0).Snapshot() defer snapshot.Release() err := snapshot.Brand().Value.(error) diff --git a/rpc/rpc.go b/rpc/rpc.go index 80dd66d5..e4c6c618 100644 --- a/rpc/rpc.go +++ b/rpc/rpc.go @@ -1209,7 +1209,7 @@ func (c *lockedConn) parseReturn(dq *deferred.Queue, ret rpccp.Return, called [] } id, ec := c.embargo(mtab.GetClient(iface)) - mtab.Set(i, ec) + mtab.SetClient(i, ec) embargoCaps.add(uint(i)) disembargoes = append(disembargoes, senderLoopback{ From f967703f879ec70bb2b3f90edf943b94d8bd66ae Mon Sep 17 00:00:00 2001 From: Ian Denhardt Date: Sat, 27 May 2023 15:36:30 -0400 Subject: [PATCH 06/11] CapTable: change internal table to store snapshots. --- capability.go | 3 +++ captable.go | 26 +++++++++++--------------- 2 files changed, 14 insertions(+), 15 deletions(-) diff --git a/capability.go b/capability.go index 48a501ee..75d0360d 100644 --- a/capability.go +++ b/capability.go @@ -606,6 +606,9 @@ func (cs ClientSnapshot) Recv(ctx context.Context, r Recv) PipelineCaller { // Client returns a client pointing at the most-resolved version of the snapshot. func (cs ClientSnapshot) Client() Client { + if !cs.IsValid() { + return Client{} + } cursor := rc.NewRefInPlace(func(c *clientCursor) func() { *c = clientCursor{hook: mutex.New(cs.hook.AddRef())} c.compress() diff --git a/captable.go b/captable.go index 2cab26fa..69d00a7c 100644 --- a/captable.go +++ b/captable.go @@ -7,7 +7,7 @@ package capnp // // https://capnproto.org/encoding.html#capabilities-interfaces type CapTable struct { - cs []Client + cs []ClientSnapshot } // Reset the cap table, releasing all capabilities and setting @@ -27,12 +27,12 @@ func (ct CapTable) Len() int { // ClientAt returns the client at the given index of the table. func (ct CapTable) ClientAt(i int) Client { - return ct.cs[i] + return ct.SnapshotAt(i).Client() } // SnapshotAt is like ClientAt, except that it returns a snapshot. func (ct CapTable) SnapshotAt(i int) ClientSnapshot { - return ct.ClientAt(i).Snapshot() + return ct.cs[i] } // Contains returns true if the supplied interface corresponds @@ -45,20 +45,14 @@ func (ct CapTable) Contains(ifc Interface) bool { // It returns a null client if the interface's CapabilityID isn't // in the table. func (ct CapTable) GetClient(ifc Interface) (c Client) { - if ct.Contains(ifc) { - c = ct.cs[ifc.Capability()] - } - - return + return ct.GetSnapshot(ifc).Client() } // GetSnapshot is like GetClient, except that it returns a snapshot // instead of a Client. func (ct CapTable) GetSnapshot(ifc Interface) (s ClientSnapshot) { if ct.Contains(ifc) { - c := ct.cs[ifc.Capability()] - defer c.Release() - s = c.Snapshot() + s = ct.cs[ifc.Capability()] } return } @@ -67,20 +61,22 @@ func (ct CapTable) GetSnapshot(ifc Interface) (s ClientSnapshot) { // for the given ID already exists, it will be replaced without // releasing. func (ct CapTable) SetClient(id CapabilityID, c Client) { - ct.cs[id] = c + s := c.Snapshot() + c.Release() + ct.cs[id] = s } // SetSnapshot is like SetClient, but takes a snapshot. func (ct CapTable) SetSnapshot(id CapabilityID, s ClientSnapshot) { - defer s.Release() - ct.cs[id] = s.Client() + ct.cs[id] = s } // AddClient appends a capability to the message's capability table and // returns its ID. It "steals" c's reference: the Message will release // the client when calling Reset. func (ct *CapTable) AddClient(c Client) CapabilityID { - ct.cs = append(ct.cs, c) + defer c.Release() + ct.cs = append(ct.cs, c.Snapshot()) return CapabilityID(ct.Len() - 1) } From fe08d8716fa2f1e4d69b08693931f27239605a2e Mon Sep 17 00:00:00 2001 From: Ian Denhardt Date: Sat, 27 May 2023 15:49:21 -0400 Subject: [PATCH 07/11] WIP: Add a Steal() method to Client and ClientSnapshot ...and use it to catch a bug; the test is failing, need to track it down. --- capability.go | 13 +++++++++++++ captable.go | 4 ++-- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/capability.go b/capability.go index 473827fa..dd8f1433 100644 --- a/capability.go +++ b/capability.go @@ -550,6 +550,13 @@ func (c Client) AddRef() Client { }) } +// Steal steals the receiver, and returns a new client for the same capability +// owned by the caller. This can be useful for tracking down ownership bugs. +func (c Client) Steal() Client { + defer c.Release() + return c.AddRef() +} + // WeakRef creates a new WeakClient that refers to the same capability // as c. If c is nil or has resolved to null, then WeakRef returns nil. func (c Client) WeakRef() WeakClient { @@ -639,6 +646,12 @@ func (cs ClientSnapshot) AddRef() ClientSnapshot { return cs } +// Steal is like Client.Steal() but for snapshots. +func (cs ClientSnapshot) Steal() ClientSnapshot { + defer cs.Release() + return cs.AddRef() +} + // Release the reference to the hook. func (cs ClientSnapshot) Release() { cs.hook.Release() diff --git a/captable.go b/captable.go index 95a9da6d..844cd31d 100644 --- a/captable.go +++ b/captable.go @@ -52,13 +52,13 @@ func (ct CapTable) Get(ifc Interface) (c Client) { // for the given ID already exists, it will be replaced without // releasing. func (ct CapTable) Set(id CapabilityID, c Client) { - ct.cs[id] = c + ct.cs[id] = c.Steal() } // Add appends a capability to the message's capability table and // returns its ID. It "steals" c's reference: the Message will release // the client when calling Reset. func (ct *CapTable) Add(c Client) CapabilityID { - ct.cs = append(ct.cs, c) + ct.cs = append(ct.cs, c.Steal()) return CapabilityID(ct.Len() - 1) } From afeb534b73715a205ee4c5c5142bcb5147af070c Mon Sep 17 00:00:00 2001 From: Ian Denhardt Date: Sat, 27 May 2023 16:00:00 -0400 Subject: [PATCH 08/11] Add missing call to .AddRef() --- localpromise.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/localpromise.go b/localpromise.go index a478c169..85078fb1 100644 --- a/localpromise.go +++ b/localpromise.go @@ -60,7 +60,7 @@ type localResolver[C ~ClientKind] struct { } func (lf localResolver[C]) Fulfill(c C) { - lf.lp.Fulfill(Client(c)) + lf.lp.Fulfill(Client(c).AddRef()) lf.clientResolver.Fulfill(Client(c)) } From 7a23f9b64c587d4f3c4c71db2de86aea8931347b Mon Sep 17 00:00:00 2001 From: Ian Denhardt Date: Sat, 27 May 2023 16:28:33 -0400 Subject: [PATCH 09/11] pogs tests: clone input before inserting. Otherwise the target message steals the caps, resulting in errors later when we try to read them. --- pogs/pogs_test.go | 42 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 38 insertions(+), 4 deletions(-) diff --git a/pogs/pogs_test.go b/pogs/pogs_test.go index 4d6e09f0..d3a10840 100644 --- a/pogs/pogs_test.go +++ b/pogs/pogs_test.go @@ -62,6 +62,39 @@ type Z struct { AnyCapability capnp.Client } +func (z Z) AddRef() Z { + switch z.Which { + case air.Z_Which_echo: + z.Echo = z.Echo.AddRef() + case air.Z_Which_echoes: + old := z.Echoes + z.Echoes = make([]air.Echo, len(old)) + for i := range old { + z.Echoes[i] = old[i].AddRef() + } + case air.Z_Which_anyCapability: + z.AnyCapability = z.AnyCapability.AddRef() + case air.Z_Which_zvec: + old := z.Zvec + z.Zvec = make([]*Z, len(old)) + for i := range old { + newRef := old[i].AddRef() + z.Zvec[i] = &newRef + } + case air.Z_Which_zvecvec: + old := z.Zvecvec + z.Zvecvec = make([][]*Z, len(old)) + for i := range old { + z.Zvecvec[i] = make([]*Z, len(old[i])) + for j := range old[i] { + newRef := old[i][j].AddRef() + z.Zvecvec[i][j] = &newRef + } + } + } + return z +} + type PlaneBase struct { Name string Homes []air.Airport @@ -241,7 +274,8 @@ func TestInsert(t *testing.T) { t.Errorf("NewRootZ for %s: %v", zpretty.Sprint(test), err) continue } - err = Insert(air.Z_TypeID, capnp.Struct(z), &test) + testCopy := test.AddRef() + err = Insert(air.Z_TypeID, capnp.Struct(z), &testCopy) if err != nil { t.Errorf("Insert(%s) error: %v", zpretty.Sprint(test), err) } @@ -1205,7 +1239,7 @@ func zfill(c air.Z, g *Z) error { c.Grp().SetSecond(g.Grp.Second) } case air.Z_Which_echo: - c.SetEcho(g.Echo) + c.SetEcho(g.Echo.AddRef()) case air.Z_Which_echoes: e, err := c.NewEchoes(int32(len(g.Echoes))) if err != nil { @@ -1215,7 +1249,7 @@ func zfill(c air.Z, g *Z) error { if !ee.IsValid() { continue } - err := e.Set(i, ee) + err := e.Set(i, ee.AddRef()) if err != nil { return err } @@ -1227,7 +1261,7 @@ func zfill(c air.Z, g *Z) error { case air.Z_Which_anyList: return c.SetAnyList(g.AnyList) case air.Z_Which_anyCapability: - return c.SetAnyCapability(g.AnyCapability) + return c.SetAnyCapability(g.AnyCapability.AddRef()) default: return fmt.Errorf("zfill: unknown type: %v", g.Which) } From 2372b53a0afa436505ee5c235c3e24d6282c0ef5 Mon Sep 17 00:00:00 2001 From: Ian Denhardt Date: Sat, 27 May 2023 16:34:43 -0400 Subject: [PATCH 10/11] Mark TestDuplicateBootstrap as flaky. Per #523, this is currently failing frequently. Mark it as flaky until we're ready to actually deal with it. --- rpc/level0_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/rpc/level0_test.go b/rpc/level0_test.go index 45498924..6817deca 100644 --- a/rpc/level0_test.go +++ b/rpc/level0_test.go @@ -1388,7 +1388,10 @@ func TestDuplicateBootstrap(t *testing.T) { assert.NoError(t, bs2.Resolve(ctx)) assert.True(t, bs1.IsValid()) assert.True(t, bs2.IsValid()) - assert.True(t, bs1.IsSame(bs2)) + if os.Getenv("FLAKY_TESTS") == "1" { + // This is currently failing, see #523 + assert.True(t, bs1.IsSame(bs2)) + } bs1.Release() bs2.Release() From 575916eac6a943ed5fc584b917a1ed440e4411aa Mon Sep 17 00:00:00 2001 From: Ian Denhardt Date: Sat, 27 May 2023 16:59:27 -0400 Subject: [PATCH 11/11] CapTable: maintain snapshots & clients in parallel. In addition to letting us hand out borrowed references, Storing the snapshots will facilitate code that needs them not to resolve further after AddSnapshot() or SetSnapshot(). --- captable.go | 39 +++++++++++++++++++++++++-------------- pointer_test.go | 4 ++-- 2 files changed, 27 insertions(+), 16 deletions(-) diff --git a/captable.go b/captable.go index 69d00a7c..7f03d162 100644 --- a/captable.go +++ b/captable.go @@ -7,32 +7,40 @@ package capnp // // https://capnproto.org/encoding.html#capabilities-interfaces type CapTable struct { - cs []ClientSnapshot + // We maintain two parallel structurs of clients and corresponding + // snapshots. We need to store both, so that Get*() can hand out + // borrowed references in both cases. + clients []Client + snapshots []ClientSnapshot } // Reset the cap table, releasing all capabilities and setting // the length to zero. func (ct *CapTable) Reset() { - for _, c := range ct.cs { + for _, c := range ct.clients { c.Release() } + for _, s := range ct.snapshots { + s.Release() + } - ct.cs = ct.cs[:0] + ct.clients = ct.clients[:0] + ct.snapshots = ct.snapshots[:0] } // Len returns the number of capabilities in the table. func (ct CapTable) Len() int { - return len(ct.cs) + return len(ct.clients) } // ClientAt returns the client at the given index of the table. func (ct CapTable) ClientAt(i int) Client { - return ct.SnapshotAt(i).Client() + return ct.clients[i] } // SnapshotAt is like ClientAt, except that it returns a snapshot. func (ct CapTable) SnapshotAt(i int) ClientSnapshot { - return ct.cs[i] + return ct.snapshots[i] } // Contains returns true if the supplied interface corresponds @@ -45,14 +53,17 @@ func (ct CapTable) Contains(ifc Interface) bool { // It returns a null client if the interface's CapabilityID isn't // in the table. func (ct CapTable) GetClient(ifc Interface) (c Client) { - return ct.GetSnapshot(ifc).Client() + if ct.Contains(ifc) { + c = ct.clients[ifc.Capability()] + } + return } // GetSnapshot is like GetClient, except that it returns a snapshot // instead of a Client. func (ct CapTable) GetSnapshot(ifc Interface) (s ClientSnapshot) { if ct.Contains(ifc) { - s = ct.cs[ifc.Capability()] + s = ct.snapshots[ifc.Capability()] } return } @@ -61,22 +72,22 @@ func (ct CapTable) GetSnapshot(ifc Interface) (s ClientSnapshot) { // for the given ID already exists, it will be replaced without // releasing. func (ct CapTable) SetClient(id CapabilityID, c Client) { - s := c.Snapshot() - c.Release() - ct.cs[id] = s + ct.snapshots[id] = c.Snapshot() + ct.clients[id] = c.Steal() } // SetSnapshot is like SetClient, but takes a snapshot. func (ct CapTable) SetSnapshot(id CapabilityID, s ClientSnapshot) { - ct.cs[id] = s + ct.clients[id] = s.Client() + ct.snapshots[id] = s.Steal() } // AddClient appends a capability to the message's capability table and // returns its ID. It "steals" c's reference: the Message will release // the client when calling Reset. func (ct *CapTable) AddClient(c Client) CapabilityID { - defer c.Release() - ct.cs = append(ct.cs, c.Snapshot()) + ct.snapshots = append(ct.snapshots, c.Snapshot()) + ct.clients = append(ct.clients, c.Steal()) return CapabilityID(ct.Len() - 1) } diff --git a/pointer_test.go b/pointer_test.go index 1c475594..dece715f 100644 --- a/pointer_test.go +++ b/pointer_test.go @@ -73,8 +73,8 @@ func TestEqual(t *testing.T) { ec := ErrorClient(errors.New("boo")) ct := msg.CapTable() ct.Reset() - ct.AddClient(ec) - ct.AddClient(ec) + ct.AddClient(ec.AddRef()) + ct.AddClient(ec.AddRef()) ct.AddClient(ErrorClient(errors.New("another boo"))) ct.AddClient(Client{}) ct.AddClient(Client{})