This simplifies the construction paradigm and leads to the eventual
removal of TransportCreationParamsFilterFactory. The eventual end goal
is to be able to shut down ProtocolNegotiators as is necessary for ALTS.
The only reason the initialization was delayed was for 'authority', so
we now plumb the authority through GrpcHttp2ConnectionHandler.
It appears to be stable now. Ran for 1000s of times. I do see some
general flakiness in TransportTest, but it applies to the tests in
general and isn't specific to this one test. It is:
```
org.mockito.exceptions.verification.WantedButNotInvoked:
Wanted but not invoked:
listener.transportReady();
-> at io.grpc.internal.testing.AbstractTransportTest.startTransport(AbstractTransportTest.java:1815)
Actually, there were zero interactions with this mock.
```
This flake is not seen often because it occurs less frequently when
running all the tests (~.1% vs 1%). One of the early tests must warm
something up to make it less likely.
This is an API used to coordinate across packages and must live in
`io.grpc`.
Prepending `Internal` makes it easier to detect and hide this class
from public visibility when using certain build tools.
fixes#4796
There's no good way to provide users of ALTS a choice between grpc-netty
and grpc-netty-shaded. Since Netty is not exposed through the ALTS API
surface, we opt for the shaded version as it has fewer deployment
issues. However, this also means that we _can't_ expose any Netty API,
like EventLoopGroup.
Passing a promise to WriteQueue was only misused to add a listener on
the promise before issuing the write. Although in this case the listener
ordering will be "random" because listeners are being added from two
different threads, in general we always want to add a listener after the
write returns to let any lower-level listeners be registered first.
Future work can resolve the "random" listener order by passing the
listener to the WriteQueue and adding the listener from the event loop.
Most of the changes are changing the signature of newClientTransport.
Since this is annoying, I choose to introduce a ClientTransportOptions
object to avoid the churn in the future.
With ClientTransportOptions in place, there's only a few lines necessary
of plumbing for the Attributes: add the field to ClientTransportOptions
and populate it in InternalSubchannel. There are no consumers of the
field in this commit.
A new RPC starts with the following steps:
1. Pick a READY transport
2. the READY transport calls `transport.newStream()`
3. the new stream calls `stream.start()`
4. `stream.start()` invokes or enqueus `writeHeaders()` (or for GET request, noop)
A racy GOAWAY could happen between 3 and 4, and by the retry spec, the RPC should be transparent-retry-able in this case. For Netty and OkHttp transport implementation, before step 4, (even if step 1, 2, and 3 excluding 4 are made atomic,) the http2-stream for the RPC is not created, so the current transparent retry logic does not apply and need fix.
Of course, if step 1, 2, and 3 including 4 are made atomic, and not with GET, there will be no such problem.
This PR adds an automatic gradle format checker and reformats all the *.gradle files. After this, new changes to *.gradle files will fail to build if not in good format, just like checkStyle failure.
This fixes the warning:
`Tag @link: reference not found: Channelz.Security`
Javadoc `@link` is simplistic in its processing of '.' and thinks if a
dot exists it means it is part of the package name. You're forced to use
the full name of nested classes.
The peer socket is read from TRANSPORT_ATTR_REMOTE_ADDR from the
stream attributes. We only log the peer on receive initial metadata.
The call id assumes census is available. The call ID read from the
context via SERVER_CALL_ID_CONTEXT_KEY on server side, and read from
CallOptions via CLIENT_CALL_ID_CALLOPTION_KEY on client side. The
value is copied from CONTEXT_SPAN_KEY which is set by census.
Pass around CallId with two longs, not a byte[].
Server listen sockets differ from normal sockets in that they do not
have a remote address, do not have stats on calls started/failed/etc,
and do not have security info.
Always set the remote address, no reason why this should be a TLS-only
feature. This is needed for channelz, and is especially useful in unit
tests where we are using plaintext.
This PR adds the attr for plaintext.
Changes:
- `ClientStreamListener.onClose(Status status, RpcProgress rpcProgress, Metadata trailers)` added.
- `AbstractClientStream.transportReportStatus(Status status, RpcProgress rpcProgress, boolean stopDelivery, Metadata trailers)` added
- `ClientCallImpl.ClientStreamListenerImpl` will ignore the arg `rpcProgress` (non retry)
- `RetriableStream.SubListener` will handle `rpcProgress` and decide if transparent retry.
- `NettyClientHandler` and `OkHttpClientTransport` will pass `RpcProgress.REFUSED` to client stream listener for later stream ids when received GOAWAY, or for stream received a RST_STREAM frame with REFUSED code.
- All other files are just a result of refactoring.
Transport ststistics should really be a child member of SocketStats.
While we're at it, let's add the local and remote SocketAddress to
SocketStats, with a test.
This partially reverts commit 48ca4527c1.
It leaves the changes to ServerCallImpl and test.
This also partially reverts "Lint fixes" commit
3002a23a0f which removed unused variables
which are now necessary again.
This is reverted for the combined result of two issues:
* Some users are testing that they get UNKNOWN when the service throws.
That's not unreasonable given the behavior was well-publicised when it
changed in v1.5. We should probably keep the UNKNOWN in some common
cases (like the service threw immediately, before sending anything).
* The client could see CANCELLED instead of INTERNAL as had been
intended. It's unclear as to why (I didn't investigate heavily). This
behavior is visible in MoreInProcessTest and was overlooked during
review.
Some users have reported "Channel closed but for unknown reason".
Adding this information doesn't tell us where the bug is, but may help
us narrow down why getShutdownStatus() is null.
This fixes the gradle warning:
The SimpleWorkResult type has been deprecated and is scheduled to be
removed in Gradle 5.0. Please use WorkResults.didWork() instead.
This adds a method on GrpcHttp2ConnectionHandler which, when called, indicates that the channel associated with the handler is no longer needed.
Notes:
* The handler may not be on the channel, but will either need to be added or will never be added.
* The channel will only be "unused" on the server side.
* It is expected that after calling `notifyUnused()`, the channel will be deregistered from the loop without being properly shut down. This allows the channel to be handed off to a Non-netty API.
Spies are really magical and easily produce unexpected results. Using them in
tests can easily yield tests that don't do what you think they do. Delegation
is much safer when possible.
Delegation doesn't work when methods `return true`, final methods, and with
restricted visibility, though. So CensusModulesTest and
MaxConnectionIdleManagerTest are left as-is.
The channelz service must not live in io.grpc.internal, and channelz
needs to be able to get the identifier of the entities it
tracks. Since io.grpc can not refer to io.grpc.internal, the LogId
must be moved out of internal.
Since Netty may have set some parameters already, we should modify the
existing SSLParameters instead of starting from scratch.
This may fix ALPN with JDK9, but full support for ALPN with JDK9 is
still later work and we're not supporting it yet.
Fixes#3532
The method name passed to MethodDescriptor does not include the leading
'/'. If it does, on the wire it will actually cause two slashes. This
has been this way for a _long_ time, but in tests that ignore the method
name or use the same MethodDescriptor no client and server the extra /
"works fine." But it's misleading, so let's remove it.
Only bump the counter from AbstractServerStream.TransportState, and hole punch
from AbstractServerStream to TransportState when the application calls close.
This diff does not actually change any behaviors yet, that will come
in the next diff along with unit tests for those new behaviors. This
diff's goal is only to change the method signatures so future diffs
are cleaner.
Counters are bumped when a message is completely written. If a
part of a message is still buffered and not yet flushed, we will
not increment the stats.
Move netty connection log info to a separate logger:
io.grpc.netty.NettyServerTransport.connections
Users can redirect or disable this log using the usual way:
-Djava.util.logging.config.file="logging.properties"
This is needed for both completeness and stats/tracing contexts propagation.
Stats recording with Census is intentionally disabled (#2284), while the rest of the Census-related logic work the same as on the other transports.
* core: add finalizer checks for ManagedChannels
Cleaning up channels is something users should do. To promote this
behavior, add a log message to indicate that the channel has not
been properly cleaned.
This change users WeakReferences to avoid keeping the channel
alive and retaining too much memory. Only the id and the target
are kept. Additionally, the lost references are only checked at
JVM shutdown and on new channel creation. This is done to avoid
Object finalizers.
The test added checks to see that the message is logged. Since
java does not allow forcing of a GC cycle, this code is best
effort, giving up after about a second. A custom log filter is
added to hook the log messages and check to see if the correct
one is present. Handlers are not used because they are
hierarchical, and would be annoying to restore their state after
the test.
The other tests in the file contribute a lot of bad channels. This
is reasonable, because they aren't real channels. However, it does
mean that less than half of them are being cleaned up properly.
After trying to fix a few, it is too hard to do. It would only
serve to massively complicate the tests.
Instead, this code just keeps track of how many it wasn't able to
clean up, and ignores them for the test. They are still logged,
because really they should be closed.
Carl Mastrangelo
Eric Anderson
ZHANG Dapeng
zpencer
Kun Zhang
Eric Gribkoff
Rama Chavali
Shohei Kamimori
Teo Jia Jiun
Xiaoshuang LU
Ray Tsang