If TransportSet fails to connect a transport (i.e., transportShutdown()
called without transportReady()), TransportSet will automatically
schedule reconnection for the next address, unless it has reached the end
of the address list, in which case it will fail the delayed transport.
This will reduce stream errors caused by bad addresses appearing before
good addresses in the resolved address list.
Before this change, TransportSet would return the real transport on the
first call of obtainActiveTransport(). After this change, it will return
the delayed transport instead.
Our current lock ordering rules prohibit holding a lock when calling the
channel and stream. This change avoids the lock for both
DelayedClientTransport and DelayedStream. It is effectively a rewrite of
DelayedStream.
The fixes to ClientCallImpl were to ensure sane state in DelayedStream.
Fixes#1510
To ManagedChannelImpl, TransportSet and all client transport
implementations, so they can be correlated in the logs. Also added more
life-cycle logging in general.
Because `scheduleConnection()` is run under lock, if we ran
`createTransportRunnable` inside `scheduleConnection()`,
`savedDelayedTransport.setTransport()` will be under lock which violates
the assumption made in https://github.com/grpc/grpc-java/issues/1408 that
> there is an implicit rule today that channel layer will not hold any lock while calling into transport
and had caused deadlock with `InProcessTransport`.
Also updated tests to
1. Make it clear that we want the first obtained transport to be the
real transport instead of the delayed transport, because
`InProcessTransport` has deadlock issues with delayed
transport (grpc/grpc-java/pull/1510)
2. Not rely on identity equity to check whether `TransportSet` has
switched to a different address. Instead, call `newStream()` and check
which real transport is actually called, which is more reliable.
3. Add timeout when checking for real transport creation and
starting. Tests in general should not check for implementation detail,
e.g., whether certain work is done synchronously.
Netty client shutdown would race with the negotiation handling and
circumvent AbstractBufferingHandler. Use a new command in order to
leave channel.close() available for abrupt killing of the connection
when connecting.
ping_afterTermination was previously racey that made it succeed. After
fixing the test, Netty would consistently fail to call callback. After
fixing Netty to fail the callback it was not using the right status
because when Netty's channel is closed none of our handlers are run.
This reliably fails the future with ClosedChannelException, which is
useless, so now we special-case that exception and fill in the reason
for shutdown.
To prevent accidentally reporting Status.OK, the transports no longer
use OK when calling transportShutdown. The OK status was already no
longer being consumed, since keying off whether transportReady was
called is more helpful.
This fixes#1330
This is lower-level than the existing AbstractTransportTest. It should
be used by all transports, but InProcessTransport is the only one as
part of this commit.
DelayedClientTransport.PendingStream will override cancel(), which has a
clearer semantic.
Also permitting all status codes except OK in ClientStream.cancel(),
instead of just 4 codes.
This PR finishes the refactoring started by #1395. Resolves#1342.
Major changes:
- All interfaces that return `ListenableFuture<T>` now return `T`
- Stop passing `null` for transports after shut down. Pass
`FailingClientTransport` instead. This simplifies the interface
semantics of interfaces that return a transport, as well as their
callers because they no longer need to check for `null`.
- Add two methods to `TransportManager`:
- `createInterimTransport()` takes the place of `BlankFutureProvider`
in `LoadBalancer` implementations.
- `createFailingTransport()` takes the place of errors in the `Future`
when `LoadBalancer` implementations propagate errors to the caller.
- `createInterimTransport()` creates a `DelayedClientTransport` tracked
by `ManagedChannelImpl`, which will not terminate until all
`DelayedClientTransport`s, in addition to the `TransportSet`s, are
terminated.
- Removed the transport argument from the ready and shutdown callbacks,
because if `LoadBalancer` was given a delayed transport earlier, it
will get the real transport's shutdown event, thus won't be able to
match the two through identity comparison, which beats the purpose of
passing the transport.
Although the changes were determined automatically, they were manually
applied to the codebase.
ClientCalls actually has a bug fix, since the suggestion to add
interrupt() made it obvious that interrupted() was inappropriate.
Always return a completed future from `TransportSet`. If a (real) transport has not been created (e.g., in reconnect back-off), a `DelayedClientTransport` will be returned.
Eventually we will get rid of the transport futures everywhere, and have streams always __owned__ by some transports.
DelayedClientTransport
----------------------
After we get rid of the transport future, this is what `ClientCallImpl` and `LoadBalancer` get when a real transport has not been created yet. It buffers new streams and pings until `setTransport()` is called, after which point all buffered and future streams/pings are transferred to the real transport.
If a buffered stream is cancelled, `DelayedClientTransport` will remove it from the buffer list, thus #1342 will be resolved after the larger refactoring is complete.
This PR only makes `TransportSet` use `DelayedClientTransport`. Follow-up changes will be made to allow `LoadBalancer.pickTransport()` to return null, in which case `ManagedChannelImpl` will give `ClientCallImpl` a `DelayedClientTransport`.
Changes to ClientTransport shutdown semantics
---------------------------------------------
Previously when shutdown() is called, `ClientTransport` should not accept newStream(), and when all existing streams have been closed, `ClientTransport` is terminated. Only when a transport is terminated would a transport owner (e.g., `TransportSet`) remove the reference to it.
`DelayedClientTransport` brings about a new case: when `setTransport()` is called, we switch to the real transport and no longer need the delayed transport. This is achieved by calling `shutdown()` on the delayed transport and letting it terminate. However, as the delayed transport has already been handed out to users, we would like `newStream()` to keep working for them, even though the delayed transport is already shut down and terminated.
In order to make it easy to manage the life-cycle of `DelayedClientTransport`, we redefine the shutdown semantics of transport:
- A transport can own a stream. Typically the transport owns the streams
it creates, but there may be exceptions. `DelayedClientTransport` DOES
NOT OWN the streams it returns from `newStream()` after `setTransport()`
has been called. Instead, the ownership would be transferred to the
real transport.
- After `shutdown()` has been called, the transport stops owning new
streams, and `newStream()` may still succeed. With this idea,
`DelayedClientTransport`, even when terminated, will continue
passing `newStream()` to the real transport.
- When a transport is in shutdown state, and it doesn't own any stream,
it then can enter terminated state.
ManagedClientTransport / ClientTransport
----------------------------------------
Remove life-cycle interfaces from `ClientTransport`, and put them in its subclass - `ManagedClientTransport`, with the same idea that we have `Channel` and `ManagedChannel`. Only the one who creates the transport will get `ManagedClientTransport` thus is able to start and shutdown the transport. The users of transport, e.g., `LoadBalancer`, can only get `ClientTransport` thus are not alter its state. This change clarifies the responsibility of transport life-cycle management.
Fix TransportSet shutdown semantics
-----------------------------------
Currently, if `TransportSet.shutdown()` has been called, it no longer create new transports, which is wrong.
The correct semantics of `TransportSet.shutdown()` should be:
- Shutdown all transports, thus stop new streams being created on them
- Stop `obtainActiveTransport()` from returning transports
- Streams that already created, including those buffered in delayed transport, should continue. That means if delayed transport has buffered streams, we should let the existing reconnect task continue.
Before the service would be leaked, because when the scheduled future
was cancelled the scheduler wouldn't be released. Also, Future isn't
powerful enough to signal when we should release when cancelling.
Given the nature of Context, it also seems beneficial to not have it own
threads. Since any caller of withDeadline*() is required to cancel the
Context, it shouldn't be too burdensome for them to manage the lifecycle
of the scheduler.
The Set from Collections.synchronizedSet() is not protected against
concurrent modification during iteration. We copy an ArrayList out of it
for iteration.
When triggered, it caused the ClientCall.Listener never to complete.
Fixes#1343
The new test doesn't actually fail on my machine with the old code, but
we would hope it would be flaky. Since a race is involved, I don't
expect a more reliable test.
This provides more structured data into the application for it to do
special handling.
In general, we would hope most people don't need this functionality, but
it is a good escape hatch to allow users to workaround infrastructure
problems.
This reverts commit eca1f7c1d6.
We want to preserve the status message identical to what the server
sent. We'll need a better way to communicate debugging details.
- Set knownAcceptEncodingRegistry in SingleTransportChannel
- Change the package name of load_balancer.proto to be consistent with
what is used internally.
Use LinkedHashSet in BlankFutureProvider so that it fulfills the futures
in the same order as they were created. This makes the behavior more
predictable, thus fixes the flakiness of GrpclbLoadBalancerTest and
simplifies BlankFutureProviderTest.
This LoadBalancer does round-robin on a address list received from a
separate "load-balancer service", via the protocol defined in
load_balancer.proto. Everything is put under a subproject `grpc-grpclb`,
because it has dependency to protobuf.
updateRetainedTransports() now accepts EquivalentAddressGroups. The
LoadBalancer merges the LB and normal server address groups when calling
it.
`TransportSet` won't connect/reconnect until a transport is requested
through `obtainActiveTransport()`.
Lazy connection is safer, and more desirable in mobile environments.
It's also what C core is doing.
To warm up connections, `LoadBalancer` can call
`TransportManager.getTransport()`, even periodically if it wants to
maintain live connections.
Locking is necessary to avoid race with setStream() since the
application may be calling cancel() in another thread, and we must not
call listener.close() multiple times.
"onHeaders always is called" is from the initial gRPC design where
0-many context frames were sent before the first message. That is why it
was possible for "no headers were received". That has long-since not
been true, but in the various refactorings this language was
accidentally left. The language "Headers always precede messages" is
correct since headers are only guaranteed if messages were sent.
TransportSet (as well as TransportManager) accepts a group of equivalent
addresses (EquivalentAddressGroup) instead of a single address.
TransportSet will move down the address list when reconnecting, and
applies back-off only after the entire list has been tried.
Main benefits:
- It will stop channel from trying to reconnect addresses that have been
failed to connect to and moved away from. (#1212)
- It will make future implementation of Happy Eyeballs possible, inside
TransportSet.
Tested: covered by TransportSetTest and
ManagedChannelImplTransportManagerTest.
See the javadocs of ManagedChannelBuilder.forTarget().
The most interesting case is passing an IPv6 address as target. It can
be either be passed as an authority, where brackets should not be
escaped ([::1]), or as a path of a full URI, where brackets must be
escaped (dns:///%5B::1%5D).
Previously, dns:///[::1], being an invalid URI (brackets not allowed in
path), would be converted to dns:////dns:///%5B::1%5D and passed to
DnsNameResolver. Though it would fail eventually, the error would be
very confusing to users. I changed the logic so that it would try with
dns:/// only if the target string doesn't look like an intended URI
target.
I have restricted the "URI target" to be absolute and hierarchical,
i.e., must start with scheme://. I couldn't find a way to better tell if
a string is intended to be a URI, but I am open to other options.
Refactored tests:
- Move the tests for getNameResolver() into a separate file
ManagedChannelImplGetNameResolverTest, because those tests are not
quite compatible with the facility provided by ManagedChannelImplTest.
- Create DnsNameResolverTest. Move DnsNameResolver out of the factory
class to accommodate for the test.
When using a direct executor we don't need to wrap calls in a
serializing executor and can thus also avoid the overhead that
comes with it.
Benchmarks show that throughput can be improved substantially.
On my MBP I get a 24% improvement in throughput with also
significantly better latency throughout all percentiles.
(running qps_client and qps_server with --address=localhost:1234 --directexecutor)
=== BEFORE ===
Channels: 4
Outstanding RPCs per Channel: 10
Server Payload Size: 0
Client Payload Size: 0
50%ile Latency (in micros): 452
90%ile Latency (in micros): 600
95%ile Latency (in micros): 726
99%ile Latency (in micros): 1314
99.9%ile Latency (in micros): 5663
Maximum Latency (in micros): 136447
QPS: 78498
=== AFTER ===
Channels: 4
Outstanding RPCs per Channel: 10
Server Payload Size: 0
Client Payload Size: 0
50%ile Latency (in micros): 399
90%ile Latency (in micros): 429
95%ile Latency (in micros): 453
99%ile Latency (in micros): 650
99.9%ile Latency (in micros): 1265
Maximum Latency (in micros): 33855
QPS: 97552
If a LoadBalancer is requested for a transport future before it can get
one from TransportManager, e.g., before name resolution is completed,
LoadBalancer will return a blank future created by BlankFutureProvider
and to be linked with real futures later.
This allows for binding state to be reset to known-good states precisely which in turn
facilitates making it safe to have 'detach' not throw exceptions and instead log
a severe error that attach/detach calls were not correctly balanced.
The error occurs when name resolution completes after the channel is
shut down. What ManagedChannelImpl doing right now is violating the
TransportManager interface, because TransportManager.getTransport()
should never return null.
- Channel builders decide the default port based on whether TLS is used.
- Channel builders populate the default port via an Attributes object
passed to NameResolver.Factory#newNameResolver
Although it is unlikely the awaits return false, it would be useful
information to know about the failure if they did.
This should provide more clues in case the test flakes again (#1146)
- NameResolverRegistry contains all the official NameResolvers. Users
can also add custom NameResolvers to it. It looks up NameResolver by
try-and-fail. It is the default NameResolver.Factory for builders.
DnsNameResolver.
- Pass target as Strings instead of URIs from the channel builder to
ManagedChannelImpl. A target string is not necessarily a valid URI, in
which case ManagedChannelImpl will add "dns:///" to the beginning of
the target and use it as URI.
- DnsNameResolver will require scheme "dns" to be present. It no longer
allows scheme-absent URIs.
Otherwise, when DelayedStream is created it ends up calling
clientTransportProvider a second time. However, we already have a
transportFuture available, we should just use it instead.
transportFailsOnStart was still flaky. By looking at history, it became
obvious that transportFailsOnStart was created to test two cases we no
longer support: the transport throwing an exception during start and the
transport calling listener.transportShutdown during start. The part of
the test checking throwing an exception was removed earlier.
The scheduling on another thread led to a race where sometimes the
future wasn't completed by the time isDone() was checked in
ClientCallImpl causing the usage of DelayedStream, which really messed
up what the test was trying to do.
- Add NameResolver and LoadBalancer interfaces.
- ManagedChannelImpl now uses NameResolver and LoadBalancer for
transport selection, which may return transports to multiple
addresses.
- Transports are still owned by ManagedChannelImpl, which implements
TransportManager interface that is provided to LoadBalancer, so that
LoadBalancer doesn't worry about Transport lifecycles.
- Channel builders can be created by forTarget() that accepts the fully
qualified target name, which is an URI. (TODO) it's not tested.
- The old address-based construction pattern is supported by using
AbstractManagedChannelImplBuilder.DirectAddressNameResolver.
- (TODO) DnsNameResolver and SimpleLoadBalancer are currently
incomplete. They merely work for the single-address scenario.
- ListenableFutures of transports, instead of actual transports, are
passed through multiple layers to ClientCallImpl, so that name
resolution and load-balancing, which may delay the creation of
transports, won't block the creation of ClientCall. This also
simplifies reconnect logic.
- Moved Transport management for a single address to a separate class
TransportSet. Later, ManagedChannelImpl will own multiple
TransportSets instead of one.
- ClientCallImpl will buffer requests in DelayedStream until transport
is ready.
This test occasionally flakes due to the way the deadline timer cancels the stream. Stream cancellation immediately closes the outbound status, disallowing the sending of further messages. The true cancellation is done sometime later in the transport thread in Netty. So there is a race between closing the outbound status and performing the actual cancellation where sent messages will fail with the wrong status.
ServerCall already had "headers must be sent before any messages, which
must be sent before closing," but the implementation did not enforce it
and our async server handler didn't obey.
The benefit of forcing sending headers first is that it removes the only
implicit call in our API and interceptors dealing just with metadata
don't need to override sendMessage. The implicit behavior was bug-prone
since it wasn't obvious you were forgetting that headers may not be
sent.
It is typically used for receiving, but is on the sending object. It
would be a pain for users to coordinate, and the implementation is
already thread-safe because we always thought it would be thread-safe.
So make it part of the documentation.
We want to allow overriding authority in the ManagedChannelBuilder for
testing. In doing that, we basically require that all Channels support
authority. In reality, this simplifies things and is already being done
by the C implementation, as their unix domain socket support uses
"localhost" just like our in-process transport now does.
We can debate some whether "localhost" is really the most appropriate
authority for the in-process transport, but that should probably happen
later since "localhost" is "good enough" for now.
Although the functionality is currently available by passing a
manually-created InetAddress, that requires that the user do I/O before
calling our API and does not work with naming in the future.
There is no need to use ServerMethodDefinition in codegen. The create()
method itself could be helpful to a dynamic HandlerRegistry
implementation, so we won't remove it.
This provides an API for applications to use gRPC without using
ExperimentalApis. It also allows swapping out a transport implementation
in the future.
buchgr
Xiao Hang
Lukasz Strzalkowski
Carl Mastrangelo
elandau
Kun Zhang
Eric Anderson
William Thurston
Louis Ryan
Solomon Duskis
nmittler
Xudong Ma
Éamonn McManus