Open
Conversation
…ected` (#11220) Motivation: `Http2FrameCodecBuilder` defines static factory methods `forClient()` and `forServer()` that should be used to create a new instance. The default ctor is useful only when users need to override behavior of the existing builder. Those users should define another way to create an instance. Modifications: - Decrease visibility of `Http2FrameCodecBuilder` default ctor from `public` to `protected`; - Add javadoc to clarity responsibilities; Result: Users of `Http2FrameCodecBuilder` are not confused why `new Http2FrameCodecBuilder().build()` works for the server-side, but does not work for the client-side.
Motivation: DNS resolver falls back to trying CNAME if no records found, but should only try this for A/AAAA queries. Does not make sense for other query types, results in a redundant CNAME query that is just going to fail. Modification: Check query type before deciding to try CNAME. Only proceed if type is A or AAAA. Added unit test to verify CNAME is only tried after A/AAAA queries. Result: Fixes #11214.
Motivation: We've seen (very rare) flaky test failures due to timeouts. They are too rare to analyse properly, but a theory is that on overloaded, small cloud CI instances, it can sometimes take a surprising amount of time to start a thread. It could be that the event loop thread is getting an unlucky schedule, and takes seconds to start, causing the timeouts to elapse. Modification: Increase the initial timeouts in the SSLEngineTest, that could end up waiting for the event loop thread to start. Also fix a few simple warnings from Intellij. Result: Hopefully we will not see these tests be flaky again.
Motivation: We use an old version of maven atm. Modifications: Update to maven 3.8.1 Result: Use latest maven release when compiling
Motivation: Fix switch case fall through, add default block in MqttVersion Modification: Fix switch case fall through, add default block in MqttVersion Result: Code cleanup Signed-off-by: xingrufei <xingrufei@sogou-inc.com> Co-authored-by: xingrufei <xingrufei@sogou-inc.com>
Motivation: When checking the latest commit i saw some bad exception messages in MqttVersion hence i improved them. Modification: Improved exception messages in MqttVersion. Result: Better exception messages in MqttVersion.
Motivation: We should add explicit null checks so its easier for people to understand why it throws. Modification: Add explicit checkNotNull(...) Result: Easier to understand for users why it fails. Signed-off-by: xingrufei <xingrufei@sogou-inc.com> Co-authored-by: xingrufei <xingrufei@sogou-inc.com>
Motivation: It seems like it is a known issue that maven frequently sees connection reset / connection timeout during CI builds. We should workaround these issues like others did: - kiegroup/kie-wb-common#3416 Modifications: Add extra maven options during build to reduce the likelyness of timeouts / resets Result: More stable builds
Motivation: 0f25213 introduced some properties that were used to make builds more stable on the ci. All of these properties were duplicated everywhere, this made it hard to maintain Modifications: - Add profile which sets the properties. - Just use the profile when build on the ci Result: Easier to maintain custom properties for the ci build
Motivation: Netty lacks client side support for decompressing Brotli compressed response bodies. Modification: * Introduce optional dependency to brotli4j by @hyperxpro. It will be up to the user to provide the brotli4j libraries for the target platform in the classpath. brotli4j is currently available for Linux, OSX and Windows, all for x86 only. * Introduce BrotliDecoder in codec module * Plug it onto `HttpContentDecompressor` for HTTP/1 and `DelegatingDecompressorFrameListener` for HTTP/2 * Add test in `HttpContentDecoderTest` * Add `BrotliDecoderTest` that doesn't extend `AbstractDecoderTest` that looks flaky Result: Netty now support decompressing Brotli compressed response bodies.
Motivation: Mac OS specific DNS resolver fails to take into account search order of resolvers causing wrong resolver being used is some circumstances Modifications: Re-order array of resolvers using their sort order as an ordering key. Final order is opposite of the search order to make sure that resolver with the lower sort order goes last (so it overrides previous one in the `resolverMap`). Result: Fixes issue #11225
…rop (#11239) Motivation: `PlatformDependent#normalizedOs()` already caches normalized variant of the value of `os.name` system property. Instead of inconsistently normalizing it in every case, use the utility method. Modifications: - `PlatformDependent`: `isWindows0()` and `isOsx0()` use `NORMALIZED_OS`; - `PlatformDependent#normalizeOs(String)` define `darwin` as `osx`; - `OpenSsl#loadTcNative()` does not require `equalsIgnoreCase` bcz `os` is already normalized; - Epoll and KQueue: `Native#loadNativeLibrary()` use `normalizedOs()`; - Use consistent `Locale.US` for lower case conversion of `os.name`; - `MacOSDnsServerAddressStreamProvider#loadNativeLibrary()` uses `PlatformDependent.isOsx()`; Result: Consistent approach for `os.name` parsing.
Motivation: Conscrypt not correctly filters out non support TLS versions which may lead to test failures. Related to google/conscrypt#1013 Modifications: - Bump up to latest patch release - Add workaround Result: No more test failures caused by conscrypt
Motivation: TLSv1 and TLSv1.1 is considered insecure. Let's follow the JDK and disable these by default Modifications: - Disable TLSv1 and TLSv1.1 by default when using OpenSSL. - Add unit tests Result: Use only strong TLS versions by default when using OpenSSL
Motivation: We should use the same maven cache for all builds so we can re-use as much of the downloaded maven dependencies as possible Modifications: - Just use the same cache for all Result: Hopefully be able to re-use most of the dependencies
#11248) Motivation: We should setup the caching so it will be able to use different restore keys and so almost never need to start from scratch Modifications: Adjust caching config to make use of different restore keys for maven caching but also docker caching Result: Better cache usage
Motivation: When trying to compile with java16 we should use adopt@1.16* Modifications: - Use adopt@1.16.0-1- - Upgrade to blockhoud 1.0.6 to be able to support java16 Result: Use correct java version / flavor
Motivation: We had some println left in the test-classes. Modifications: Remove println usage Result: Cleanup
Motivation: We introduced the ability to offload certain operations to an executor that may take some time to complete. At the moment this is not enabled by default when using the openssl based SSL provider. Let's enable it by default as we have this support for some while now and didnt see any issues yet. This will also make things less confusing and more consistent with the JDK based provider. Modifications: Use true as default value for io.netty.handler.ssl.openssl.useTasks. Result: Offloading works with openssl based SSL provider as well by default
Motivation: Just use MAVEN_OPTS to setup all the timeouts etc for dependency downloads. This way we at least can be sure these are applied. Modifications: - Use MAVEN_OPTS - Remove ci profile - Remove unused settings.xml file - Always use ./mvnw Result: Build stability improvements
Motivation: `SelfSignedCertificate` creates a certificate and private key files and store them in a temporary directory. However, if the certificate uses a wildcard hostname that uses asterisk *, e.g. `*.shieldblaze.com`, it'll throw an error because * is not a valid character in the file system. Modification: Replace the asterisk with 'x' Result: Fixes #11240
…le's entries for a hostname (#11246) Motivation: DefaultHostsFileEntriesResolver should provide all hosts file's entries for a hostname when DnsNameResolver#resolveAll as opposed to the current implementation where only the first entry is taken into consideration Modification: - Add DefaultHostsFileEntriesResolver#addresses to provide all hosts file's entries for a hostname - Add HostsFileEntriesProvider to provide all hosts file's entries for a hostname and to keep backwards compatibility for HostsFileEntries and HostsFileParser - DnsNameResolver#resolveAll uses the new DefaultHostsFileEntriesResolver#addresses - BlockHound configuration: replace HostsFileParser#parse with HostsFileEntriesProvider$ParserImpl#parse as the latter does the parsing - Add junit tests Result: Fixes #10834
Motivation: There is a typo in the javadocs Modification: correct grammar mistakes Result: cleanup
Motivation: ParserImpl is stateless and so we can use the same instance multiple times Modifications: - Make constructor private - Return the same instance all the time Result: Less object creation
Motivation: After the release was done we need to also copy the apidocs and xref to the netty-website Modifications: Add script that does the copy etc Result: Less manual steps to remember
Motivation: The current initialization of Slf4JLoggerFactory is not singleton. Modification: Use Slf4JLoggerFactory.INSTANCE to initialize Slf4JLoggerFactory. Result: The instance of Slf4JLoggerFactory became a singleton.
Motivation: When changing the netty-all artifact to not include any sources we also removed the ability to generate the javadocs / xref files for our website Modifications: - Add new profile which will generate the files - Add script which generates all the files and copy these over to the netty-website Result: Easier to generate files for website
Motivation: Cannot load the native library for DNS resolutions on MacOS. The exception below is observed: 18:02:43.453 [Test worker] ERROR i.n.r.d.DnsServerAddressStreamProviders - Unable to load io.netty.resolver.dns.macos.MacOSDnsServerAddressStreamProvider, fallback to system defaults. This may result in incorrect DNS resolutions on MacOS. java.lang.reflect.InvocationTargetException: null at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method) at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62) at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45) at java.lang.reflect.Constructor.newInstance(Constructor.java:423) at io.netty.resolver.dns.DnsServerAddressStreamProviders.<clinit>(DnsServerAddressStreamProviders.java:64) at io.netty.resolver.dns.DnsNameResolverBuilder.<init>(DnsNameResolverBuilder.java:60) at reactor.netty.transport.NameResolverProvider.newNameResolverGroup(NameResolverProvider.java:432) ... Caused by: java.lang.UnsatisfiedLinkError: io.netty.resolver.dns.macos.MacOSDnsServerAddressStreamProvider.resolvers()[Lio/netty/resolver/dns/macos/DnsResolver; at io.netty.resolver.dns.macos.MacOSDnsServerAddressStreamProvider.resolvers(Native Method) at io.netty.resolver.dns.macos.MacOSDnsServerAddressStreamProvider.retrieveCurrentMappings(MacOSDnsServerAddressStreamProvider.java:127) at io.netty.resolver.dns.macos.MacOSDnsServerAddressStreamProvider.<init>(MacOSDnsServerAddressStreamProvider.java:123) This is a regression made with #11239 Modification: When checking for OS, an exception must be thrown when the OS is not MacOS Result: The native library for DNS resolutions on MacOS can be loaded
Motivation: The `HttpContentCompressor.beginEncode()` method has too many if else, so consider refactoring Modification: Create the corresponding `CompressionEncoderFactory` according to the compression algorithm, remove the if else Result: The code of `HttpContentCompressor` is cleaner than the previous implementation Signed-off-by: xingrufei <xingrufei@sogou-inc.com> Co-authored-by: Norman Maurer <norman_maurer@apple.com>
…1582) Motivation: - Fix `HttpContentCompressor` errors due to missing optional compressor libraries such as Brotli and Zstd at runtime. - Improve support for optional encoders by only considering the `CompressionOptions` provided to the constructor and ignoring those for which the encoder is unavailable. Modification: The `HttpContentCompressor` constructor now only creates encoder factories for the CompressionOptions passed to the constructor when the encoder is available which must be checked for Brotli and Zstd. In case of Brotli, it is not possible to create BrotliOptions if brotly4j is not available so there's actually nothing to check. In case of Zstd, I had to create class `io.netty.handler.codec.compression.Zstd` similar to `io.netty.handler.codec.compression.Brotli` which is used to check that zstd-jni is availabie at runtime. The `determineEncoding()` method had to change as well in order to ignore encodings for which there's no `CompressionEncoderFactory` instance. When the HttpContentCompressor is created using deprecated constructor (ie. with no CompressionOptions), we consider all available encoders. Result: Fixes #11581.
Motivation: At present, the verification methods of `ZstdOptions` and `BrotliOptions` are not consistent, and the processing methods of `ZstdOptions` and `BrotliOptions` in `HttpContentCompressor` are also inconsistent. The http2 module does not add zstd-jni dependency, so `ClassNotFoundException` may be thrown Modification: Added `Zstd.isAvailable()` check in `ZstdOptions` to be consistent, and added zstd-jni dependency in http2 module Result: The verification methods of `ZstdOptions` and `BrotliOptions` are consistent, and `ClassNotFoundException` will not be thrown Signed-off-by: xingrufei <xingrufei@sogou-inc.com>
Motivation: These cleanups were done in another PR but were not directly related to that PR. This extracts those changes and backports them to 4.1. Modification: * Remove the use of mocking in DefaultPromiseTest. * Fix a few warnings. * Make `testStackOverFlowChainedFuturesB` test with the right listener chain. Result: Cleaner code.
…Context (#11602) Motivation: At the moment why you try to build a SslContext via SslProvider.OPENSSL* and netty-tcnative* is not on the classpath it will fail with: ``` Exception in thread "main" java.lang.NoClassDefFoundError: io/netty/internal/tcnative/SSLPrivateKeyMethod at io.netty.handler.ssl.SslContext.newClientContextInternal(SslContext.java:830) at io.netty.handler.ssl.SslContextBuilder.build(SslContextBuilder.java:611) at io.netty.handler.codec.http.Main.main(Main.java:34) Caused by: java.lang.ClassNotFoundException: io.netty.internal.tcnative.SSLPrivateKeyMethod at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:581) at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178) at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:522) ... 3 more ``` We should do better here. Modifications: Call `OpenSsl.ensureAvailability()` before trying to construct OpenSsl*Context instances Result: More clear error message like: ``` java.lang.UnsatisfiedLinkError: failed to load the required native library at io.netty.handler.ssl.OpenSsl.ensureAvailability(OpenSsl.java:540) at io.netty.handler.ssl.SslContext.newClientContextInternal(SslContext.java:830) at io.netty.handler.ssl.SslContextBuilder.build(SslContextBuilder.java:611) ... Caused by: java.lang.ClassNotFoundException: io.netty.internal.tcnative.SSLContext at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:581) at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178) at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:522) at java.base/java.lang.Class.forName0(Native Method) at java.base/java.lang.Class.forName(Class.java:398) at io.netty.handler.ssl.OpenSsl.<clinit>(OpenSsl.java:126) ... 68 more ```
Motivation: The generics for the existing futures, promises, and listeners are too complicated. This complication comes from the existence of `ChannelPromise` and `ChannelFuture`, which forces listeners to care about the particular _type_ of future being listened on. Modification: * Add a `FutureContextListener` which can take a context object as an additional argument. This allows our listeners to have the channel piped through to them, so they don't need to rely on the `ChannelFuture.channel()` method. * Make the `FutureListener`, along with the `FutureContextListener` sibling, the default listener API, retiring the `GenericFutureListener` since we no longer need to abstract over the type of the future. * Change all uses of `ChannelPromise` to `Promise<Void>`. * Change all uses of `ChannelFuture` to `Future<Void>`. * Change all uses of `GenericFutureListener` to either `FutureListener` or `FutureContextListener` as needed. * Remove `ChannelFutureListener` and `GenericFutureListener`. * Introduce a `ChannelFutureListeners` enum to house the constants that previously lived in `ChannelFutureListener`. These constants now implement `FutureContextListener` and take the `Channel` as a context. * Remove `ChannelPromise` and `ChannelFuture` — all usages now rely on the plain `Future` and `Promise` APIs. * Add static factory methods to `DefaultPromise` that allow us to create promises that are initialised as successful or failed. * Remove `CompleteFuture`, `SucceededFuture`, `FailedFuture`, `CompleteChannelFuture`, `SucceededChannelFuture`, and `FailedChannelFuture`. * Remove `ChannelPromiseNotifier`. Result: Cleaner generics and more straight forward code.
Motivation: We tightly integrate the TrustManger and KeyManager into our native SSL implementation which means that both of them need to support TLSv1.3 as protocol. This is not always the case and so can produce runtime exceptions. As TLSv1.3 support was backported to Java8 quite some time now we should be a bit more conservative and only enable TLSv1.3 for our native implementation if the JDK implementation supports it as well. This also allows us to remove some hacks we had in place to be able to support it before in Java8. Modifications: - Only enable TLSv1.3 support for our native SSL implementation when the JDK supports it as well - Remove OpenSslTlsv13X509ExtendedTrustManager as its not needed anymore Result: Fixes #11589
) Motivation: Make SslHandler's handlerRemoved0 method release the sslEngine even if it fails in the middle. See details in #11595. Modifications: Wrap the release of sslEngine into a finally block. Result: The sslEngine would be released eventually. Co-authored-by: Chen Liu <cliu@splunk.com>
Motivation: In another PR we did observe a leak report for TcpDnsTest Modifications: Correctly release the query. Result: No more leaks
Motivation: We can make things easier for implementations by providing some default methods Modifications: - Add default methods to Channel - Remove code from AbstractChannel Result: Easier to implement custom Channel
…11594) Motivation: Since most futures in Netty are of the `Void` type, methods like `getNow()` and `cause()` cannot distinguish if the future has finished or not. This can cause data race bugs which, in the case of `Void` futures, can be silent. Modification: The methods `getNow()` and `cause()` now throw an `IllegalStateException` if the future has not yet completed. Most use of these methods are inside listeners, and so are not impacted. One place in `AbstractBootstrap` was doing a racy read and has been adjusted. Result: Data race bugs around `getNow()` and `cause()` are no longer silent.
Motivation: Keeping the version of netty-tcnative correct can sometimes be hard. Modifications: Add entries for netty-tcnative* to the bom Result: Fixes #11567
Motivation: At the moment the outbound operations of ChannelHandler take a Promise as argument. This Promise needs to be carried forward to the next handler in the pipeline until it hits the transport. This is API choice has a few quirks which we should aim to remove: - There is a difference between if you add a FutureListener to the Promise or the Future that is returned by the outbound method in terms of the ordering of execution of the listeners. Sometimes we add the listener to the promise while in reality we usually always want to add it to the future to ensure the listerns are executed in the "correct order". - It is quite easy to "loose" a promise by forgetting to use the right method which also takes a promise - We have no idea what EventExecutor is used for the passed in Promise which may invalid our assumption of threading. While changing the method signature of the outbound operations of the ChannelHandler is a good step forward we should also take care of just remove all the methods from ChannelOutboundInvoker (and its sub-types) that take a Promise and just always use the methods that return a Future only. Modifications: - Change the signature of the methods that took a Promise to not take one anymore and just return a Future - Remove all operations for ChannelOutboundInvoker that take a Promise. - Adjust all code to cope with the API changes Result: Cleaner API which is easier to reason about and easier to use.
Motivation: This workflow has been broken for a couple of months now. It would be nice if we could get junit test reports back in out PR checks. Modificatoin: Fix a test matrix typo. Update workflow action versions. Result: Hopefully the PR Reports workflow starts working again. Draft until it actually does work. There are probably several things wrong that will take time to work through.
…() (#11617) Motivation: We should just add `executor()` to the `ChannelOutboundInvoker` interface and override this method in `Channel` to return `EventLoop`. Modifications: - Add `executor()` method to `ChannelOutboundInvoker` - Let `Channel` override this method and return `EventLoop`. - Adjust all usages of `eventLoop()` - Add some default implementations Result: API cleanup
* Remove deprecated Channel*Handler* classes Motivation: There is no need to keep the older adapter and duplex classes around. Modifications: - Remove old adapter and duplex classes - Adjust javadocs Result: Cleanup * Address nit
Motivation: 232c669 did add overflow protection but did miss to take the array offset into account and so could report false-positives Modifications: - Correctly take offset into account when check for overflow. - Add unit tests Result: Correctly take offset into account when overflow check is performed
Motivation: We did miss to update some code that setup the EventLoop mock and so did see test-failures due NPE's Modifications: Correctly mock the newPromise() method. Result: No more test-failures
…nse (#11616) Motivation: The expression "not is success" can mean that either the future failed, or it has not yet completed. However, many places where such an expression is used is expecting the future to have completed. Specifically, they are expecting to be able to call `cause()` on the future. It is both more correct, and semantically clearer, to call `isFailed()` instead of `!isSuccess()`. Modification: Change all places that used `!isSuccess()` to mean that the future had failed, to use `isFailed()`. A few places are relying on `isSuccess()` returning `false` for _incomplete_ futures, and these places have been left unchanged. Result: Clearer code, with potentially fewer latent bugs.
Motiviation: The workflow from the default branch is the one that gets to run for all PRs, regardless of what branch they are targeting. This means the 4.1 PR Reports workflow needs to tollerate master branch PRs, where there is no Java 8 builds. Modification: Make the PR Reports tolerate that the Java 8 matrix fails if they are missing. Result: Hopefully we'll get working PR Reports for master branch PRs now.
…1607) Motivation: Making futures easier to compose, combine, and extend is useful to have as part of the API, since implementing this correctly and efficiently can be tricky. Modification: Add `Future.map(Function<V,R>) -> Future<R>` and `Future.flatMap(Function<V,Future<R>>) -> Future<R>` default methods to the `Future` interface. These methods return new Future instance, that will be completed when the original future completes, and the result will be processed through the given mapping function. These two methods take care to propagate cancellation and exceptions correctly: Cancellation propagates both ways between the new and original future. Failures only propagate from the original future to the returned new Future instance. Result: A few convenient methods for modifying and composing futures. This PR fixes #8523, and perhaps also #2105
Motivation: The need of cascade from a Future to a Promise exists. We should add some default implementation for it. Modifications: - Merge PromiseNotifier into Futures - Add default cascadeTo(...) methods to Future - Add tests to FuturesTest - Replace usage of PromiseNotifier with Future.cascadeTo - Use combination of map(...) and cascadeTo(...) in *Bootstrap to reduce code duplication Result: Provide default implementation of cascadeTo.
Motivation: Our current ChannelFutureListeners are too restrictive in terms of the type of the context. We should lift this a bit Modifications: Refactor ChannelFutureListeners to not be an enum and so allow most of its instances to be used with any ChannelOutboundInvoker. Result: More flexible usage
Motivation: We did recently change the Channel / ChannelHandler API to always act on the Future only. We should do the same for our handlers. Modifications: - Adjust http2 API - Adjust other handlers API Result: Easier to use API and more consistent
Motivation: We should not use master as branch name, better to switch to main Modifications: Replace master branch name with main Result: Use main as replacement for master
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation:
Explain here the context, and why you're making that change.
What is the problem you're trying to solve.
Modification:
Describe the modifications you've done.
Result:
Fixes #.
If there is no issue then describe the changes introduced by this PR.