Don't block when queuing methods on *Server. #225
Conversation
|
Got myself a stack trace, and several of the goroutines are blocked in |
This should fix capnproto#189, though right now the tests are deadlocking and I don't have the energy to debug, so I'm putting it down. This also obviates MaxConcurrentCalls; in the current version of the patch it is still present, but unused. Once this is working, a subsequent commit will remove the Policy struct. This significantly restructures the internals of *Server, and I think the result is easier to understand: the methods on *Server that start calls just stick the calls in a queue, and there is a goroutine that pulls them out and handles them. This even gets rid of Server.mu entirely, since nothing actually needed the lock anymore! Hopefully it will stay simple when I actually get it working.
zenhack
force-pushed
the
no-MaxConcurrentCalls
branch
from
94b64d7 to
c3601e8
Compare
|
Rebased, but unfortunately it's still deadlocking. The test that is deadlocking is |
See the comments documenting ordering requirements; previously, if the method call was blocking on its own context, it would not be stopped in response to server.Shutdown(), since the context for handleCalls is entirely independent.
|
Alright, fixed it. The fix is fiddlier than I would like; see the comments about ordering in the last commit. Open to suggestions for simpler solutions. I have let |
zenhack
changed the title
There was a problem hiding this comment.
LGTM.
It took me a while to grok the exact behavior of handleCall(s) and Ack(), so I'm wondering if it might not be a good idea to document the high-level process somewhere? Perhaps as a docstring for handleCalls?
Below is my understanding of the process, which I want to sanity-check with you, and which might also serve as a starting point for such a docstring.
- The default behavior is to process incoming method calls synchronously.
c.Ack()signals that the server start processing the next queued method call, and thatccan continue concurrently in the background.Ack()spawns another call tohandleCallsin a separate goroutinehandleCallsblocks onsrv.handleCall, until the call completes; if it determines that ack was called, it returns since another instance ofhandleCallsis running in the background (via step 3).
|
Quoting Louis Thibault (2022-04-18 12:52:14)
Below is my understanding of the process, which I want to sanity-check
with you, and which might also serve as a starting point for such a
docstring.
1. The default behavior is to process incoming method calls
synchronously.
2. c.Ack() signals that the server start processing the next queued
method call, and that c can continue concurrently in the
background.
3. Ack() spawns another call to handleCalls in a separate goroutine
4. handleCalls blocks on srv.handleCall, until the call completes; if
it determines that ack was called, it returns since another
instance of handleCalls is running in the background (via step 3).
Correct. And yeah, it could use high-level docs; I'll do that soonish.
|
Ok perfect. Merging in the meantime, as this is just a detail anyway. |
This should fix #189, though right now the tests are deadlocking and
I don't have the energy to debug, so I'm putting it down.
This also obviates MaxConcurrentCalls; in the current version of the
patch it is still present, but unused. Once this is working, a
subsequent commit will remove the Policy struct.
This significantly restructures the internals of *Server, and I
think the result is easier to understand: the methods on *Server
that start calls just stick the calls in a queue, and there is a
goroutine that pulls them out and handles them. This even gets rid
of Server.mu entirely, since nothing actually needed the lock
anymore!
Hopefully it will stay simple when I actually get it working.