__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:
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:
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:
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:
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 codec-memcache tests
Result:
Related to https://github.com/netty/netty/issues/10757
Motivation:
To make it possible to experiment with alternative buffer implementations, we need a way to abstract away the concrete buffers used throughout most of the Netty pipelines, while still having a common currency for doing IO in the end.
Modification:
- Introduce an ByteBufConvertible interface, that allow arbitrary objects to convert themselves into ByteBuf objects.
- Every place in the code, where we did an instanceof check for ByteBuf, we now do an instanceof check for ByteBufConvertible.
- ByteBuf itself implements ByteBufConvertible, and returns itself from the asByteBuf method.
Result:
It is now possible to use Netty with alternative buffer implementations, as long as they can be converted to ByteBuf.
This has been verified elsewhere, with an alternative buffer implementation.
Motivation:
HTTP is a plaintext protocol which means that someone may be able
to eavesdrop the data. To prevent this, HTTPS should be used whenever
possible. However, maintaining using https:// in all URLs may be
difficult. The nohttp tool can help here. The tool scans all the files
in a repository and reports where http:// is used.
Modifications:
- Added nohttp (via checkstyle) into the build process.
- Suppressed findings for the websites
that don't support HTTPS or that are not reachable
Result:
- Prevent using HTTP in the future.
- Encourage users to use HTTPS when they follow the links they found in
the code.
Motivation:
junit deprecated Assert.assertThat(...)
Modifications:
Use MatcherAssert.assertThat(...) as replacement for deprecated method
Result:
Less deprecation warnings
Motivation:
ByteToMessageDecoder requires using an intermediate List to put results into. This intermediate list adds overhead (memory/CPU) which grows as the number of objects increases. This overhead can be avoided by directly propagating events through the ChannelPipeline via ctx.fireChannelRead(...). This also makes the semantics more clear and allows us to keep track if we need to call ctx.read() in all cases.
Modifications:
- Remove List from the method signature of ByteToMessageDecoder.decode(...) and decodeLast(...)
- Adjust all sub-classes
- Adjust unit tests
- Fix javadocs.
Result:
Adjust ByteToMessageDecoder as noted in https://github.com/netty/netty/issues/8525.
Motivations
-----------
Calling `copy()`, `duplicate()` or `replace()` on `FullBinaryMemcacheResponse`
or `FullBinaryMemcacheRequest` instances should copy status, opCode, etc.
that are defined in `AbstractBinaryMemcacheMessage`.
Modifications
-------------
- Modified duplicate, copy and replace methods in
DefaultFullBinaryMemcacheRequest and DefaultFullBinaryMemcacheResponse
to always copy metadata from parent classes.
- Unit tests verifying duplicate, copy and replace methods for
DefaultFullBinaryMemcacheRequest and DefaultFullBinaryMemcacheResponse
copy buffers and metadata as expected.
Result
------
Calling copy(), duplicate() or replace() methods on
DefaultFullBinaryMemcacheRequest or DefaultFullBinaryMemcacheResponse
produces valid copies with all expected metadata.
Fixes#9159
Motivation:
We can replace some "hand-rolled" integer checks with our own static utility method to simplify the code.
Modifications:
Use methods provided by `ObjectUtil`.
Result:
Cleaner code and less duplication
Motivation:
We can just use Objects.requireNonNull(...) as a replacement for ObjectUtil.checkNotNull(....)
Modifications:
- Use Objects.requireNonNull(...)
Result:
Less code to maintain.
Motivation:
While working on #6087 some buffer leaks showed up.
Modifications:
Correctly release buffers.
Result:
No more buffer leaks in memcache and stomp codec tests.
Motivation:
the build doesnt seem to enforce this, so they piled up
Modifications:
removed unused import lines
Result:
less unused imports
Signed-off-by: radai-rosenblatt <radai.rosenblatt@gmail.com>
Related: #4333#4421#5128
Motivation:
slice(), duplicate() and readSlice() currently create a non-recyclable
derived buffer instance. Under heavy load, an application that creates a
lot of derived buffers can put the garbage collector under pressure.
Modifications:
- Add the following methods which creates a non-recyclable derived buffer
- retainedSlice()
- retainedDuplicate()
- readRetainedSlice()
- Add the new recyclable derived buffer implementations, which has its
own reference count value
- Add ByteBufHolder.retainedDuplicate()
- Add ByteBufHolder.replace(ByteBuf) so that..
- a user can replace the content of the holder in a consistent way
- copy/duplicate/retainedDuplicate() can delegate the holder
construction to replace(ByteBuf)
- Use retainedDuplicate() and retainedSlice() wherever possible
- Miscellaneous:
- Rename DuplicateByteBufTest to DuplicatedByteBufTest (missing 'D')
- Make ReplayingDecoderByteBuf.reject() return an exception instead of
throwing it so that its callers don't need to add dummy return
statement
Result:
Derived buffers are now recycled when created via retainedSlice() and
retainedDuplicate() and derived from a pooled buffer
Motivation:
Some codecs should be considered unstable as these are relative new. For this purpose we should introduce an annotation which these codecs should us to be marked as unstable in terms of API.
Modifications:
- Add UnstableApi annotation and use it on codecs that are not stable
- Move http2.hpack to http2.internal.hpack as it is internal.
Result:
Better document unstable APIs.
Motivation:
It's better to have some tests for FullMemcacheMessageRequest and FullMemcacheMessageResponse to avoid regression like #5197.
Modifications:
Add tests for FullMemcacheMessageRequest and FullMemcacheMessageResponse.
Result:
There are some basic tests for FullMemcacheMessageRequest and FullMemcacheMessageResponse.
Motivation:
This reverts commit 3405aee2ab. This commit introduces a bug and the encoder no longer encodes FullMemcacheMessage objects correctly.
Modifications:
- Revert commit
Result:
Fixes https://github.com/netty/netty/issues/5197
Motivation:
People need to set all length fields manually when creating a memcache message and it's error prone. See #2736 for more dicussion.
Modifications:
This patch adds the logic to update the keyLength, extrasLength and totalBodyLength when key, extras or content is set.
Result:
The length fields of memcache messages will be updated automatically.
Motivation:
The key can be ByteBuf to avoid converting between ByteBuf and String. See #3689.
Modifications:
Replace the type of key with ByteBuf.
Result:
The type of key becomes ByteBuf.
Motivation:
Some duplicated methods in message types of codec-memcache can be cleaned using AbstractReferenceCounted.
Modifications:
Use AbstractReferenceCounted to avoid duplicated methods.
Result:
Duplicated methods are cleaned.
Motivation:
BinaryMemcacheObjectAggregator doesn't retain ByteBuf `extras`. So `io.netty.util.IllegalReferenceCountException: refCnt: 0, decrement: 1` will be thrown when aggregating a message containing `extras`. See the unit test for an example.
Modifications:
`ratain` extras to fix IllegalReferenceCountException.
Result:
`extras` is retained.
Motivation:
AbstractBinaryMemcacheDecoder.currentMessage is not retained after sending it out. Hence, if a message contains `extras`, `io.netty.util.IllegalReferenceCountException` will be thrown in `channelInactive`.
Modifications:
Retain AbstractBinaryMemcacheDecoder.currentMessage After putting it to `out` and release it when it's not used.
Result:
No IllegalReferenceCountException or leak.
Motivation
----------
Currently, only the fixed 24 bytes are allocated for the header and
then all the params as well as the optional extras and key are written
into the header section.
It is very likely that the buffer needs to expand at least two times
if either the extras and/or the key take up more space.
Modifications
-------------
Since at the point of allocation we know the key and extras length,
the buffer can be preallocated with the exact size, avoiding unnecessary
resizing and even allocating too much (since it uses power of two
internally).
Result
------
Less buffer resizing needed when encoding a memcache operation.
Motivation:
ChannelInboundHandler and ChannelOutboundHandler both can implement exceptionCaught(...) method and so we need to dispatch to both of them.
Modifications:
- Correctly first dispatch exceptionCaught to the ChannelInboundHandler but also make sure the next handler it will be dispatched to will be the ChannelOutboundHandler
- Add removeInboundHandler() and removeOutboundHandler() which allows to remove one of the combined handlers
- Let *Codec extends it and not ChannelHandlerAppender
- Remove ChannelHandlerAppender
Result:
Correctly handle events and also have same behavior as in 4.0
Motivation:
As part of merging / cherry-picking there were some Erroneous imports added to AbstractMemcacheObjectAggregator.
Modifications:
- Remove Imports from AbstractMemcacheObjectAggregator.
Result:
Code now builds.
Motivation:
The HttpObjectAggregator always responds with a 100-continue response. It should check the Content-Length header to see if the content length is OK, and if not responds with a 417.
Modifications:
- HttpObjectAggregator checks the Content-Length header in the case of a 100-continue.
Result:
HttpObjectAggregator responds with 417 if content is known to be too big.
Motivation:
Current AbstractMemcacheObjectEncoder does unnecessary message type checking if the message is MemcacheMessage type.
Modifications:
Returns after encoding MemcacheMessage message.
Result:
Small performance improvement for this encoder.
If a connection is closed unexpectedly while
AbstractBinaryMemcacheDecoder decodes a message, the half-constructed
message's content might not be released.
Motivation:
Persuit for consistent method naming across all classes
Modifications:
Remove 'get' prefix for the getter methods in codec-memcache
Result:
More consistent method naming
Motivation:
DefaultFullBinaryMemcacheRequest/Response overrides release(), retain(),
and touch() methods without calling its super, resulting in a leak of
the extras.
Modifications:
When overriding release(), retain(), and touch(), ensure to call super.
Result:
Fixes#2533 by fixing the buffer leak
Motivation:
We have different message aggregator implementations for different
protocols, but they are very similar with each other. They all stems
from HttpObjectAggregator. If we provide an abstract class that provide
generic message aggregation functionality, we will remove their code
duplication.
Modifications:
- Add MessageAggregator which provides generic message aggregation
- Reimplement all existing aggregators using MessageAggregator
- Add DecoderResultProvider interface and extend it wherever possible so
that MessageAggregator respects the state of the decoded message
Result:
Less code duplication
Motivation:
4 and 5 were diverged long time ago and we recently reverted some of the
early commits in master. We must make sure 4.1 and master are not very
different now.
Modification:
Fix found differences
Result:
4.1 and master got closer.
Motivation:
When no currentMessage has been set and the channel is inactive, a NPE is raised.
Modification:
Make sure that a currentMessage is available before checking the extras.
Result:
No more NPE raised potentially.
This changeset removes the separate message headers and merges the
field directly into the messages. This greatly simplifies the
object hierachy and also saves header allocations in the pipeline.
This changeset is related to #2182, which exposes the failure in
the http codec, but the memcache codec works very similar. In addition,
better failure handling in the decoder has been added.
- Related: #2163
- Add ResourceLeakHint to allow a user to provide a meaningful information about the leak when touching it
- DefaultChannelHandlerContext now implements ResourceLeakHint to tell where the message is going.
- Cleaner resource leak report by excluding noisy stack trace elements
