Commit Graph

7612 Commits

Author SHA1 Message Date
Norman Maurer ea8ff3b4f6 Not use safeRelease(...) but release(...) to release non-readable holders to ensure we not mask errors.
Motivation:

AbstractChannel attempts to "filter" messages which are written [1]. A goal of this process is to copy from heap to direct if necessary. However implementations of this method [2][3] may translate a buffer with 0 readable bytes to EMPTY_BUFFER. This may mask a user error where an empty buffer is written but already released.

Modifications:

Replace safeRelease(...) with release(...) to ensure we propagate reference count issues.

Result:

Fixes [#7383]
2017-12-04 20:39:19 +01:00
Silvio Giebl 6583b5271c Fixed default OpenSsl cipher suites
Motivation:

The default enabled cipher suites of the OpenSsl engine are not set to
SslUtils#DEFAULT_CIPHER_SUITES. Instead all available cipher suites are
enabled. This should happen only as a fallback.

Modifications:

Moved the line in the static initializer in OpenSsl which adds the
SslUtils#DEFAULT_CIPHER_SUITES to the default enabled cipher suites up
before the fallback.

Result:

The default enabled cipher suites of the OpenSsl engine are set to the
available ones of the SslUtils#DEFAULT_CIPHER_SUITES.
The default enabled cipher suites of the OpenSsl engine are only set to
all available cipher suites if no one of the
SslUtils#DEFAULT_CIPHER_SUITES is supported.
2017-11-29 12:23:35 +01:00
Norman Maurer 6507f237d1 Dont use ThreadDeathWatcher to cleanup PoolThreadCache if FastThreadLocalThread with wrapped Runnable is used
Motivation:

We dont need to use the ThreadDeathWatcher if we use a FastThreadLocalThread for which we wrap the Runnable and ensure we call FastThreadLocal.removeAll() once the Runnable completes.

Modifications:

- Dont use a ThreadDeathWatcher if we are sure we will call FastThreadLocal.removeAll()
- Add unit test.

Result:

Less overhead / running theads if you only allocate / deallocate from FastThreadLocalThreads.
2017-11-28 13:43:49 +01:00
Norman Maurer ae4a272e46 Add tests for HttpObjectDecoder related to limits
Motivation:

HttpObjectDecoder will throw a TooLongFrameException when either the max size for the initial line or the header size was exceeed. We have no tests for this.

Modifications:

Add test cases.

Result:

More tests.
2017-11-28 13:41:24 +01:00
Carl Mastrangelo 777b400fd6 Throw FileNotFoundException when connecting to a missing UDS path
Motivation:
Exception handling is nicer when a more specific Exception is thrown

Modification:
Add a static reference for ENOENT, and throw FNFE if it is returned

Result:
More precise exception handling
2017-11-28 13:19:16 +01:00
Tim Ward 8d58e9a3d2 Provide a test for #6548 using the OSGi test suite
Motiviation:

The OSGi Test suite runs without access to sun.misc.Unsafe, and so is a good place to put a test to avoid regressing #6548.

Modification:

Added a test-case that failed before https://github.com/netty/netty/pull/7432.

Result:

Test for fix included.
2017-11-27 19:52:49 +01:00
Norman Maurer 7ceea22236 Guard against NoClassDefFoundError when trying to load Unsafe.
Motivation:

OSGI and other enviroments may not allow to even load Unsafe which will lead to an NoClassDefFoundError when trying to access it. We should guard against this.

Modifications:

Catch NoClassDefFoundError when trying to load Unsafe.

Result:

Be able to use netty with a strict OSGI config.
2017-11-24 20:06:52 +01:00
Soner Kaya 1f8a3a9429 When System property is empty use def value.
Motivation:

When system property is empty, the default value should be used.

Modification:

- Correctly use the default value in all cases
- Add unit tests

Result:

Correct behaviour
2017-11-23 19:46:48 +01:00
Norman Maurer 4375b6da22 Correctly propagate channelInactive even if cleanup throws
Motivation:

Its possible that cleanup() will throw if invalid data is passed into the wrapped EmbeddedChannel. We need to ensure we still call channelInactive(...) in this case.

Modifications:

- Correctly forward Exceptions caused by cleanup()
- Ensure all content is released when cleanup() throws
- Add unit tests

Result:

Correctly handle the case when cleanup() throws.
2017-11-21 11:59:35 +01:00
Norman Maurer 8009c4c8a0 Fix writing of empty LastHttpContent with trailers
Motivation:

4732fabb16 introduced a regression in HttpObjectEncoder which will lead to buffer leak and IllegalStateException when a LastHttpContent with trailers is written.

Modifications:

- Correctly add the buffer to the encoded list.
- Add testcases

Result:

Fixes [#7418]
2017-11-21 07:47:16 +01:00
Scott Mitchell 5ca273814f AbstractByteBuf readSlice bound check bug
Motivation:
AbstractByteBuf#readSlice relied upon the bounds checking of the slice operation in order to detect index out of bounds conditions. However the slice bounds checking operation allows for the slice to go beyond the writer index, and this is out of bounds for a read operation.

Modifications:
- AbstractByteBuf#readSlice and AbstractByteBuf#readRetainedSlice should ensure the desired amount of bytes are readable before taking a slice

Result:
No reading of undefined data in AbstractByteBuf#readSlice and AbstractByteBuf#readRetainedSlice.
2017-11-18 09:07:46 +01:00
Norman Maurer ae7436813a Dont fire an SslHandshakeEvent if the handshake was not started at all.
Motivation:

We should not fire a SslHandshakeEvent if the channel is closed but the handshake was not started.

Modifications:

- Add a variable to SslHandler which tracks if an handshake was started yet or not and depending on this fire the event.
- Add a unit test

Result:

Fixes [#7262].
2017-11-16 20:02:25 +01:00
Scott Mitchell bbf570dc92 Add comments for ApplicationProtocolConfig
Motivation:
The behavior for SelectorFailureBehavior and SelectedListenerFailureBehavior enum values are not clear. Additional comments would clarify the expected behavior.

Modifications:
- Add comments for each value in SelectedListenerFailureBehavior and SelectorFailureBehavior which clarify the expected behavior

Result:
The behavior of SelectedListenerFailureBehavior and SelectorFailureBehavior are more clearly communicated.
2017-11-15 08:02:00 +01:00
Violeta Georgieva 24d12d35b1 Correctly handle 205 Reset Content response with transfer-encoding
Motivation:

According to RFC 7231 the server may choose to:
```
indicate a zero-length payload for the response by including a
Transfer-Encoding header field with a value of chunked and a message
body consisting of a single chunk of zero-length
```
https://tools.ietf.org/html/rfc7231#page-53

In such cases the exception below appears during decoding phase:
```
java.lang.IllegalArgumentException: invalid version format: 0
	at io.netty.handler.codec.http.HttpVersion.<init>(HttpVersion.java:121)
	at io.netty.handler.codec.http.HttpVersion.valueOf(HttpVersion.java:76)
	at io.netty.handler.codec.http.HttpResponseDecoder.createMessage(HttpResponseDecoder.java:118)
	at io.netty.handler.codec.http.HttpObjectDecoder.decode(HttpObjectDecoder.java:219)
```

Modifications:

HttpObjectDecoder.isContentAlwaysEmpty specifies content NOT empty
when 205 Reset Content response

Result:

There is no `IllegalArgumentException: invalid version format: 0`
when handling 205 Reset Content response with transfer-encoding
2017-11-14 08:59:38 +01:00
lutovich 33d7348fda Improved error message in FixedChannelPool
Motivation:

`FixedChannelPool` allows users to configure `acquireTimeoutMillis`
and expects given value to be greater or equal to zero when timeout
action is supplied. However, validation error message said that
value is expected to be greater or equal to one. Code performs
check against zero.

Modifications:

Changed error message to say that value greater or equal to
zero is expected. Added test to check that zero is an acceptable
value.

Result:

Exception with right error message is thrown.
2017-11-13 20:29:21 +01:00
Norman Maurer d83de7f81d Use Parameterized to run SslHandler tests with different SslProviders.
Motivation:

At the moment use loops to run SslHandler tests with different SslProviders which is error-prone and also make it difficult to understand with which provider these failed.

Modifications:

- Move unit tests that should run with multiple SslProviders to extra class.
- Use junit Parameterized to run with different SslProvider combinations

Result:

Easier to understand which SslProvider produced test failures
2017-11-10 07:25:16 -08:00
Norman Maurer 4c525d1e51 CompositeBytebuf.copy() and copy(...) should respect the allocator
Motivation:

When calling CompositeBytebuf.copy() and copy(...) we currently use Unpooled to allocate the buffer. This is not really correct and may produce more GC then needed. We should use the allocator that was used when creating the CompositeByteBuf to allocate the new buffer which may be for example the PooledByteBufAllocator.

Modifications:

- Use alloc() to allocate the new buffer.
- Add tests
- Fix tests that depend on the copy to be backed by an byte-array without checking hasArray() first.

Result:

Fixes [#7393].
2017-11-10 07:17:32 -08:00
Norman Maurer d23feba2fa [maven-release-plugin] prepare for next development iteration 2017-11-09 00:08:30 +00:00
Norman Maurer f571151794 [maven-release-plugin] prepare release netty-4.0.53.Final 2017-11-09 00:05:39 +00:00
Norman Maurer 85faf9e025 Correctly convert empty HttpContent to ByteBuf
Motivation:

93130b172a introduced a regression where we not "converted" an empty HttpContent to ByteBuf and just passed it on in the pipeline. This can lead to the situation that other handlers in the pipeline will see HttpContent instances which is not expected.

Modifications:

- Correctly convert HttpContent to ByteBuf when empty
- Add unit test.

Result:

Handlers in the pipeline will see the expected message type.
2017-11-08 13:52:31 -08:00
Carl Mastrangelo 849147f428 Keep all leak records up to the target amount
Motivation:
When looking for a leak, its nice to be able to request at least a
number of leaks.

Modification:

* Made all leak records up to the target amoutn recorded, and only
  then enable backing off.
* Enable recording more than 32 elements.  Previously the shift
  amount made this impossible.

Result:
Ability to record all accesses.
2017-11-07 19:09:43 -08:00
Norman Maurer 1f18ef8845 Set readPending to false when EOF is detected while issue an read
Motivation:

We need to set readPending to false when we detect a EOF while issue a read as otherwise we may not unregister from the Selector / Epoll / KQueue and so keep on receving wakeups.

The important bit is that we may even get a wakeup for a read event but will still will only be able to read 0 bytes from the socket, so we need to be very careful when we clear the readPending. This can happen because we generally using edge-triggered mode for our native transports and because of the nature of edge-triggered we may schedule an read event just to find out there is nothing left to read atm (because we completely drained the socket on the previous read).

Modifications:

Set readPending to false when EOF is detected.

Result:

Fixes [#7255].
2017-11-06 17:14:12 -08:00
Scott Mitchell 040130dee9 SslHandler leak
Motivation:
SslHandler only supports ByteBuf objects, but will not release objects of other types. SslHandler will also not release objects if its internal state is not correctly setup.

Modifications:
- Release non-ByteBuf objects in write
- Release all objects if the SslHandler queue is not setup

Result:
Less leaks in SslHandler.
2017-11-06 15:33:08 -08:00
Norman Maurer 3422e97ac6 Revert "Set readPending to false when ever a read is done"
This reverts commit 687a2e51cf as it introduced an regression when edge-triggered mode is used which is true for our native transports by default. With 687a2e51cf included it was possible that we set readPending to false by mistake even if we would be interested in read more.
2017-11-06 09:24:14 -08:00
Norman Maurer 4f4d800bbf Correctly handle WebSockets 00 when using HttpClientCodec.
Motivation:

7995afee8f introduced a change that broke special handling of WebSockets 00.

Modifications:

Correctly delegate to super method which has special handling for WebSockets 00.

Result:

Fixes [#7362].
2017-11-03 16:00:50 +01:00
Scott Mitchell 756e1579b3 HttpObjectEncoder and MessageAggregator EMPTY_BUFFER usage
Motivation:
HttpObjectEncoder and MessageAggregator treat buffers that are not readable special. If a buffer is not readable, then an EMPTY_BUFFER is written and the actual buffer is ignored. If the buffer has already been released then this will not be correct as the promise will be completed, but in reality the original content shouldn't have resulted in any write because it was invalid.

Modifications:
- HttpObjectEncoder should retain/write the original buffer instead of using EMPTY_BUFFER
- MessageAggregator should retain/write the original ByteBufHolder instead of using EMPTY_BUFFER

Result:
Invalid write operations which happen to not be readable correctly reflect failed status in the promise, and do not result in any writes to the channel.
2017-11-03 07:38:23 +01:00
Scott Mitchell 5c71b47122 OpenSslEngine support unwrap plaintext greater than 2^14 and avoid
infinite loop

Motivation:
If SslHandler sets jdkCompatibilityMode to false and ReferenceCountedOpenSslEngine sets jdkCompatibilityMode to true there is a chance we will get stuck in an infinite loop if the peer sends a TLS packet with length greater than 2^14 (the maximum length allowed in the TLS 1.2 RFC [1]). However there are legacy implementations which actually send larger TLS payloads than 2^14 (e.g. OpenJDK's SSLSessionImpl [2]) and in this case ReferenceCountedOpenSslEngine will return BUFFER_OVERFLOW in an attempt to notify that a larger buffer is to be used, but if the buffer is already at max size this process will repeat indefinitely.

[1] https://tools.ietf.org/html/rfc5246#section-6.2.1
[2] http://hg.openjdk.java.net/jdk8u/jdk8u/jdk/file/d5a00b1e8f78/src/share/classes/sun/security/ssl/SSLSessionImpl.java#l793

Modifications:
- Support TLS payload sizes greater than 2^14 in ReferenceCountedOpenSslEngine
- ReferenceCountedOpenSslEngine should throw an exception if a
BUFFER_OVERFLOW is impossible to rectify

Result:
No more infinite loop in ReferenceCountedOpenSslEngine due to
BUFFER_OVERFLOW and large TLS payload lengths.
2017-11-02 11:43:00 -07:00
Norman Maurer 26417cf2ad Fix possible NPE in ReferenceCountedOpenSslEngine.rejectRemoteInitiatedRenegotiation()
Motivation:

ReferenceCountedOpenSslEngine.rejectRemoteInitiatedRenegotiation() is called in a finally block to ensure we always check for renegotiation. The problem here is that sometimes we will already shutdown the engine before we call the method which will lead to an NPE in this case as the ssl pointer was already destroyed.

Modifications:

Check that the engine is not destroyed yet before calling SSL.getHandshakeCount(...)

Result:

Fixes [#7353].
2017-11-02 14:29:17 +01:00
Scott Mitchell b5849f5e08 SslHandler dervies jdkCompatibilityMode from SSLEngine
Motivation:
Some SSLEngine implementations (e.g. ReferenceCountedOpenSslContext) support unwrapping/wrapping multiple packets at a time. The SslHandler behaves differently if the SSLEngine supports this feature, but currently requires that the constructor argument between the SSLEngine creation and SslHandler are coordinated. This can be difficult, or require package private access, if extending the SslHandler.

Modifications:
- The SslHandler should inspect the SSLEngine to see if it supports jdkCompatibilityMode instead of relying on getting an extra constructor argument which maybe out of synch with the SSLEngine

Result:
Easier to override SslHandler and have consistent jdkCompatibilityMode between SSLEngine and SslHandler.
2017-11-01 12:01:17 -07:00
Norman Maurer 4a429692b3 Take the architecture into account when loading netty-tcnative
Motivation:

We should ensure we only try to load the netty-tcnative version that was compiled for the architecture we are using.

Modifications:

Include architecture into native lib name.

Result:

Only load native lib if the architecture is supported.
2017-11-01 08:54:29 +01:00
Dmitry Minkovsky 82b7103765 HttpPostMultipartRequestDecoder should decode header field parameters
Motivation:

I am receiving a multipart/form_data upload from a Mailgun webhook. This webhook used to send parts like this:

--74e78d11b0214bdcbc2f86491eeb4902
Content-Disposition: form-data; name="attachment-2"; filename="attached_�айл.txt"
Content-Type: text/plain
Content-Length: 32

This is the content of the file

--74e78d11b0214bdcbc2f86491eeb4902--
but now it posts parts like this:

--74e78d11b0214bdcbc2f86491eeb4902
Content-Disposition: form-data; name="attachment-2"; filename*=utf-8''attached_%D1%84%D0%B0%D0%B9%D0%BB.txt

This is the content of the file

--74e78d11b0214bdcbc2f86491eeb4902--
This new format uses field parameter encoding described in RFC 5987. More about this encoding can be found here.

Netty does not parse this format. The result is the filename is not decoded and the part is not parsed into a FileUpload.

Modification:

Added failing test in HttpPostRequestDecoderTest.java and updated HttpPostMultipartRequestDecoder.java
Refactored to please Netkins
Result:

Fixes:

HttpPostMultipartRequestDecoder identifies the RFC 5987 format and parses it.
Previous functionality is retained.
2017-10-30 15:26:16 +01:00
Piotr Kołaczkowski efcc6c8288 Don't disable HttpObjectDecoder on upgrade from HTTP/1.x to HTTP/1.x over TLS
This change allows to upgrade a plain HTTP 1.x connection to TLS
according to RFC 2817. Switching the transport layer to TLS should be
possible without removing HttpClientCodec from the pipeline,
because HTTP/1.x layer of the protocol remains untouched by the switch
and the HttpClientCodec state must be retained for proper
handling the remainder of the response message,
per RFC 2817 requirement in point 3.3:

  Once the TLS handshake completes successfully, the server MUST
  continue with the response to the original request.

After this commit, the upgrade can be established by simply
inserting an SslHandler at the front of the pipeline after receiving
101 SWITCHING PROTOCOLS response, exactly as described in SslHander
documentation.

Modifications:
- Don't set HttpObjectDecoder into UPGRADED state if
  101 SWITCHING_PROTOCOLS response contains HTTP/1.0 or HTTP/1.1 in
  the protocol stack described by the Upgrade header.
- Skip pairing comparison for 101 SWITCHING_PROTOCOLS, similar
  to 100 CONTINUE, since 101 is not the final response to the original
  request and the final response is expected after TLS handshake.

Fixes #7293.
2017-10-29 13:37:19 +01:00
Scott Mitchell 687a2e51cf Set readPending to false when ever a read is done
Motivation:
readPending is currently only set to false if data is delivered to the application, however this may result in duplicate events being received from the selector in the event that the socket was closed.

Modifications:
- We should set readPending to false before each read attempt for all
transports besides NIO.
- Based upon the Javadocs it is possible that NIO may have spurious
wakeups [1]. In this case we should be more cautious and only set
readPending to false if data was actually read.

[1] https://docs.oracle.com/javase/7/docs/api/java/nio/channels/SelectionKey.html
That a selection key's ready set indicates that its channel is ready for some operation category is a hint, but not a guarantee, that an operation in such a category may be performed by a thread without causing the thread to block.

Result:
Notification from the selector (or simulated events from kqueue/epoll ET) in the event of socket closure.
Fixes #7255
2017-10-29 13:19:44 +01:00
Trask Stalnaker b12a1e3fa6 Support running Netty in bootstrap class loader
Motivation:

Fix NullPointerExceptions that occur when running netty-tcnative inside the bootstrap class loader.

Modifications:

- Replace loader.getResource(...) with ClassLoader.getSystemResource(...) when loader is null.
- Replace loader.loadClass(...) with Class.forName(..., false, loader) which works when loader is both null and non-null.

Result:

Support running native libs in bootstrap class loader
2017-10-29 13:16:31 +01:00
Carl Mastrangelo e5d4e28e46 Use WeakReferences for Resource Leaks
Motivation:
Phantom references are for cleaning up resources that were
forgotten, which means they keep their referent alive.   This
means garbage is kept around until the refqueue is drained, rather
than when the reference is unreachable.

Modification:
Use Weak References instead of Phantoms

Result:
More punctual leak detection.
2017-10-24 19:22:36 +02:00
Ned Twigg ac8f66bb7b Added QueryStringDecoder.rawPath() and rawQuery()
Motivation:

Before this commit, it is impossible to access the path component of the
URI before it has been decoded.  This makes it impossible to distinguish
between the following URIs:

/user/title?key=value
/user%2Ftitle?key=value

The user could already access the raw uri value, but they had to calculate
pathEndIdx themselves, even though it might already be cached inside
QueryStringDecoder.

Result:

The user can easily and efficiently access the undecoded path and query.
2017-10-24 09:32:21 +02:00
Norman Maurer aa3b5c0055 SslHandler.setHandshakeTimeout*(...) should also been enforced on the server side.
Motivation:

We should also enforce the handshake timeout on the server-side to allow closing connections which will not finish the handshake in an expected amount of time.

Modifications:

- Enforce the timeout on the server and client side
- Add unit test.

Result:

Fixes [#7230].
2017-10-23 19:33:21 +02:00
Nikolay Fedorovskikh ea68fcfc46 Correct filling an `origin` header for WS client
Motivation:
An `origin`/`sec-websocket-origin` header value in websocket client is filling incorrect in some cases:
- Hostname is not converting to lower-case as prescribed by RFC 6354 (see [1]).
- Selecting a `http` scheme when source URI has `wss`/`https` scheme and non-standard port.

Modifications:
- Convert uri-host to lower-case.
- Use a `https` scheme if source URI scheme is `wss`/`https`, or if source scheme is null and port == 443.

Result:
Correct filling an `origin` header for WS client.

[1] https://tools.ietf.org/html/rfc6454#section-4
2017-10-23 13:26:06 +02:00
Idel Pivnitskiy eb548b8667 Update links for actual Protobuf repo and documentation
Motivation:

Use actual links to new locations of Protobuf repo and documentation to
avoid problems when redirect will not work.

Modification:

Links in comments and all/pom.xml

Result:

Correct links to Protobuf resources
2017-10-23 08:48:04 +02:00
Norman Maurer 86e8b72958 Add supresswarnings to cleanup 16b1dbdf92.
Motivation:

We should add @SupressWarnings

Modifications:

Add annotations.

Result:

Less warnings
2017-10-22 18:17:08 +02:00
Idel Pivnitskiy a1328419fd Make methods 'static' where it possible
Motivation:

Even if it's a super micro-optimization (most JVM could optimize such
 cases in runtime), in theory (and according to some perf tests) it
 may help a bit. It also makes a code more clear and allows you to
 access such methods in the test scope directly, without instance of
 the class.

Modifications:

Add 'static' modifier for all methods, where it possible. Mostly in
test scope.

Result:

Cleaner code with proper 'static' modifiers.
2017-10-21 15:02:10 +02:00
Nikolay Fedorovskikh 134ee2dbba Minor improvements in ByteBufOutputStream
Motivation:
In the `ByteBufOutputStream` we can use an appropriate methods of `ByteBuf`
to reduce calls of virtual methods and do not copying converting logic.

Modifications:
- Use an appropriate methods of `ByteBuf`
- Remove redundant conversions (int -> byte, int -> char).
- Use `ByteBuf#writeCharSequence` in the `writeBytes(String)'.

Result:
Less code duplication. A `writeBytes(String)` method is faster.
No unnecessary conversions. More consistent and cleaner code.
2017-10-21 14:50:35 +02:00
Idel Pivnitskiy e9fe258081 Add missed 'serialVersionUID' field for Serializable classes
Motivation:

Without a 'serialVersionUID' field, any change to a class will make
previously serialized versions unreadable.

Modifications:

Add missed 'serialVersionUID' field for all Serializable
classes.

Result:

Proper deserialization of previously serialized objects.
2017-10-21 14:42:09 +02:00
Carl Mastrangelo 74adbcd25d Motivation: Resource Leak Detector (RLD) tries to helpfully indicate where an object was last accessed and report the accesses in the case the object was not cleaned up. It handles lightly used objects well, but drops all but the last few accesses.
Configuring this is tough because there is split between highly shared (and accessed) objects and lightly accessed objects.

Modification:
There are a number of changes here.  In relative order of importance:

API / Functionality changes:
* Max records and max sample records are gone.  Only "target" records, the number of records tries to retain is exposed.
* Records are sampled based on the number of already stored records.  The likelihood of recording a new sample is `2^(-n)`, where `n` is the number of currently stored elements.
* Records are stored in a concurrent stack structure rather than a list.  This avoids a head and tail.  Since the stack is only read once, there is no need to maintain head and tail pointers
* The properties of this imply that the very first and very last access are always recorded.  When deciding to sample, the top element is replaced rather than pushed.
* Samples that happen between the first and last accesses now have a chance of being recorded.  Previously only the final few were kept.
* Sampling is no longer deterministic.  Previously, a deterministic access pattern meant that you could conceivably always miss some access points.
* Sampling has a linear ramp for low values and and exponentially backs off roughly equal to 2^n.  This means that for 1,000,000 accesses, about 20 will actually be kept.  I have an elegant proof for this which is too large to fit in this commit message.

Code changes:
* All locks are gone.  Because sampling rarely needs to do a write, there is almost 0 contention.  The dropped records counter is slightly contentious, but this could be removed or changed to a LongAdder.  This was not done because of memory concerns.
* Stack trace exclusion is done outside of RLD.  Classes can opt to remove some of their methods.
* Stack trace exclusion is faster, since it uses String.equals, often getting a pointer compare due to interning.  Previously it used contains()
* Leak printing is outputted fairly differently.  I tried to preserve as much of the original formatting as possible, but some things didn't make sense to keep.

Result:
More useful leak reporting.

Faster:
```
Before:
Benchmark                                           (recordTimes)   Mode  Cnt       Score      Error  Units
ResourceLeakDetectorRecordBenchmark.record                      8  thrpt   20  136293.404 ± 7669.454  ops/s
ResourceLeakDetectorRecordBenchmark.record                     16  thrpt   20   72805.720 ± 3710.864  ops/s
ResourceLeakDetectorRecordBenchmark.recordWithHint              8  thrpt   20  139131.215 ± 4882.751  ops/s
ResourceLeakDetectorRecordBenchmark.recordWithHint             16  thrpt   20   74146.313 ± 4999.246  ops/s

After:
Benchmark                                           (recordTimes)   Mode  Cnt       Score      Error  Units
ResourceLeakDetectorRecordBenchmark.record                      8  thrpt   20  155281.969 ± 5301.399  ops/s
ResourceLeakDetectorRecordBenchmark.record                     16  thrpt   20   77866.239 ± 3821.054  ops/s
ResourceLeakDetectorRecordBenchmark.recordWithHint              8  thrpt   20  153360.036 ± 8611.353  ops/s
ResourceLeakDetectorRecordBenchmark.recordWithHint             16  thrpt   20   78670.804 ± 2399.149  ops/s
```
2017-10-19 12:42:35 -07:00
Jason Tedor b5b53daf91 Do not treat errors as decoder exception (redux)
Motivation: Today when Netty encounters a general error while decoding
it treats this as a decoder exception. However, for fatal causes this
should not be treated as such, instead the fatal error should be carried
up the stack without the callee having to unwind causes. This was
probably done for byte to byte message decoder but is now done for all
decoders.

Modifications: Instead of translating any error to a decoder exception,
we let those unwind out the stack (note that finally blocks still
execute) except in places where an event needs to fire where we fire
with the error instead of wrapping in a decoder exception.

Result: Fatal errors will not be treated as innocent decoder exceptions.
2017-10-07 20:18:02 +02:00
Johno Crawford 3637bb8ce3 Update JCTools version
Motivation:

JCTools 2.1.0 contains package-info.class which doesn't load on JRE 6.

Modifications:

Change pom JCTools version.

Result:

Really fixes https://github.com/netty/netty/issues/7117
2017-10-07 18:10:52 +02:00
Johno Crawford 78a0699ef8 Upgrade dependencies to versions which use ASM 6.0.0+
Motivation:

We need to upgrade our dependencies to versions which use ASM 6.0.0+ to support compiling on java9.

Modifications:

Update animal-sniffer-maven-plugin and maven-shade-plugin.

Result:

Fixes https://github.com/netty/netty/issues/6100
2017-10-07 14:47:23 +02:00
Jason Tedor 4ad6ed17eb Do not treat errors as decoder exception
Motivation: Today when Netty encounters a general error while decoding
it treats this as a decoder exception. However, for fatal causes this
should not be treated as such, instead the fatal error should be carried
up the stack without the callee having to unwind causes.

Modifications: Instead of translating any error to a decoder exception,
we let those unwind out the stack (note that finally blocks still
execute).

Result: Fatal errors will not be treated as innocent decoder exceptions.
2017-10-05 08:27:01 +02:00
Carl Mastrangelo 4a572e06d6 Propagate all exceptions when loading native code
Motivation:
There are 2 motivations, the first depends on the second:

Loading Netty Epoll statically stopped working in 4.1.16, due to
`Native` always loading the arch specific shared object.  In a
static binary, there is no arch specific SO.

Second, there are a ton of exceptions that can happen when loading
a native library.  When loading native code, Netty tries a bunch of
different paths but a failure in any given may not be fatal.

Additionally: turning on debug logging is not always feasible so
exceptions get silently swallowed.

Modifications:

* Change Epoll and Kqueue to try the static load second
* Modify NativeLibraryLoader to record all the locations where
  exceptions occur.
* Attempt to use `addSuppressed` from Java 7 if available.

Alternatives Considered:

An alternative would be to record log messages at each failure.  If
all load attempts fail, the log messages are printed as warning,
else as debug. The problem with this is there is no `LogRecord` to
create like in java.util.logging.  Buffering the args to
logger.log() at the end of the method loses the call site, and
changes the order of events to be confusing.

Another alternative is to teach NativeLibraryLoader about loading
the SO first, and then the static version.  This would consolidate
the code fore Epoll, Kqueue, and TCNative.   I think this is the
long term better option, but this PR is changing a lot already.
Someone else can take a crack at it later

Results:
Epoll Still Loads and easier debugging.
2017-10-04 08:49:59 +02:00
Carl Mastrangelo 45a6bd399e Optimistically update ref counts
Motivation:
Highly retained and released objects have contention on their ref
count.  Currently, the ref count is updated using compareAndSet
with care to make sure the count doesn't overflow, double free, or
revive the object.

Profiling has shown that a non trivial (~1%) of CPU time on gRPC
latency benchmarks is from the ref count updating.

Modification:
Rather than pessimistically assuming the ref count will be invalid,
optimistically update it assuming it will be.  If the update was
wrong, then use the slow path to revert the change and throw an
execption.  Most of the time, the ref counts are correct.

This changes from using compareAndSet to getAndAdd, which emits a
different CPU instruction on x86 (CMPXCHG to XADD).  Because the
CPU knows it will modifiy the memory, it can avoid contention.

On a highly contended machine, this can be about 2x faster.

There is a downside to the new approach.  The ref counters can
temporarily enter invalid states if over retained or over released.
The code does handle these overflow and underflow scenarios, but it
is possible that another concurrent access may push the failure to
a different location.  For example:

Time 1 Thread 1: obj.retain(INT_MAX - 1)
Time 2 Thread 1: obj.retain(2)
Time 2 Thread 2: obj.retain(1)

Previously Thread 2 would always succeed and Thread 1 would always
fail on the second access.  Now, thread 2 could fail while thread 1
is rolling back its change.

====

There are a few reasons why I think this is okay:

1. Buggy code is going to have bugs.  An exception _is_ going to be
   thrown.  This just causes the other threads to notice the state
   is messed up and stop early.
2. If high retention counts are a use case, then ref count should
   be a long rather than an int.
3. The critical section is greatly reduced compared to the previous
   version, so the likelihood of this happening is lower
4. On error, the code always rollsback the change atomically, so
   there is no possibility of corruption.

Result:
Faster refcounting

```
BEFORE:

Benchmark                                                                                             (delay)    Mode      Cnt         Score    Error  Units
AbstractReferenceCountedByteBufBenchmark.retainRelease_contended                                            1  sample  2901361       804.579 ±  1.835  ns/op
AbstractReferenceCountedByteBufBenchmark.retainRelease_contended                                           10  sample  3038729       785.376 ± 16.471  ns/op
AbstractReferenceCountedByteBufBenchmark.retainRelease_contended                                          100  sample  2899401       817.392 ±  6.668  ns/op
AbstractReferenceCountedByteBufBenchmark.retainRelease_contended                                         1000  sample  3650566      2077.700 ±  0.600  ns/op
AbstractReferenceCountedByteBufBenchmark.retainRelease_contended                                        10000  sample  3005467     19949.334 ±  4.243  ns/op
AbstractReferenceCountedByteBufBenchmark.retainRelease_uncontended                                          1  sample   456091        48.610 ±  1.162  ns/op
AbstractReferenceCountedByteBufBenchmark.retainRelease_uncontended                                         10  sample   732051        62.599 ±  0.815  ns/op
AbstractReferenceCountedByteBufBenchmark.retainRelease_uncontended                                        100  sample   778925       228.629 ±  1.205  ns/op
AbstractReferenceCountedByteBufBenchmark.retainRelease_uncontended                                       1000  sample   633682      2002.987 ±  2.856  ns/op
AbstractReferenceCountedByteBufBenchmark.retainRelease_uncontended                                      10000  sample   506442     19735.345 ± 12.312  ns/op

AFTER:
Benchmark                                                                                             (delay)    Mode      Cnt         Score    Error  Units
AbstractReferenceCountedByteBufBenchmark.retainRelease_contended                                            1  sample  3761980       383.436 ±  1.315  ns/op
AbstractReferenceCountedByteBufBenchmark.retainRelease_contended                                           10  sample  3667304       474.429 ±  1.101  ns/op
AbstractReferenceCountedByteBufBenchmark.retainRelease_contended                                          100  sample  3039374       479.267 ±  0.435  ns/op
AbstractReferenceCountedByteBufBenchmark.retainRelease_contended                                         1000  sample  3709210      2044.603 ±  0.989  ns/op
AbstractReferenceCountedByteBufBenchmark.retainRelease_contended                                        10000  sample  3011591     19904.227 ± 18.025  ns/op
AbstractReferenceCountedByteBufBenchmark.retainRelease_uncontended                                          1  sample   494975        52.269 ±  8.345  ns/op
AbstractReferenceCountedByteBufBenchmark.retainRelease_uncontended                                         10  sample   771094        62.290 ±  0.795  ns/op
AbstractReferenceCountedByteBufBenchmark.retainRelease_uncontended                                        100  sample   763230       235.044 ±  1.552  ns/op
AbstractReferenceCountedByteBufBenchmark.retainRelease_uncontended                                       1000  sample   634037      2006.578 ±  3.574  ns/op
AbstractReferenceCountedByteBufBenchmark.retainRelease_uncontended                                      10000  sample   506284     19742.605 ± 13.729  ns/op

```
2017-10-04 08:31:27 +02:00