Motivation:
WeakOrderQueue would drop object that has been recycled, even when it has space for it.
WeakOrderQueue#add should check DefaultHandler.hasBeenRecycler field first
Modifications:
WeakOrderQueue test the DefaultHandler.hasBeenRecycler first
Result:
WeakOrderQueue would not drop object that has been recycled when there is space
Co-authored-by: Norman Maurer <norman_maurer@apple.com>
Co-authored-by: Trustin Lee <t@motd.kr>
Motivation:
We need to use a GraalVM dependency which uses GPL2 + CE.
Modifications:
- Update all graalvm dependencies to new GAV which introduces a license change from GPL2 to GPL2 + CE
- This also required a small bump on the general version from 19.2 to 19.3, which should be fine as 19.3 is an official maintained LTS version, while 19.2 wasn't.
Result:
Fixes: #11398
Signed-off-by: Paulo Lopes <pmlopes@gmail.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:
1. The docs about the 'retun value' of the method `io.netty.util.internal.MathUtil#isOutOfBounds` is not correct.
2. The capacity parameter should be checked for overflowed case.
Modification:
1. Changed the doc to:
> @return {@code false} if the requested {@code index} and {@code length} will fit within {@code capacity}.
> {@code true} if this would result in an index out of bounds exception.
2. Improved the bounder checking logic.
Result:
Fixes#11279Fixes#11280
Removes flag by Whitesource vulnerability scanner
Motivation:
WhiteSource vulnerability scan flags the Log4J 1.x stream as vulnerable.
Modification:
Replaced reference to `log4j` with `log4j-1.2-api`
Ran `mvn test` (on a Mac) successfully
Result:
Fixes#11263
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:
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
* Address feedback
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:
It turns out it is quite easy to cause a classloader deadlock in more recent java updates if you cause classloading while you are in native code. Because of this we should just workaround this issue by pre-load all the classes that needs to be accessed in the OnLoad function.
Modifications:
- Preload all classes that would otherwise be loaded by native OnLoad functions.
Result:
Workaround for https://github.com/netty/netty/issues/11209 and https://bugs.openjdk.java.net/browse/JDK-8266310
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 are increasingly running in environments where Unsafe, setAccessible, etc. are not available.
When debug logging is enabled, we log a complete stack trace every time one of these initialisations fail.
Seeing these stack traces can cause people unnecessary concern.
For instance, people might have alerts that are triggered by a stack trace showing up in logs, regardless of its log level.
Modification:
We continue to print debug log messages on the result of our initialisations, but now we only include the full stack trace is _trace_ logging (or FINEST, or equivalent in whatever logging framework is configured) is enabled.
Result:
We now only log these initialisation stack traces when the lowest possible log level is enabled.
Fixes#7817
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:
Alignment handling was broken, and basically turned into a fixed offset into each allocation address regardless of its initial value, instead of ensuring that the allocated address is either aligned or bumped to the nearest alignment offset.
The brokenness of the alignment handling extended so far, that overlapping ByteBuf instances could even be created, as was seen in #11101.
Modification:
Instead of fixing the per-allocation pointer bump, we now ensure that 1) the minimum page size is a whole multiple of the alignment, and 2) the reference memory for each chunk is bumped to the nearest aligned address, and finally 3) ensured that the reservations are whole multiples of the alignment, thus ensuring that the next allocation automatically occurs from an aligned address.
Incidentally, (3) above comes for free because the reservations are in whole pages, and in (1) we ensured that pages are sized in whole multiples of the alignment.
In order to ensure that the memory for a chunk is aligned, we introduce some new PlatformDependent infrastructure.
The PlatformDependent.alignDirectBuffer will produce a slice of the given buffer, and the slice will have an address that is aligned.
This method is plainly available on ByteBuffer in Java 9 onwards, but for pre-9 we have to use Unsafe, which means it can fail and might not be available on all platforms.
Attempts to create a PooledByteBufAllocator that uses alignment, when this is not supported, will throw an exception.
Luckily, I think use of aligned allocations are rare.
Result:
Aligned pooled byte bufs now work correctly, and never have any overlap.
Fixes#11101
Motivation:
Due a regression in fd8c1874b4 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:
When etcResolver/hosts files are parsed, FileInputStream.read(...) is internally called by
- UnixResolverDnsServerAddressStreamProvider#parseEtcResolverSearchDomains
- UnixResolverDnsServerAddressStreamProvider#parseEtcResolverOptions
- HostsFileParser#parse
This will cause the error below when BlockHound is enabled
reactor.blockhound.BlockingOperationError: Blocking call! java.io.FileInputStream#readBytes
at java.io.FileInputStream.readBytes(FileInputStream.java)
at java.io.FileInputStream.read(FileInputStream.java:255)
Modifications:
- Add whitelist entries to BlockHound configuration
- Fix typos in UnixResolverDnsServerAddressStreamProvider
- Add tests
Result:
Fixes#11004
Motivation:
File.createTempFile(String, String)` will create a temporary file in the system temporary directory if the 'java.io.tmpdir'. The permissions on that file utilize the umask. In a majority of cases, this means that the file that java creates has the permissions: `-rw-r--r--`, thus, any other local user on that system can read the contents of that file.
This can be a security concern if any sensitive data is stored in this file.
This was reported by Jonathan Leitschuh <jonathan.leitschuh@gmail.com> as a security problem.
Modifications:
Use Files.createTempFile(...) which will use safe-defaults when running on java 7 and later. If running on java 6 there isnt much we can do, which is fair enough as java 6 shouldnt be considered "safe" anyway.
Result:
Create temporary files with sane permissions by default.
Motivation:
We produced a lot of noise during loading native libraries as we always included the stacktrace if we could not load by one mechanism. We should better just not include the stacktrace in the debugging logging if one mechanism fails. We will log all the stacks anyway when all of the mechanisms fail.
Modifications:
Make logging less aggressive
Result:
Less confusing behaviour for the end-user
Motivation:
Internally UnixResolverDnsServerAddressStreamProvider#parse calls FileInputStream.read(...)
when parsing the etcResolverFiles.
This will cause the error below when BlockHound is enabled
reactor.blockhound.BlockingOperationError: Blocking call! java.io.FileInputStream#readBytes
at java.io.FileInputStream.readBytes(FileInputStream.java)
at java.io.FileInputStream.read(FileInputStream.java:255)
Modifications:
- Add whitelist entry to BlockHound configuration
- Add test
Result:
Fixes#10925
Motivation:
A race detector discovered a data race in GlobalEventExecutor present in
netty 4.1.51.Final:
```
Write of size 4 at 0x0000cea08774 by thread T103:
#0 io.netty.util.internal.DefaultPriorityQueue.poll()Lio/netty/util/internal/PriorityQueueNode; DefaultPriorityQueue.java:113
#1 io.netty.util.internal.DefaultPriorityQueue.poll()Ljava/lang/Object; DefaultPriorityQueue.java:31
#2 java.util.AbstractQueue.remove()Ljava/lang/Object; AbstractQueue.java:113
#3 io.netty.util.concurrent.AbstractScheduledEventExecutor.pollScheduledTask(J)Ljava/lang/Runnable; AbstractScheduledEventExecutor.java:133
#4 io.netty.util.concurrent.GlobalEventExecutor.fetchFromScheduledTaskQueue()V GlobalEventExecutor.java:119
#5 io.netty.util.concurrent.GlobalEventExecutor.takeTask()Ljava/lang/Runnable; GlobalEventExecutor.java:106
#6 io.netty.util.concurrent.GlobalEventExecutor$TaskRunner.run()V GlobalEventExecutor.java:240
#7 io.netty.util.internal.ThreadExecutorMap$2.run()V ThreadExecutorMap.java:74
#8 io.netty.util.concurrent.FastThreadLocalRunnable.run()V FastThreadLocalRunnable.java:30
#9 java.lang.Thread.run()V Thread.java:835
#10 (Generated Stub) <null>
Previous read of size 4 at 0x0000cea08774 by thread T110:
#0 io.netty.util.internal.DefaultPriorityQueue.size()I DefaultPriorityQueue.java:46
#1 io.netty.util.concurrent.GlobalEventExecutor$TaskRunner.run()V GlobalEventExecutor.java:263
#2 io.netty.util.internal.ThreadExecutorMap$2.run()V ThreadExecutorMap.java:74
#3 io.netty.util.concurrent.FastThreadLocalRunnable.run()V FastThreadLocalRunnable.java:30
#4 java.lang.Thread.run()V Thread.java:835
#5 (Generated Stub) <null>
```
The race is legit, but benign. To trigger it requires a TaskRunner to
begin exiting and set 'started' to false, more work to be scheduled
which starts a new TaskRunner, that work then needs to schedule
additional work which modifies 'scheduledTaskQueue', and then the
original TaskRunner checks 'scheduledTaskQueue'. But there is no danger
to this race as it can only produce a false negative in the condition
which causes the code to CAS 'started' which is thread-safe.
Modifications:
Delete problematic references to scheduledTaskQueue. The only way
scheduledTaskQueue could be modified since the last check is if another
TaskRunner is running, in which case the current TaskRunner doesn't
care.
Result:
Data-race free code, and a bit less code to boot.
Motivation:
I did not see any tangible advantage to the padding.
The only other field that was guarded was a rarely changed object reference to a BitSet.
Without the padding, there is also no longer any use of the inheritance hierarchy.
The padding was also using `long`, which would not necessarily prevent the JVM from fitting the aforementioned object reference in an alignment gap.
Modification:
Move all the fields into the InternalThreadLocalMap and deprecate the stuff we'd like to remove.
Result:
Simpler code.
This resolves the discussion in https://github.com/netty/netty/issues/9284
