Use sendLock to guard the entire stream allocation + write to wire
operation, and streamLock to only guard access to the underlying stream
map. This ensures the following:
- We uphold the constraint that new stream IDs on the wire are always
increasing, because whoever holds sendLock will be ensured to get the
next stream ID and be the next to write to the wire.
- Locks are always released in LIFO order. This prevents deadlocks.
Taking sendLock before releasing streamLock means that if a goroutine
blocks writing to the pipe, it can make another goroutine get stuck
trying to take sendLock, and therefore streamLock will be kept locked as
well. This can lead to the receiver goroutine no longer being able to
read responses from the pipe, since it needs to take streamLock when
processing a response. This ultimately leads to a complete deadlock of
the client.
It is reasonable for a server to block writes to the pipe if the client
is not reading responses fast enough. So we can't expect writes to never
block.
I have repro'd the hang with a simple ttrpc client and server. The
client spins up 100 goroutines that spam the server with requests
constantly. After a few seconds of running I can see it hang. I have set
the buffer size for the pipe to 0 to more easily repro, but it would
still be possible to hit with a larger buffer size (just may take a
higher volume of requests or larger payloads).
I also validated that I no longer see the hang with this fix, by leaving
the test client/server running for a few minutes. Obviously not 100%
conclusive, but before I could get a hang within several seconds of
running.
Signed-off-by: Kevin Parsons <kevpar@microsoft.com>
Add a WithChainUnaryClientInterceptor client option to allow
using more that one client call interceptor which will then
get chained and invoked in the order given.
This should allow us to implement opentelemetry instrumentation
as interceptors while allowing users to keep intercepting their
client calls for other reasons at the same time.
Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
Prevent panic from closing recv channel, which may be written to after
close. Use a separate channel to signal recv has closed and check that
channel on read and write.
Signed-off-by: Derek McGowan <derek@mcg.dev>
Implementation of the 1.2 protocol with support for streaming. Provides
the client and server interfaces for implementing services with
streaming.
Unary behavior is mostly unchanged and avoids extra stream tracking just
for unary calls. Streaming calls are tracked to route data to the
appropriate stream as it is received.
Stricter stream ID handling, disallowing unexpected re-use of stream
IDs.
Signed-off-by: Derek McGowan <derek@mcg.dev>
This change replaces github.com/gogo/protobuf with
google.golang.org/protobuf, except for the code generators.
All proto-encoded structs are now generated from .proto files,
which include ttrpc.Request and ttrpc.Response.
Signed-off-by: Kazuyoshi Kato <katokazu@amazon.com>
Changes the TTRPC client logic so that sending and receiving with the
server are in completely independent goroutines, with shared state
guarded by a mutex. Previously, sending/receiving were tied together by
reliance on a coordinator goroutine. This led to issues where if the
server was not reading from the connection, the client could get stuck
sending a request, causing the client to not read responses from the
server. See [1] for more details.
The new design sets up separate sending/receiving goroutines. These
share state in the form of the set of active calls that have been made
to the server. This state is encapsulated in the callMap type and access
is guarded by a mutex.
The main event loop in `run` previously handled a lot of state
management for the client. Now that most state is tracked by the
callMap, it mostly exists to notice when the client is closed and take
appropriate action to clean up.
Also did some minor code cleanup. For instance, the code was previously
written to support multiple receiver goroutines, though this was not
actually used. I've removed this for now, since the code is simpler this
way, and it's easy to add back if we actually need it in the future.
[1] https://github.com/containerd/ttrpc/issues/72
Signed-off-by: Kevin Parsons <kevpar@microsoft.com>
ttrpc provides WithOnClose option for user and ttrpc will call the
callback function when connection is closed unexpectedly or the ttrpc
client's Close() method is called. containerd runtime plugin uses it
to handle cleanup the resources created by containerd shim.
But the ttrpc client's Close() is only trigger and the shim's cleanup
resource callback is called asynchronously, which might make part of
resources leaky. There is an example from containerd-runtime-v2 for
runc:
```happy
[Task.Delete goroutine] [cleanupCallback goroutine]
call ttrpc client.Close() -->
read bundle and call runc delete
delete bundle
```
If the cleanupCallback is called after deleting bundle, the callback
will fail to call runc delete. If there is any running processes, the
resource becomes leaky.
```unhappy
[Task.Delete goroutine] [cleanupCallback goroutine]
call ttrpc client.Close() -->
delete bundle
failed to read bundle and call runc delete
```
In order to avoid this, introduces the UserOnCloseWait to make sure that
the cleanupCallback has been called synchronously, like:
```
[Task.Delete goroutine] [cleanupCallback goroutine]
call ttrpc client.Close() -->
wait for callback
read bundle and call runc delete
<-- finish sync
delete bundle
```
Signed-off-by: Wei Fu <fuweid89@gmail.com>
This copies the codes and status package from grpc as it is the only references
to the grpc project from ttrpc. This will help ensure that API breaking changes
in grpc do not affect ttrpc.
Signed-off-by: Michael Crosby <crosbymichael@gmail.com>
When call server.Close(), server will close all listener and notify
flighting-connection to shutdown. Connections are closed asynchronously.
In TestClientEOF, client can send request into closing-connection. But
the read for reply will return error if the closing-connection is
shutdown.
In this case, we should filter error for client side about `read:
connection reset by peer`.
Signed-off-by: Wei Fu <fuweid89@gmail.com>
To account for 5da5b1f225,
which is part of gRPC v1.23.0 and up, and after which gRPC no longer sets a
Status if no error occured.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Adds a new field to the `Request` type which specifies a timeout (in
nanoseconds) for the request. This is propagated on method dispatch as a
context timeout.
There was some discussion here on supporting a broader "metadata" field
(similar to grpc) that can be used for other things, but we ended up
with a dedicated field because it is lighter weight and expect it to be
used pretty heavily as is.... metadata may be added in the future, but
is not necessary for timeouts.
Also discussed using a deadline vs a timeout in the request and decided
to go with a timeout in order to deal with potential clock skew between
the client and server. This also has the side-effect of eliminating the
protocol/wire overhead from the request timeout.
Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Because `shutdownErr` will likely be `nil` in the close select branch,
returning it to waiters will result in the waiting `(*Client).Call`
returning `(nil, nil)`. This should take whatever is set for the client
as the exit condition, which is likely to be `ErrClosed`.
Signed-off-by: Stephen J Day <stephen.day@docker.com>
To gracefully handle scenarios where the connection is closed or the
client is closed, we now set the final error to be `ErrClosed`. Callers
can resolve it through using `errors.Cause` to detect this condition.
Signed-off-by: Stephen J Day <stephen.day@docker.com>
The request and response requests opened up a nasty race condition where
waiters could find themselves either blocked or receiving errant errors.
The result was low performance and inadvertent busy waits. This
refactors the client to have a single request into the main client loop,
eliminating the race.
The reason for the original design was to allow a sender to control
request and response individually to make unit testing easier. The unit
test has now been refactored to use a channel to ensure that requests
are serviced on graceful shutdown.
Signed-off-by: Stephen J Day <stephen.day@docker.com>
This change increases the maximum message size to 4MB to be inline
with the grpc default. The buffer management approach has been changed
to use a pool to minimize allocations and keep memory usage low.
Signed-off-by: Stephen J Day <stephen.day@docker.com>
This apples logic to correctly Close a server, as well as implements
graceful shutdown. This ensures that inflight requests are not
interrupted and works similar to the functionality in `net/http`.
This required a fair bit of refactoring around how the connection is
managed. The connection now has an explicit wrapper object, ensuring
that shutdown happens in a coordinated fashion, whether or not a
forceful close or graceful shutdown is called.
In addition to the above, hardening around the accept loop has been
added. We now correctly exit on non-temporary errors and debounce the
accept call when encountering repeated errors. This should address some
issues where `SIGTERM` was not honored when dropping into the accept
spin.
Signed-off-by: Stephen J Day <stephen.day@docker.com>
Following the convention of http2, we now use odd stream ids for client
initiated streams. This makes it easier to tell who initiates the
stream. We enforce the convention on the server-side.
This allows us to upgrade the protocol in the future to have server
initiated streams.
Signed-off-by: Stephen J Day <stephen.day@docker.com>
With this changeset, ttrpc can now handle mutliple outstanding requests
and responses on the same connection without blocking. On the
server-side, we dispatch a goroutine per outstanding reequest. On the
client side, a management goroutine dispatches responses to blocked
waiters.
The protocol has been changed to support this behavior by including a
"stream id" that can used to identify which request a response belongs
to on the client-side of the connection. With these changes, we should
also be able to support streams in the future.
Signed-off-by: Stephen J Day <stephen.day@docker.com>
Rather than employ the typeurl package, we now generate code to
correctly allocate the incoming types from the caller. As a side-effect
of this activity, the services definitions have been split out into a
separate type that handles the full resolution and dispatch of the
method, incuding correctly mapping the RPC status.
This work is a pre-cursor to larger protocol change that will allow us
to handle multiple, concurrent requests.
Signed-off-by: Stephen J Day <stephen.day@docker.com>
