Motivation:
NullChecks resulting in a NullPointerException or IllegalArgumentException, numeric ranges (>0, >=0) checks, not empty strings/arrays checks must never be anonymous but with the parameter or variable name which is checked. They must be specific and should not be done with an "OR-Logic" (if a == null || b == null) throw new NullPointerEx.
Modifications:
* Add some checks to ObjectUtil not present today but utilized in the code.
* Add unit test for ObjectUtil
* Update commmons internal usage with ObjectUtil
Result:
All checks needed are present, subsequent changes of usage of ObjectUtil are possible.
Fixes for https://github.com/netty/netty/issues/11170
Motivation:
We had a bug in out DefaulThreadFactory as it always retrieved the ThreadGroup to used during creation time when now explicit ThreadGroup was given. This is problematic as the Thread may die and so the ThreadGroup is destroyed even tho the DefaultThreadFactory is still used.
This could produce exceptions like:
java.lang.IllegalThreadStateException
at java.lang.ThreadGroup.addUnstarted(ThreadGroup.java:867)
at java.lang.Thread.init(Thread.java:405)
at java.lang.Thread.init(Thread.java:349)
at java.lang.Thread.<init>(Thread.java:599)
at io.netty.util.concurrent.FastThreadLocalThread.<init>(FastThreadLocalThread.java:60)
at io.netty.util.concurrent.DefaultThreadFactory.newThread(DefaultThreadFactory.java:122)
at io.netty.util.concurrent.DefaultThreadFactory.newThread(DefaultThreadFactory.java:106)
at io.netty.util.concurrent.ThreadPerTaskExecutor.execute(ThreadPerTaskExecutor.java:32)
at io.netty.util.internal.ThreadExecutorMap$1.execute(ThreadExecutorMap.java:57)
at io.netty.util.concurrent.SingleThreadEventExecutor.doStartThread(SingleThreadEventExecutor.java:978)
at io.netty.util.concurrent.SingleThreadEventExecutor.startThread(SingleThreadEventExecutor.java:947)
at io.netty.util.concurrent.SingleThreadEventExecutor.execute(SingleThreadEventExecutor.java:830)
at io.netty.util.concurrent.SingleThreadEventExecutor.execute(SingleThreadEventExecutor.java:818)
at io.netty.channel.AbstractChannel$AbstractUnsafe.register(AbstractChannel.java:471)
at io.netty.channel.SingleThreadEventLoop.register(SingleThreadEventLoop.java:87)
at io.netty.channel.SingleThreadEventLoop.register(SingleThreadEventLoop.java:81)
at io.netty.channel.MultithreadEventLoopGroup.register(MultithreadEventLoopGroup.java:86)
at io.netty.bootstrap.AbstractBootstrap.initAndRegister(AbstractBootstrap.java:323)
at io.netty.bootstrap.AbstractBootstrap.doBind(AbstractBootstrap.java:272)
at io.netty.bootstrap.AbstractBootstrap.bind(AbstractBootstrap.java:239)
at io.netty.incubator.codec.quic.QuicTestUtils.newServer(QuicTestUtils.java:138)
at io.netty.incubator.codec.quic.QuicTestUtils.newServer(QuicTestUtils.java:143)
at io.netty.incubator.codec.quic.QuicTestUtils.newServer(QuicTestUtils.java:147)
at io.netty.incubator.codec.quic.QuicStreamFrameTest.testCloseHalfClosure(QuicStreamFrameTest.java:48)
at io.netty.incubator.codec.quic.QuicStreamFrameTest.testCloseHalfClosureUnidirectional(QuicStreamFrameTest.java:35)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:288)
at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:282)
at java.util.concurrent.FutureTask.run(FutureTask.java:266)
at java.lang.Thread.run(Thread.java:748)
Modifications:
- If the user dont specify a ThreadGroup we will just pass null to the constructor of FastThreadLocalThread and so let it retrieve on creation time
- Adjust tests
Result:
Don't risk to see IllegalThreadStateExceptions.
Motivation:
Due a regression in fd8c1874b4e24a18c562c7013efabcb155395459 we did not correctly set the result for the returned Future if it was build for a Callable.
Modifications:
- Adjust code to call get() to retrive the correct result for notification of the future.
- Add unit test
Result:
Fixes https://github.com/netty/netty/issues/11072
Motivation:
It is possible for two separate threads to race on recycling an object.
If this happens, the object might be added to a WeakOrderQueue when it shouldn't be.
The end result of this is that an object could be acquired multiple times, without a recycle in between.
Effectively, it ends up in circulation twice.
Modification:
We fix this by making the update to the lastRecycledId field of the handle, an atomic state transition.
Only the thread that "wins" the race and succeeds in their state transition will be allowed to recycle the object.
The others will bail out on their recycling.
We use weakCompareAndSet because we only need the atomicity guarantee, and the program order within each thread is sufficient.
Also, spurious failures just means we won't recycle that particular object, which is fine.
Result:
Objects no longer risk circulating twice due to a recycle race.
This fixes#10986
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:
We had two unit tests that sometimes failed due timeouts. After insepecting these I noticed these can be improved to run faster while still do the right validation
Modifications:
- Only submit one task for execution per execute
- Cleanup
Result:
No test failures due timeout
Motivation:
All scheduled executors should behave in accordance to their API.
The bug here is that scheduled tasks were not run more than once because we executed the runnables directly, instead of through the provided runnable future.
Modification:
We now run tasks through the provided future, so that when each run completes, the internal state of the task is reset and the ScheduledThreadPoolExecutor is informed of the completion.
This allows the executor to prepare the next run.
Result:
The UnorderedThreadPoolEventExecutor is now able to run scheduled tasks more than once.
Which is what one would expect from the API.
Motivation:
DefaultAttributeMap::attr has a blocking behaviour on lookup of an existing attribute:
it can be made non-blocking.
Modification:
Replace the existing fixed bucket table using a locked intrusive linked list
with an hand-rolled copy-on-write ordered single array
Result:
Non blocking behaviour for the lookup happy path
Motivation:
We're converting `Inet4Address` to `Integer` quite frequently so it's a good idea to keep that code in `NetUtils`.
Modification:
Added ipv4AddressToInt(Inet4Address) in NetUtils
Result:
Easy conversion of `Inet4Address` to `Integer`.
Motivation
- Recycler stack and delayed queue drop ratio can only be configured with
the same value. The overall drop ratio is ratio^2.
- #10251 shows that enable drop in `WeakOrderQueue` may introduce
performance degradation. Though the final reason is not clear now,
it would be better to add option to configure delayed queue drop ratio separately.
Modification
- "io.netty.recycler.delayedQueue.ratio" as the drop ratio of delayed queue
- default "delayedQueue.ratio" is same as "ratio"
Results
Able to configure recycler delayed queue drop ratio separately
Motivation
1. It's inable to collect all object because RATIO is always >=1 after
`safeFindNextPositivePowerOfTwo`
2. Enable drop object in `WeakOrderQueue`(commit:
71860e5b94dbc665fdb8d279d3780d6fe1c618ea) enlarge the drop ratio. We
can subtly control the overall drop ratio by using `io.netty.recycler.ratio` directly,
Modification
- Remove `safeFindNextPositivePowerOfTwo` before set the ratio
Results
Able to disable drop when recycle object
Motivation:
How we did wildcard matching was not correct according to RFC6125. Beside this our implementation was quite CPU heavy.
Modifications:
- Add new DomainWildcardMappingBuilder which correctly does wildcard matching. See https://tools.ietf.org/search/rfc6125#section-6.4
- Add unit tests
- Deprecate old implementations
Result:
Correctly implement wildcard matching and improve performance
Motivation:
ImmediateExecutor needs more test cases.
Modification:
Add several test cases for ImmediateExecutor.
Result:
Improve test coverage slightly.
Motivation:
If there was always a task in the taskQueue of GlobalEvenExecutor, scheduled tasks in the
scheduledTaskQueue will never be executed.
Related to #1614
Modifications:
fix bug in GlobalEventExecutor#takeTask
Result:
fix bug
Motivation:
In #9603 the executor hung on shutdown because of an abandoned task
on another executor the first was waiting for.
Modifications:
This commit modifies the executor shutdown sequence to include
switching to SHUTDOWN state and then running all remaining tasks.
This ensures that no more tasks are scheduled after SHUTDOWN and
the last pass of running remaining tasks will take it all.
Any tasks scheduled after SHUTDOWN will be rejected.
This change preserves the functionality of graceful shutdown with
quiet period and only adds one more pass of task execution after
the default shutdown process has finished and the executor is
ready for termination.
Result:
After this change tasks that succeed to be added to the executor will
be always executed. Tasks which come late will be rejected instead of
abandoned.
Motivation
The recycling ratio is currently implemented by comparing with a masked
count. The mask operation is not free and also not necessary.
Modification
Change the count(s) to just iterate over the corresponding interval,
which requires only a comparison and no mask.
Also make "first time recycle" behaviour consistent and revert change to
RecyclerTest made in #9727.
Result
Less recycling overhead
Motivation:
At the moment we only enfore ratioMask for the Stack which means that we only guard against recycle burts when recycled from the same Thread. We should also enforce the ratioMask in the WeakOrderQueue so we also guard against the bursts when recycle from other threads.
Modifications:
- Keep counter in WeakOrderQueue to enforce ratioMask as well
- Adjust unit test
Result:
Better guard against recycle bursts which could pollute the heap unnecessary.
Motivation
This is already done internally for various reasons but it would make
sense i.m.o. as a top level concept: submitting a task to be run on the
event loop which doesn't need to run immediately but must still be
executed in FIFO order relative all other submitted tasks (be those
"lazy" or otherwise).
It's nice to separate this abstract "relaxed" semantic from concrete
implementations - the simplest is to just delegate to existing execute,
but for the main EL impls translates to whether a wakeup is required
after enqueuing.
Having a "global" abstraction also allows for simplification of our
internal use - for example encapsulating more of the common scheduled
future logic within AbstractScheduledEventExecutor.
Modifications
- Introduce public LazyRunnable interface and
AbstractEventExecutor#lazyExecute method (would be nice for this to be
added to EventExecutor interface in netty 5)
- Tweak existing SingleThreadEventExecutor mechanics to support these
- Replace internal use of NonWakeupRunnable (such as for pre-flush
channel writes)
- Uplift scheduling-related hooks into AbstractScheduledEventExecutor,
eliminating intermediate executeScheduledRunnable method
Result
Simpler code, cleaner and more useful/flexible abstractions - cleaner in
that they fully communicate the intent in a more general way, without
implying/exposing/restricting implementation details
Motivation:
At the current moment HttpContentEncoder handle only first value of multiple accept-encoding headers.
Modification:
Join multiple accept-encoding headers to one separated by comma.
Result:
Fixes#9553
Motivation
#9152 reverted some static exception reuse optimizations due to the
problem with Throwable#addSuppressed() raised in #9151. This introduced
a performance issue when promises are cancelled at a high frequency due
to the construction cost of CancellationException at the time that
DefaultPromise#cancel() is called.
Modifications
- Reinstate the prior static CANCELLATION_CAUSE_HOLDER but use it just
as a sentinel to indicate cancellation, constructing a new
CancellationException only if/when one needs to be explicitly
returned/thrown
- Subclass CancellationException, overriding fillInStackTrace() to
minimize the construction cost in these cases
Result
Promises are much cheaper to cancel. Fixes#9522.
Motivation:
AsciiString.contentEqualsIgnoreCase may return true for non-matching strings of equal length when offset is non zero.
Modifications:
- Correctly take offset into account
- Add unit test
Result:
Fixes#9475
Motivation:
There is some manual coping of elements of Collections which can be replaced by Collections.addAll(...) and also some unnecessary semicolons.
Modifications:
- Simplify branches
- Use Collections.addAll
- Code cleanup
Result:
Code cleanup
Motivation:
asList should only be used if there are multiple elements.
Modification:
Call to asList with only one argument could be replaced with singletonList
Result:
Cleaner code and a bit of memory savings
Motivation
@xiaoheng1 reported incorrect behaviour of AsciiString.lastIndexOf in
#9099. Upon closer inspection it appears that it was never implemented
correctly and searches between the provided index and the end of the
string similar to indexOf(...), rather than between the provided index
and the beginning of the string as the javadoc states (and in line with
java.lang.String).
Modifications
Fix AsciiString.lastIndexOf implementation and corresponding unit tests
to behave the same as the equivalent String methods.
Result
Fixes#9099
Motivation:
In GlobalEventExecutorTest we used Thread.sleep(...) which can produce flaky results (as seen on the CI). We should use another alternative during tests.
Modifications:
Replace Thread.sleep(...) with join()
Result:
No more flaky GlobalEventExecutor tests.
Motivation:
PooledByteBufAllocator uses a PoolThreadCache per Thread that allocates / deallocates to minimize the performance overhead. This PoolThreadCache is trimmed after X allocations to free up buffers that are not allocated for a long time. This works out quite well when the app continues to allocate but fails if the app stops to allocate frequently (for whatever reason) and so a lot of memory is wasted and not given back to the arena / freed.
Modifications:
- Add a ThreadExecutorMap that offers multiple methods that wrap Runnable / ThreadFactory / Executor and allow to call ThreadExecutorMap.currentEventExecutor() to get the current executing EventExecutor for the calling Thread.
- Use these methods in the constructors of our EventExecutor implementations (which also covers the EventLoop implementations)
- Add io.netty.allocator.cacheTrimIntervalMillis system property which can be used to specify a fixed rate / interval on which we should try to trim the PoolThreadCache for a EventExecutor that allocates.
- Add PooledByteBufAllocator.trimCurrentThreadCache() to allow the user to trim the cache of the calling thread manually.
- Add testcases
- Introduce FastThreadLocal.getIfExists()
Result:
Allow to better / more frequently trim PoolThreadCache and so give back memory to the area / system.
Motivation:
2bb9f64e16dbb5cbbf691e284a97d745378a7b8a introduced a change which made it possible to use different shaded versions of netty-tcnative on the classpath. This only partly worked as we did not correctly handled the case when os / arch is part of the library name (which is the case when netty-tcnative-boringssl-static is used with the uber jar).
Modifications:
- If patching the ID failed we retry again with the os / arch stripped
- Add unit tests to verify that patching ID now works with and without os / arch as suffix.
Result:
Using multiple shaded version of netty-tcnative-boringssl-static on MacOS works.
Motivation:
PromiseCombiner is not thread-safe and even assumes all added Futures are using the same EventExecutor. This is kind of fragile as we do not enforce this. We need to enforce this contract to ensure it's safe to use and easy to spot concurrency problems.
Modifications:
- Add new contructor to PromiseCombiner that takes an EventExecutor and deprecate the old non-arg constructor.
- Check if methods are called from within the EventExecutor thread and if not fail
- Correctly dispatch on the right EventExecutor if the Future uses a different EventExecutor to eliminate concurrency issues.
Result:
More safe use of PromiseCombiner + enforce correct usage / contract.
Motivation:
https://github.com/netty/netty/pull/8866 added support for calling Iterator.remove() but did not add a testcase.
Modifications:
Add testcase to ensure removal works.
Result:
Better test-coverage.
Motivation:
When we fail a call to PromiseCombiner.finish(...) because of a null argument we must not update the internal state before throwing.
Modifications:
- First do the null check and only after we validated that the argument is not null update the internal state
- Add test case.
Modifications:
Do not mess up internal state of PromiseCombiner when finish(...) is called with a null argument.
Result:
After your change, what will change.
Motivation
As pointed out by @91he in
https://github.com/netty/netty/pull/8595#issuecomment-459181794, there
is a remaining bug in LocationAwareSlf4JLogger following the updates
done in #8595. The logging methods which take a varargs message
parameter array should format using MessageFormatter.arrayFormat rather
than MessageFormatter.format.
Modifications
Change varargs param methods in LocationAwareSlf4JLogger to use
MessageFormatter.arrayFormat and extend unit test to cover these cases.
Results
Correct log output when logging messages with > 2 parameters when using
LocationAwareSlf4JLogger.
Motivation:
If there are no listeners attached to the promise when full-filling it we do not need to schedule a task to notify.
Modifications:
- Don't schedule a task if there is nothing to notify.
- Add unit tests.
Result:
Fixes https://github.com/netty/netty/issues/8795.
Motivation:
To conform to the CharSequence interface we need to return an empty CharSequence when start == end index and a subSequence is requested.
Modifications:
- Correctly handle the case where start == end
- Add unit test
Result:
Fix https://github.com/netty/netty/issues/8796.
Motivation:
Incrementing two variables in sync is not necessary when only one will do.
Modifications:
- Remove `j` from `for` loop and replace with `i`.
- Add more unit testing scenarios to cover changed code.
Results:
Unnecessary variable removed.
Motivation
#8563 highlighted race conditions introduced by the prior optimistic
update optimization in 83a19d565064ee36998eb94f946e5a4264001065. These
were known at the time but considered acceptable given the perf
benefit in high contention scenarios.
This PR proposes a modified approach which provides roughly half the
gains but stronger concurrency semantics. Race conditions still exist
but their scope is narrowed to much less likely cases (releases
coinciding with retain overflow), and even in those
cases certain guarantees are still assured. Once release() returns true,
all subsequent release/retains are guaranteed to throw, and in
particular deallocate will be called at most once.
Modifications
- Use even numbers internally (including -ve) for live refcounts
- "Final" release changes to odd number (equivalent to refcount 0)
- Retain still uses faster getAndAdd, release uses CAS loop
- First CAS attempt uses non-volatile read
- Thread.yield() after a failed CAS provides a net gain
Result
More (though not completely) robust concurrency semantics for ref
counting; increased latency under high contention, but still roughly
twice as fast as the original logic. Bench results to follow
Motivation:
We did miss to use MessageFormatter inside LocationAwareSlf4jLogger and so {} was not correctly replaced in log messages when using slf4j.
This regression was introduced by afe0767e9c150369cd6ea150b5cc1a9021fd5c3c.
Modifications:
- Make use of MessageFormatter
- Add unit test.
Result:
Fixes https://github.com/netty/netty/issues/8483.
* Log the correct line-number when using SLF4j with netty if possible.
Motivation:
At the moment we do not log the correct line number in many cases as it will log the line number of the logger wrapper itself. Slf4j does have an extra interface that can be used to filter out these nad make it more usable with logging wrappers.
Modifications:
Detect if the returned logger implements LocationAwareLogger and if so make use of its extra methods to be able to log the correct origin of the log request.
Result:
Better logging when using slf4j.
Motivation:
Log4J2Logger had some code-duplication with AbstractInternalLogger
Modifications:
Reuse AbstractInternaLogger.EXCEPTION_MESSAGE in Log4J2Logger and so remove some code-duplication
Result:
Less duplicated code.
Motivation:
There was a race condition between the task submitter and task executor threads such that the last Runnable submitted may not get executed.
Modifications:
The bug was fixed by checking the task queue and state in the task executor thread after it saw the task queue was empty.
Result:
Fixes#8230
Motivation:
Recycler may produce a NPE when the same object is recycled multiple times from different threads.
Modifications:
- Check if the id has changed or if the Stack became null and if so throw an IllegalStateException
- Add unit test
Result:
Fixes https://github.com/netty/netty/issues/8220.
Motivation:
We do not correctly check for previous calles of setUncancellable() in getNow() which may result in ClassCastException as we incorrectly return the internally UNCANCELLABLE object and not null if setUncancellable() we as called before.
Modifications:
Correctly check for UNCANCELLABLE and add unit test.
Result:
Fixes https://github.com/netty/netty/issues/8135.
Motivation:
We incorrectly calculated the length that was used for our for loop in AsciiString.indexOf(...). This lead to a possible ArrayIndexOutOfBoundsException.
Modifications:
- Not include the start in the length calculation
- Add unit test.
Result:
Fixes https://github.com/netty/netty/issues/8112.
Motivation:
ObjectCleaner does start a Thread to handle the cleaning of resources which leaks into the users application. We should not use it in netty itself to make things more predictable.
Modifications:
- Remove usage of ObjectCleaner and use finalize as a replacement when possible.
- Clarify javadocs for FastThreadLocal.onRemoval(...) to ensure its clear that remove() is not guaranteed to be called when the Thread completees and so this method is not enough to guarantee cleanup for this case.
Result:
Fixes https://github.com/netty/netty/issues/8017.
Motivation:
We should allow to schedule tasks with a delay up to Long.MAX_VALUE as we did pre 4.1.25.Final.
Modifications:
Just ensure we not overflow and put the correct max limits in place when schedule a timer. At worse we will get a wakeup to early and then schedule a new timeout.
Result:
Fixes https://github.com/netty/netty/issues/7970.
