Motivation:
It is important to avoid blocking method calls in an event loop thread, since that can stall the system.
Netty's Future interface was extending the JDK Future interface, which included a number of blocking methods of questionable use in Netty.
We wish to reduce the number of blocking methods on the Future API in order to discourage their use a little.
Further more, the Netty Future specification of the behaviour of the cancel() and isDone() methods are inconsistent with those of the JDK Future.
If Netty's Future stop extending the JDK Future interface, it will also no longer be bound by its specification.
Modification:
Make Netty's Future no longer extend the JDK Future interface.
Change the EvenExecutorGroup interface to no longer extend ScheduledExecutorService.
The EventExecutorGroup still extends Executor, because Executor does not dictate any return type of the `execute()` method — this is also useful in the DefaultFutureCompletionStage implementation.
The Netty ScheduledFuture interface has been removed since it provided no additional features that were actually used.
Numerous changes to use sites that previously relied on the JDK types.
Remove the `Future.cancel()` method that took a boolean argument — this argument was always ignored in our implementations, which was another spec deviation.
Various `invoke*` and `shutdown*` methods have been removed from the EvenExecutorGroup API since it no longer extends ScheduledExecutorService — these were either not used anywhere, or deprecated with better alternatives available.
Updates to cancellation javadocs.
Result:
Cleaner code, leaner API.
__Motivation__
As we start to migrate codecs to use the new `Buffer` API, we need a way for them to get a handle of `BufferAllocator`.
__Modification__
Added `bufferAllocator()` method to `ChannelConfig`, `Channel` and `ChannelHandlerContext`
__Result__
Codecs can allocate `Buffer` instances
Motivation:
When the ByteBuf size exceeds the max limit/defined size, IOException is thrown.
HttpData#addContent/setContent should release the buffer in such cases otherwise memory leak will happen.
Modification:
- Release the provided ByteBuf before throwing IOException
- Add unit tests
Result:
Fixes#11618
Motivation:
We wish to separate these two into clearer write/read interfaces.
In particular, we don't want to be able to add listeners to promises, because it makes it easy to add them out of order.
We can't prevent it entirely, because any promise can be freely converted to a future where listeners can be added.
We can, however, discourage this in the API.
Modification:
The Promise interface no longer extends the Future interface.
Numerous changes to make the project compile and its tests run.
Result:
Clearer separation of concerns in the code.
Motivation:
If we don't need the scheduled future, then it will be one less complication when we change Netty Future to no longer extend JDK Future.
It would also result in fewer elements in our API.
Modification:
There was only one real use of ScheduledFuture in our code, in Cache.
This has been changed to wrap an ordinary future with a deadline for implementing the Delayed interface.
All other places were effectively overspecifying by relying on ScheduledFuture.
A few places were also specifying JDK Futures - these have been changed to specify Netty Futures.
Result:
Reduced dependency on the ScheduledFuture interfaces.
Motivation:
Usually the outbound operation should start at the "current" ChanneöHandlercontext which was often not the case
Modifications:
Use the ChannelHandlerContext for closing the connection
Result:
Start the operation on the right position of the pipeline
Motivation:
DiskFileUpload#toString should use the value provided when constructing DiskFileUpload
and not the default one
Modification:
- DiskFileUpload#toString is modified to use the correct deleteOnExit value
Result:
DiskFileUpload#toString returns a string that uses the correct deleteOnExit
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:
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:
In this issue(#11578 ) discussed that tilde should not be encoded in QueryStringEncoder, this pr is to fix this problem
Modification:
Modified QueryStringEncoder so that it does not encode tilde, and added a test case
Result:
Fixes#11578
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.
* 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:
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
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:
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:
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:
- 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:
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>
__Motivation__
Since request.headers().getAll() will never return null. And the check null condition will not work as expected.
__Modification__
Add isEmpty() checking as well.
__Result__
Fixes#11568
Motivation:
We should make variables `final` which are not reinstated again in code to match the code style and makes the code look better.
Modification:
Made couples of variables as `final`.
Result:
Variables marked as `final`.
Motivation:
Let's have fewer warnings about broken, missing, or abuse of javadoc comments.
Modification:
Added descriptions to throws clauses that were missing them.
Remove link clauses from throws clauses - these are implied.
Turned some javadoc comments into block comments because they were not applied to APIs.
Use code clauses instead of code tags.
Result:
Fewer javadoc crimes.
Motivation:
We should get rid of the unnecessary toString calls because they're redundant in nature.
Modification:
Removed unnecessary toString calls.
Result:
Better code
Motivation:
We should get rid of unnecessary semicolons because they don't do anything in code.
Modification:
Removed unnecessary semicolons.
Result:
Better code
Motivation:
There are lots of imports which are unused. We should get rid of them to make the code look better,
Modification:
Removed unused imports.
Result:
No unused imports.
Bootstrap methods now return Future<Channel> instead of ChannelFuture
Motivation:
In #8516 it was proposed to at some point remove the specialised ChannelFuture and ChannelPromise.
Or at least make them not extend Future and Promise, respectively.
One pain point encountered in this discussion is the need to get access to the channel object after it has been initialised, but without waiting for the channel registration to propagate through the pipeline.
Modification:
Add a Bootstrap.createUnregistered method, which will return a Channel directly.
All other Bootstrap methods that previously returned ChannelFuture now return Future<Channel>
Result:
It's now possible to obtain an initialised but unregistered channel from a bootstrap, without blocking.
And the other bootstrap methods now only release their channels through the result of their futures, preventing racy access to the channels.
Motivation:
At the moment we not correctly propagate cancellation in some case when we use the PromiseNotifier.
Modifications:
- Add PromiseNotifier static method which takes care of cancellation
- Add unit test
- Deprecate ChannelPromiseNotifier
Result:
Correctly propagate cancellation of operation
Co-authored-by: Nitesh Kant <nitesh_kant@apple.com>
Motivation:
#11468 was merged but didn't fix tests completely. There is a fight between `LF` and `CRLF`. So to eliminate this, we should just get rid of them.
Modification:
Use a small sample dataset without `LF` and `CRLF`.
Result:
Simple and passing test.
Motivation:
JavaDoc of StandardCompressionOptions should point towards public methods. Also, Brotli tests were failing on Windows.
Modification:
Fixed JavaDoc and enabled Brotli tests on Windows.
Result:
Better JavaDoc and Brotli tests will run on Windows
Co-authored-by: Norman Maurer <norman_maurer@apple.com>
Motivation:
In #11256, We introduced `Iterable` as a parameter but later in review, it was removed. But we forgot to change `compressionOptionsIterable` to just `compressionOptions`.
Modification:
Changed `compressionOptionsIterable` to `compressionOptions`.
Result:
Correct ObjectUtil message
Motivation:
Currently, Netty only has BrotliDecoder which can decode Brotli encoded data. However, BrotliEncoder is missing which will encode normal data to Brotli encoded data.
Modification:
Added BrotliEncoder and CompressionOption
Result:
Fixes#6899.
Co-authored-by: Norman Maurer <norman_maurer@apple.com>
Motivation:
ZSTD has a wide range of uses on the Internet, so should consider adding `application/zstd` HTTP media-type and `zstd` content-encoding, see https://tools.ietf.org/html/rfc8478
Modification:
Add `application/zstd` HTTP media-type and `zstd` content-encoding
Result:
netty provides `application/zstd` HTTP media-type and `zstd content-encoding` as http headers
Signed-off-by: xingrufei <xingrufei@sogou-inc.com>
Co-authored-by: xingrufei <xingrufei@sogou-inc.com>
Motivation:
At the moment all methods in `ChannelHandler` declare `throws Exception` as part of their method signature. While this is fine for methods that handle inbound events it is quite confusing for methods that handle outbound events. This comes due the fact that these methods also take a `ChannelPromise` which actually need to be fullfilled to signal back either success or failure. Define `throws...` for these methods is confusing at best. We should just always require the implementation to use the passed in promise to signal back success or failure. Doing so also clears up semantics in general. Due the fact that we can't "forbid" throwing `RuntimeException` we still need to handle this in some way tho. In this case we should just consider it a "bug" and so log it and close the `Channel` in question. The user should never have an exception "escape" their implementation and just use the promise. This also clears up the ownership of the passed in message etc.
As `flush(ChannelHandlerContext)` and `read(ChannelHandlerContext)` don't take a `ChannelPromise` as argument this also means that these methods can never produce an error. This makes kind of sense as these really are just "signals" for the underlying transports to do something. For `RuntimeException` the same rule is used as for other outbound event handling methods, which is logging and closing the `Channel`.
Motifications:
- Remove `throws Exception` from signature
- Adjust code to not throw and just notify the promise directly
- Adjust unit tests
Result:
Much cleaner API and semantics.
Motivation:
The `PerMessageDeflateClientExtensionHandler` has the following strange behaviors currently:
* The `requestedServerNoContext` parameter doesn't actually add the `server_no_context_takeover` parameter to the client offer; instead it depends on the requested server window size.
* The handshake will fail if the server responds with a `server_no_context_takeover` parameter and `requestedServerNoContext` is false. According to RFC 7692 (7.1.1.1) the server may do this, and this means that to cover both cases one needs to use two handshakers in the channel pipeline: one with `requestedServerNoContext = true` and one with `requestedServerNoContext = false`.
* The value of the `server_max_window_bits` parameter in the server response is never checked (should be between 8 and 15). And the value of `client_max_window_bits` is checked only in the branch handling the server window parameter.
Modification:
* Add the `server_no_context_takeover` parameter if `requestedServerNoContext` is true.
* Accept a server handshake response which includes the server no context takeover parameter even if we did not request it.
* Check the values of the client and server window size in their respective branches and fail the handshake if they are out of bounds.
Result:
There will be no need to use two handshakers in the pipeline to be lenient in what handshakes are accepted.
Motivation:
Including codec-http in the project and building a native-image out of it using a GraalVM 21.2 nightly can result in a failure.
Modification:
By delaying the initialization of `io.netty.handler.codec.compression.BrotliDecoder` to runtime, native-image will not try to eagerly initialize the class during the image build, avoiding the build failure described in the issue.
Result:
Fixes#11427
Motivation:
HTTP header values are case sensitive. The expected value for `x-request-with` header is `XMLHttpRequest`, not `XmlHttpRequest`.
Modification:
Fix constant's case.
Result:
Correct `XMLHttpRequest` HTTP header value.
__Motivation__
`HttpUtil#normalizeAndGetContentLength()` throws `StringIndexOutOfBoundsException` for empty `content-length` values, it should instead throw `IllegalArgumentException` for all invalid values.
__Modification__
- Throw `IllegalArgumentException` if the `content-length` value is empty.
- Add tests
__Result__
Fixes https://github.com/netty/netty/issues/11408
Motivation:
Netty will fail a handshake for the Per-Message Deflate WebSocket
extension if the server response contains a smaller
`server_max_window_bits` value than the client offered.
However, this is allowed by RFC 7692:
> A server accepts an extension negotiation offer with this parameter
> by including the “server_max_window_bits” extension parameter in the
> extension negotiation response to send back to the client with the
> same or smaller value as the offer.
Modifications:
- Allow the server to respond with a smaller value than offered.
- Change the unit tests to test for this.
Result:
The client will not fail when the server indicates it is using a
smaller window size than offered by the client.
__Motivation__
As described in https://github.com/netty/netty/issues/11370 we should support quoted charset values
__Modification__
Modify `HttpUtil.getCharset(CharSequence contentTypeValue, Charset defaultCharset)` to trim the double-quotes if present.
__Result__
`HttpUtil.getCharset()` now supports quoted charsets. Fixes https://github.com/netty/netty/issues/11370
Motivation:
This special case implementation of Promise / Future requires the implementations responsible for completing the promise to have knowledge of this class to provide value. It also requires that the implementations are able to provide intermediate status while the work is being done. Even throughout the core of Netty it is not really supported most of the times and so just brings more complexity without real gain.
Let's remove it completely which is better then only support it sometimes.
Modifications:
Remove Progressive* API
Result:
Code cleanup.... Fixes https://github.com/netty/netty/issues/8519
Motivation:
Sometime in the past we introduced the concept of Void*Promise. As it turned out this was not a good idea at all as basically each handler in the pipeline need to be very careful to correctly handle this. We should better just remove this "optimization".
Modifications:
- Remove Void*Promise and all the related APIs
- Remove tests which were related to Void*Promise
Result:
Less error-prone API
Motivation:
When decoding the cookies on the server, the "Cookie" HTTP request header value should be considered.
The "Set-Cookie" HTTP response header is used to send cookies from the server to the user agent.
Modification:
- Specify in javadoc that the "Cookie" HTTP request header value should be considered and
not the "Set-Cookie" HTTP response header value.
Result:
Correct ServerCookieDecoder javadoc
Motivation:
There should always be a default in switch blocks.
Modification:
Add default
Result:
Code cleanup.
Signed-off-by: xingrufei <xingrufei@sogou-inc.com>
Motivation:
There should always be a default in switch blocks.
Modification:
Add default
Result:
Code cleanup.
Signed-off-by: xingrufei <xingrufei@sogou-inc.com>
Motivation:
When searching for the delimiter, the decoder part within HttpPostBodyUtil
was not checking the left space to check if it could be included or not,
while it should.
Modifications:
Add a check on toRead being greater or equal than delimiterLength before
going within the loop. If the check is wrong, the delimiter is obviously not found.
Add a Junit test to preserve regression.
Result:
No more IndexOutOfBoundsException
Fixes#11334
Motivation:
Every switch block should also have a default case.
Modification:
Add default block in DefaultHttpHeaders to ensure we not fall-through by mistake
Result:
Cleanup
Signed-off-by: xingrufei <xingrufei@sogou-inc.com>
Motivation:
JUnit 5 is more expressive, extensible, and composable in many ways, and it's better able to run tests in parallel.
Modifications:
Use JUnit5 in tests
Result:
Related to https://github.com/netty/netty/issues/10757
Motivation:
Let's also build on windows during PR validation
Modifications:
Add build on windows during PR
Result:
Validate that all also pass on windows
