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.

Sign up for GitHub

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

Jump to bottom

Fixes wrt. withLocked #414

Merged
merged 2 commits into from
Jan 5, 2023
Merged

Fixes wrt. withLocked #414

merged 2 commits into from
Jan 5, 2023

Conversation

zenhack
Copy link
Contributor

@zenhack zenhack commented Jan 4, 2023

The first commit adds a lockedConn parameter to answer.sendException, for symmetry with answer.sendReturn. This resulted in a cascade of type errors, which that commit also fixes.

While doing that, I noticed a FIXME and decided to just fix it by passing stuff off to a releaseList, in the second commit. Most likely the (now removed) comment was written either when I was in the middle of something else or before releaseLists were clearly the right option for this sort of thing. This potentially removes a deadlock, though I've observed no symptoms.

@zenhack
type safety re: lockedConn/answer.sendException.
8533ceb 
This patch adds a lockedConn parameter to answer.sendException to mirror
sendReturn... and then follows the type errors, which results in a lot
of changes.
@zenhack
Fix a FIXME.
3f8d908 
Pass in a releaseList and add stuff to that; it is not in general safe
to call client.Release() when holding a lock.
@lthibault
Copy link
Collaborator 

==================
WARNING: DATA RACE
Read at 0x00c0000b8128 by goroutine 1023:
  capnproto.org/go/capnp/v3.(*Segment).slice()
      /home/runner/work/go-capnproto2/go-capnproto2/segment.go:53 +0x5a
  capnproto.org/go/capnp/v3.(*Segment).readUint64()
      /home/runner/work/go-capnproto2/go-capnproto2/segment.go:69 +0x51
  capnproto.org/go/capnp/v3.(*Segment).readRawPointer()
      /home/runner/work/go-capnproto2/go-capnproto2/segment.go:73 +0x50
  capnproto.org/go/capnp/v3.(*Segment).resolveFarPointer()
      /home/runner/work/go-capnproto2/go-capnproto2/segment.go:239 +0x4f
  capnproto.org/go/capnp/v3.(*Segment).readPtr()
      /home/runner/work/go-capnproto2/go-capnproto2/segment.go:120 +0x6f
  capnproto.org/go/capnp/v3.Struct.Ptr()
      /home/runner/work/go-capnproto2/go-capnproto2/struct.go:116 +0x119
  capnproto.org/go/capnp/v3.Transform()
      /home/runner/work/go-capnproto2/go-capnproto2/answer.go:581 +0x2ed
  capnproto.org/go/capnp/v3.resolution.ptr()
      /home/runner/work/go-capnproto2/go-capnproto2/answer.go:615 +0xfc
  capnproto.org/go/capnp/v3.resolution.client()
      /home/runner/work/go-capnproto2/go-capnproto2/answer.go:624 +0x96
  capnproto.org/go/capnp/v3.(*Answer).PipelineRecv()
      /home/runner/work/go-capnproto2/go-capnproto2/answer.go:394 +0x3f4
  capnproto.org/go/capnp/v3.(*Answer).PipelineRecv-fm()
      <autogenerated>:1 +0xc7
  capnproto.org/go/capnp/v3/server.queueCaller.PipelineRecv()
      /home/runner/work/go-capnproto2/go-capnproto2/server/answer.go:162 +0x2a2
  capnproto.org/go/capnp/v3/server.(*answerQueue).PipelineRecv()
      /home/runner/work/go-capnproto2/go-capnproto2/server/answer.go:137 +0xc9
  capnproto.org/go/capnp/v3/rpc.(*Conn).handleCall.func4.6()
      /home/runner/work/go-capnproto2/go-capnproto2/rpc/rpc.go:921 +0x155
  capnproto.org/go/capnp/v3/rpc.(*releaseList).Release()
      /home/runner/work/go-capnproto2/go-capnproto2/rpc/releaselist.go:11 +0x8c
  capnproto.org/go/capnp/v3/rpc.(*Conn).handleCall.func5()
      /home/runner/work/go-capnproto2/go-capnproto2/rpc/rpc.go:740 +0x39
  runtime.deferreturn()
      /opt/hostedtoolcache/go/1.18.9/x64/src/runtime/panic.go:436 +0x32
  capnproto.org/go/capnp/v3/rpc.(*Conn).receive()
      /home/runner/work/go-capnproto2/go-capnproto2/rpc/rpc.go:610 +0x5ee
  capnproto.org/go/capnp/v3/rpc.(*Conn).receive-fm()
      <autogenerated>:1 +0x39
  capnproto.org/go/capnp/v3/rpc.(*Conn).backgroundTask.func1()
      /home/runner/work/go-capnproto2/go-capnproto2/rpc/rpc.go:273 +0x92
  golang.org/x/sync/errgroup.(*Group).Go.func1()
      /home/runner/go/pkg/mod/golang.org/x/sync@v0.0.0-20201020160332-67f06af15bc9/errgroup/errgroup.go:57 +0x91

Previous write at 0x00c0000b8128 by goroutine 1476:
  capnproto.org/go/capnp/v3.alloc()
      /home/runner/work/go-capnproto2/go-capnproto2/message.go:358 +0x23c
  capnproto.org/go/capnp/v3.NewCompositeList()
      /home/runner/work/go-capnproto2/go-capnproto2/list.go:65 +0xe4
  capnproto.org/go/capnp/v3/std/capnp/rpc.NewCapDescriptor_List()
      /home/runner/work/go-capnproto2/go-capnproto2/std/capnp/rpc/rpc.capnp.go:2406 +0xce
  capnproto.org/go/capnp/v3/std/capnp/rpc.Payload.NewCapTable()
      /home/runner/work/go-capnproto2/go-capnproto2/std/capnp/rpc/rpc.capnp.go:2179 +0x84
  capnproto.org/go/capnp/v3/rpc.(*lockedConn).fillPayloadCapTable()
      /home/runner/work/go-capnproto2/go-capnproto2/rpc/export.go:182 +0x204
  capnproto.org/go/capnp/v3/rpc.(*answer).sendReturn()
      /home/runner/work/go-capnproto2/go-capnproto2/rpc/answer.go:232 +0x17d
  capnproto.org/go/capnp/v3/rpc.(*answer).Return.func2()
      /home/runner/work/go-capnproto2/go-capnproto2/rpc/answer.go:202 +0x58
  capnproto.org/go/capnp/v3/rpc.(*Conn).withLocked.func1()
      /home/runner/work/go-capnproto2/go-capnproto2/rpc/rpc.go:147 +0x3e
  capnproto.org/go/capnp/v3/internal/syncutil.With()
      /home/runner/work/go-capnproto2/go-capnproto2/internal/syncutil/with.go:12 +0xa2
  capnproto.org/go/capnp/v3/rpc.(*Conn).withLocked()
      /home/runner/work/go-capnproto2/go-capnproto2/rpc/rpc.go:146 +0x6d
  capnproto.org/go/capnp/v3/rpc.(*answer).Return()
      /home/runner/work/go-capnproto2/go-capnproto2/rpc/answer.go:201 +0x1a6
  capnproto.org/go/capnp/v3/server.(*Server).handleCall()
      /home/runner/work/go-capnproto2/go-capnproto2/server/server.go:214 +0x267
  capnproto.org/go/capnp/v3/server.(*Server).handleCalls.func2()
      /home/runner/work/go-capnproto2/go-capnproto2/server/server.go:182 +0x84
  capnproto.org/go/capnp/v3/server.(*Server).handleCalls()
      /home/runner/work/go-capnproto2/go-capnproto2/server/server.go:183 +0x20a
  capnproto.org/go/capnp/v3/server.New.func1()
      /home/runner/work/go-capnproto2/go-capnproto2/server/server.go:122 +0x58

Goroutine 1023 (running) created at:
  golang.org/x/sync/errgroup.(*Group).Go()
      /home/runner/go/pkg/mod/golang.org/x/sync@v0.0.0-20201020160332-67f06af15bc9/errgroup/errgroup.go:54 +0xee
  capnproto.org/go/capnp/v3/rpc.NewConn()
      /home/runner/work/go-capnproto2/go-capnproto2/rpc/rpc.go:242 +0x751
  capnproto.org/go/capnp/v3/rpc.TestCallReceiverAnswerRpc()
      /home/runner/work/go-capnproto2/go-capnproto2/rpc/receiveranswer_test.go:105 +0x2cc
  testing.tRunner()
      /opt/hostedtoolcache/go/1.18.9/x64/src/testing/testing.go:1439 +0x213
  testing.(*T).Run.func1()
      /opt/hostedtoolcache/go/1.18.9/x64/src/testing/testing.go:1486 +0x47

Goroutine 1476 (running) created at:
  capnproto.org/go/capnp/v3/server.New()
      /home/runner/work/go-capnproto2/go-capnproto2/server/server.go:122 +0x514
  capnproto.org/go/capnp/v3/rpc/internal/testcapnp.CapArgsTest_NewServer()
      /home/runner/work/go-capnproto2/go-capnproto2/rpc/internal/testcapnp/test.capnp.go:654 +0xc9
  capnproto.org/go/capnp/v3/rpc/internal/testcapnp.CapArgsTest_ServerToClient()
      /home/runner/work/go-capnproto2/go-capnproto2/rpc/internal/testcapnp/test.capnp.go:660 +0x36
  capnproto.org/go/capnp/v3/rpc.TestCallReceiverAnswerRpc()
      /home/runner/work/go-capnproto2/go-capnproto2/rpc/receiveranswer_test.go:108 +0x284
  testing.tRunner()
      /opt/hostedtoolcache/go/1.18.9/x64/src/testing/testing.go:1439 +0x213
  testing.(*T).Run.func1()
      /opt/hostedtoolcache/go/1.18.9/x64/src/testing/testing.go:1486 +0x47
==================

@zenhack zenhack merged commit 86da365 into capnproto:main Jan 5, 2023
@zenhack zenhack deleted the more-withLocked branch January 5, 2023 09:15
@zenhack zenhack mentioned this pull request Jan 9, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lthibault lthibault lthibault approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@zenhack @lthibault