* Add option in OkHttpServerBuilder
* Add value as MAX_CONCURRENT_STREAM setting in settings frame sent by the server to the client per connection
* Enforce limit by sending a RST frame with REFUSED_STREAM error
The recommended way to load dependencies from `rules_jvm_external`
is to make use of the `@maven` workspace, and the most readable
way of doing that is to use the `artifact` macro provides.
This removes the need to generate the "compat" namespaces, which
`rules_jvm_external` provided for backwards compatibility with
older releases. This change also sets things up for supporting
`bzlmod`: this requires all workspaces accessed by a library to
be named "up front" in the `MODULE.bazel` file. This way, the
only repo that needs to be exported is `@maven`, rather than the
current huge list.
* Allow the queued byte threshold for a Stream to be ready to be configurable
- on clients this is exposed by setting a CallOption
- on servers this is configured by calling a method on ServerCall or ServerStreamListener
SelfSignedCertificate is not available on Java 17 because
OpenJdkSelfSignedCertGenerator is not available. This only impacted
tests.
AccessController is being removed, and these locations are doing simple
reflection which is unlikely to require it even when a security policy
is in effect. There's other places we do reflection without the
AccessController, so either no security policies care or the users can
update their policies to allow it.
`-link` does I/O to download the package list, for every javadoc
invocation. There is no caching, so this happens many times per build.
Swap to offline mode to avoid spamming the servers, and avoid build
failures if the servers aren't entirely healthy.
Http2OkHttp is now unnecessary, as Http2Test tests OkHttp client to
Netty server. receivedDataForFinishedStream() was the only remaining
unique test and it seems already covered by AbstractInteropTest these
days.
All the changes outside libs.versions.toml and examples were
because of ErrorProne. It didn't actually find anything to fix; signal
vs noise has gotten pretty bad with the newer checks.
Status was changed for ErrorProne's SuperCallToObjectMethod. With the
old code it didn't notice the trivial implementation. The fail-for-test
code wasn't used, so it was easiest to just remove it.
Some of the libs had their versions inlined; now that we have
:checkForUpdates it isn't much of a risk for versions to diverge when
there's only a few artifacts sharing a version. If we need 4+ artifacts
to have the same version, then it makes sense to still use a shared
version.
Dependencies not upgraded: google-auth-libray, mockito, netty, cronet
* core, netty, okhttp: implement new logic for nameResolverFactory API in channelBuilder
fix ManagedChannelImpl to use NameResolverRegistry instead of NameResolverFactory
fix the ManagedChannelImplBuilder and remove nameResolverFactory
* Integrate target parsing and NameResolverProvider searching
Actually creating the name resolver is now delayed to the end of
ManagedChannelImpl.getNameResolver; we don't want to call into the name
resolver to determine if we should use the name resolver.
Added getDefaultScheme() to NameResolverRegistry to avoid needing
NameResolver.Factory.
---------
Co-authored-by: Eric Anderson <ejona@google.com>
This breaks the ABI of the classes listed below.
Users that recompiled their code using grpc-java [`v1.36.0`]
(https://github.com/grpc/grpc-java/releases/tag/v1.36.0) (released on
Feb 23, 2021) and later, ARE NOT AFFECTED.
Users that compiled their source using grpc-java earlier than
[`v1.36.0`]
(https://github.com/grpc/grpc-java/releases/tag/v1.36.0) need to
recompile when upgrading to grpc-java `v1.59.0`. Otherwise the code
will fail on runtime with `NoSuchMethodError`. For example, code:
```java
NettyChannelBuilder.forTarget("localhost:100").maxRetryAttempts(2);
```
Will fail with
> `java.lang.NoSuchMethodError: 'io.grpc.internal.AbstractManagedChannelImplBuilder
io.grpc.netty.NettyChannelBuilder.maxRetryAttempts(int)'`
**Affected classes**
Class `AbstractManagedChannelImplBuilder` is deleted, and no longer in
the class hierarchy of the channel builders:
- `io.grpc.netty.NettyChannelBuilder`
- `io.grpc.okhttp.OkhttpChannelBuilder`
- `grpc.cronet.CronetChannelBuilder`
This prevents grpc-util from being exposed on the classpath when
compiling code using grpc-okhttp. grpc-core is still needed because of
AbstractManagedChannelImplBuilder.
This allows okhttp to service the Grpc.newServerBuilderForPort() API.
Note that, unlike Netty, it will throw if you try to use
ServerBuilder.forPort().
This fixes android-interop-testing which was broken by c9864a119.
The PipeSocket was convenient and avoided real I/O, but the
shutdown/close while connecting/handshaking tests were triggering a
Socket bug in Java (https://bugs.openjdk.org/browse/JDK-8278326). Using
a real socket doesn't trigger the bug because the test stops sharing
state with the code under test.
Fixes#10228
```
Details
==================
WARNING: ThreadSanitizer: data race (pid=4528)
Write of size 1 at 0x0000cfb9d5f4 by thread T36 (mutexes: write M0):
#0 java.net.Socket.setCreated()V Socket.java:687
#1 java.net.AbstractPlainSocketImpl.create(Z)V AbstractPlainSocketImpl.java:149
#2 java.net.Socket.createImpl(Z)V Socket.java:477
#3 java.net.Socket.getImpl()Ljava/net/SocketImpl; Socket.java:540
#4 java.net.Socket.setTcpNoDelay(Z)V Socket.java:998
#5 io.grpc.okhttp.OkHttpServerTransport.startIo(Lio/grpc/internal/SerializingExecutor;)V OkHttpServerTransport.java:164
#6 io.grpc.okhttp.OkHttpServerTransport.lambda$start$0(Lio/grpc/internal/SerializingExecutor;)V OkHttpServerTransport.java:159
#7 io.grpc.okhttp.OkHttpServerTransport$$Lambda$56.run()V ??
#8 io.grpc.internal.SerializingExecutor.run()V SerializingExecutor.java:133
#9 java.util.concurrent.ThreadPoolExecutor.runWorker(Ljava/util/concurrent/ThreadPoolExecutor$Worker;)V ThreadPoolExecutor.java:1130
#10 java.util.concurrent.ThreadPoolExecutor$Worker.run()V ThreadPoolExecutor.java:630
#11 java.lang.Thread.run()V Thread.java:830
#12 (Generated Stub) <null>
Previous read of size 1 at 0x0000cfb9d5f4 by thread T35 (mutexes: write M1, write M2):
#0 java.net.Socket.close()V Socket.java:1512
#1 io.grpc.okhttp.OkHttpServerTransportTest$PipeSocket.close()V OkHttpServerTransportTest.java:1384
#2 io.grpc.okhttp.OkHttpServerTransportTest.clientCloseDuringHandshake()V OkHttpServerTransportTest.java:290
```
This avoids the (often missing) evaluationDependsOn and fixes using
results from other projects without propagating those through
Configuration. It also reduces the number of useless classes pulled in
by down-stream tests, reducing the probability of rebuilds.
The expectation of fixtures is they help testing down-stream code that
use the classes in main. That applies to all the classes here except for
FakeClock and StaticTestingClassLoader. It would also apply to many
internal classes in grpc-testing, but let's consider cleaning that up
future work.
TlsTesting.loadCert() is a public API and so should be preferred over
our internal utility. It avoids creating a temp file that has to be
deleted by a shutdown hook. Usages that needed a file were not migrated.
Our tests assume localhost is in /etc/hosts or uses some other form of
local-only resolution. But that wouldn't apply to "host". What was
happening is this was causing a DNS resolution, which would fail, and
the InetSocketAddress would be "unresolved". Thus, the equivalent and
faster code would be `InetSocketAddress.createUnresolved("host", 1234)`.
But there doesn't seem to be any reason to avoid localhost in this test,
so swap to the more typical solution instead.
This should avoid flakes like:
```
io.grpc.okhttp.OkHttpClientTransportTest > invalidAuthorityPropagates FAILED
org.junit.runners.model.TestTimedOutException: test timed out after 10 seconds
at java.base@11.0.17/java.net.Inet6AddressImpl.lookupAllHostAddr(Native Method)
at java.base@11.0.17/java.net.InetAddress$PlatformNameService.lookupAllHostAddr(InetAddress.java:929)
at java.base@11.0.17/java.net.InetAddress.getAddressesFromNameService(InetAddress.java:1529)
at java.base@11.0.17/java.net.InetAddress$NameServiceAddresses.get(InetAddress.java:848)
at java.base@11.0.17/java.net.InetAddress.getAllByName0(InetAddress.java:1519)
at java.base@11.0.17/java.net.InetAddress.getAllByName(InetAddress.java:1378)
at java.base@11.0.17/java.net.InetAddress.getAllByName(InetAddress.java:1306)
at java.base@11.0.17/java.net.InetAddress.getByName(InetAddress.java:1256)
at java.base@11.0.17/java.net.InetSocketAddress.<init>(InetSocketAddress.java:220)
at app//io.grpc.okhttp.OkHttpClientTransportTest.invalidAuthorityPropagates(OkHttpClientTransportTest.java:1687)
```
Previously, untrustedServer_fails could have been failing for the same
reason as unmatchedServerSubjectAlternativeNames_fails. The
implementation could have been broken and not checking the cert chain
but still checking the hostname. We'd either need to override the
authority to match the badserver cert or use the normal server
certificates. It is best to use the normal server certificates as
mtls_succeeds confirms the configuration is correct and so our test is
failing for the right reason.
Trying to upgrade Gradle to 7.6 improved the checkstyle plugin such that
it appears to have been running in new occasions. That in turn exposed
us to https://github.com/checkstyle/checkstyle/issues/5088. That bug was
fixed in 8.28, which also fixed lots of other bugs. So now we have
better checking and some existing volations needed fixing. Since the
code style fixes generated a lot of noise, this is a pre-fix to reduce
the size of a Gradle upgrade.
I did not upgrade past 8.28 because at some point some other bugs were
introduced, in particular with the Indentation module. I chose the
oldest version that had the particular bug impacting me fixed. Upgrading
to this old-but-newer version still makes it easier to upgrade to a
newer version in the future.
When allocating bytes to streams within a flow control window we always
go through the streams in the same order. This can lead to large streams
hogging all the bytes and a smaller one down the list getting starved
out. This change shuffles the stream array to lower the chance of this
happening.
Fixes#9089
%s is fairly safe (requires a Formattable to use Locale), so %d is the
main risk item. Places that really didn't need to use String.format()
were converted to plain string concatenation. Logging locations were
generally converted to using the log infrastructure's delayed
formatting, which is generally locale-sensitive but we're okay with
that. That wasn't done in okhttp, however, because Android frequently
doesn't use MessageFormat so we'd lose the parameters. Everywhere else
was explicitly defined to be Locale.US, to be consistent independent of
the default system locale.
This reverts commit c1abc7f8ac. It
produced compilation issues inside Google. I strongly suspect it isn't
this commit or gRPC's fault, but it prevents further testing until it is
resolved.
Android linters can't recognize the difference when VisibleForTesting
is used because the method has different visibility, or because
the method only intended for testing.
Because of that linter complains when VisibleForTesting methods are
used in the production code.
Ideally we want to replace or remove this annotation, as its
usage for marking altered visibility for testing purposes is
discouraged since guava v30.0.
This can avoid creating an additional 736 tasks (previously 502 out of
1591 were not created). That's not all that important as the build time
is essentially the same, but this lets us see the poor behavior of the
protobuf plugin in our own project and increase our understanding of how
to avoid task creation when developing the plugin. Of the tasks still
being created, protobuf is the highest contributor with 165 tasks,
followed by maven-publish with 76 and appengine with 53. The remaining
59 are from our own build, but indirectly caused by maven-publish.
This fixes a regression introduced in e96d0477. The NullPointerException
only happens on client-side when some other error occurred during
handshaking.
I tried to add a test, but SerializingExecutor catches+logs the
exception and the expected behavior in the circumstance is that close()
is a noop. So the NPE was entirely benign other than annoying log
messages.
François JACQUES