Commit Graph

6759 Commits

Author SHA1 Message Date
Scott Mitchell
cbc38e938a UNSAFE.throwException null arg crashes JVM
Motivation:
It has been observed that passing a null argument to Unsafe.throwException can crash the JVM.

Modifications:
- PlatformUnsafe0.throwException should honor http://docs.oracle.com/javase/specs/jls/se8/html/jls-14.html#jls-14.18 and throw a NPE

Result:
No risk of JVM crashing for null argument.
Fixes https://github.com/netty/netty/issues/4131
2015-08-26 23:50:51 -07:00
Trustin Lee
73f472b65d Fix DNS lookup hang / Remove Comodo Secure DNS
Motivation:

- DNS lookup sometimes hang because it does not call
  tryToFinishResolve()
- Comodo Secure DNS handles negative lookup incorrectly.

Modifications:

- Add missing tryToFinishResolve()
- Remove Comodo Secure DNS servers from the list

Result

- DNS lookup does not hang on non-existent domain names
- More reliable DnsNameResolverTest
2015-08-27 12:52:29 +09:00
Trustin Lee
75efe016d6 Add more public DNS servers to DnsNameResolverTest
so that we have more chance of passing the test when some DNS servers
are unavailable or throtlling us.
2015-08-27 12:25:38 +09:00
Scott Mitchell
14dc571956 Http2ConnectionHandler channelInactive sequencing
Motivation:
ByteToMessageDecoder may call decode after channelInactive is called. This will lead to a NPE.

Modifications:
- Call super.channelInactive() before we process the event in Http2ConnectionHandler

Result:
No more NPE in decode.
2015-08-26 15:31:47 -07:00
Scott Mitchell
e280251b15 Revert "Add PooledSlicedByteBuf and PooledDuplicatedByteBuf"
Motivation:
Currently the "derived" buffer will only ever be recycled if the release call is made on the "derived" object, and the "wrapped" buffer ends up being "fully released" (aka refcount goes to 0). From my experience this is not the common use case and thus the "derived" buffers will not be recycled.

Modifications:
- revert https://github.com/netty/netty/pull/3788

Result:
Less complexity, and less code to create new objects in majority of cases.
2015-08-26 13:24:44 -07:00
Trustin Lee
719d1dbad1 Replace infinite Iterable/Iterator with dedicated types
Related: #4065

Motivation:

DnsNameResolver was using a special Iterable/Iterator implementation
that yields an infinite stream of DNS server addresses. However, this
seems to cause confusion.

Modifications:

- Make DnsServerAddresses an abstract class with an abstract stream()
  method that returns DnsServerAddressStream
- Add DnsServerAddressStream that yields DNS server address infinitely
- Remove DnsServerResolver(Group) constructors that accept only a single
  server address, which wasn't very useful in practice
- Extract the DnsServerAddresses implementations to top level
- DnsServerAddresses.defaultAddresses() now returns DnsServerAddresses.
  - Add DnsServerAddresses.defaultAddressList() instead

Result:

Less confusion and more explicitness
2015-08-26 17:38:43 +09:00
Scott Mitchell
99d6a97b4a HTTP to HTTP/2 translation errors (round 2)
Motivation:
Commit 0d8ce23c83 failed to fix the Host header processing. Host is not a URI but is instead defined in https://tools.ietf.org/html/rfc3986#section-3.2.2 as host        = IP-literal / IPv4address / reg-name

Modifications:
- Host should not be treated as a URI.
- We should be more explicit about required fields, and unexpected input by throwing exceptions.

Result:
Translation from HTTP/1.x to HTTP/2 is more correct.
2015-08-24 20:37:58 -07:00
Scott Mitchell
6046adef2b HttpRequestEncoder consistency with master branch
Motivation:
The HttpRequestEncoder.encodeInitialLine can now be consistent with the master branch after 85c79dbbe4

Modifications:
- Use the AsciiString and ByteBufUtil.copy methods

Result:
Consistent behavior/code between 4.1 and master branches.
2015-08-21 11:49:46 -07:00
Scott Mitchell
85c79dbbe4 HTTP to HTTP/2 tranlation errors
Motivation:
HttpUtil.toHttp2Headers is currently not translating HTTP request headers to HTTP/2 request headers correctly.  The path, scheme, and authority are tranlation process are not respecting the HTTP/2 RFC https://tools.ietf.org/html/rfc7540#section-8.1.2.3 and HTTP RFC https://tools.ietf.org/html/rfc7230#section-5.3.

Modifications:
- path, scheme, authority must be set according to rules defined in https://tools.ietf.org/html/rfc7540#section-8.1.2.3
- HTTP/1.x URIs must be handled as defined in https://tools.ietf.org/html/rfc7230#section-5.3

Result:
More correct translation from HTTP/1.x requests to HTTP/2 requests.
2015-08-21 11:33:10 -07:00
Scott Mitchell
a55df36ae4 ce6931e0e5 buffer leak in EpollSocketChannelTest
Motivation:
ce6931e0e5 introduced a buffer leak in EpollSocketChannelTest.

Modifications:
- Fix buffer leak

Result:
No more buffer leak.
2015-08-21 11:29:15 -07:00
Scott Mitchell
ac9ae14bd9 HTTP/2 SimpleChannelPromiseAggregator failure condition
Motivation:
If a SimpleChannelPromiseAggregator is failed before any new promises are generated, the failure is not propegated through to the aggregated promise.

Modifications:
- Failures should be allowed to occur even if no new promises have been generated

Result:
Failures are always allowed.
2015-08-21 11:26:31 -07:00
Norman Maurer
1fefe9affb [#4087] Correctly forward bytes when remove codec and handle channelInactive / channelReadComplete(...)
Motivation:

We missed to correctly implement the handlerRemoved(...) / channelInactive(...) and channelReadComplete(...) method, this leaded to multiple problems:

 - Missed to forward bytes when the codec is removed from the pipeline
 - Missed to call decodeLast(...) once the Channel goes in active
 - No correct handling of channelReadComplete that could lead to grow of cumulation buffer.

Modifications:

- Correctly implement methods and forward to the internal ByteToMessageDecoder
- Add unit test.

Result:

Correct behaviour
2015-08-21 18:26:32 +02:00
Norman Maurer
e59ae12b42 [#4079] Fix IllegalStateException when HttpContentEncoder is used and 100 Continue response is used.
Motivation:

Whe a 100 Continue response was written an IllegalStateException was produced as soon as the user wrote the following response. This regression was introduced by 41b0080fcc.

Modifications:

- Special handle 100 Continue responses
- Added unit tests

Result:

Fixed regression.
2015-08-21 08:02:43 +02:00
Norman Maurer
50b9768928 [#4095] Correctly handle Upgrade responses with special handling of Hixie 76
Motivation:

Hixie 76 needs special handling compared to other connection upgrade responses. Our detection code of non websocket responses did actually always use the special handling that only should be used for Hixie 76 responses.

Modifications:

Correctly detect connection upgrade responses which are not for websockets.

Result:

Be able to upgrade connections for other protocols then websockets.
2015-08-21 07:28:05 +02:00
Norman Maurer
4feafc4a52 Consistent naming style for enum
Motivation:

We should use camel-case for Enums.

Modifications:

Rename enums to use camel-case.

Result:

Consistent naming
2015-08-21 07:18:19 +02:00
fei wong reed
2eb6809070 Correct OSGi manifests in source jars
Related: #3886

Motivation:

We were including OSGi manifests in sources/javadoc JARs, and OSGi
container treats them as correct dependencies when resolving from OBR
repository, which is incorrect. Runtime fails with non-descriptive
ClassNotFoundException as a result.

Modifications:

- Do not include the OSGi manifests in sources/javadoc JARs
- Include Eclipse-related manifest entries in sources/javadoc JARs

Result:

Better OSGi compatibility
2015-08-21 12:58:43 +09:00
Scott Mitchell
08477eaf03 HTTP/2 Graceful Shutdown Timeout
Motivation:
If any streams are still active the graceful shutdown code will wait until they are all closed before the connection is closed. In some situations this event may never occur, and thus a timeout should be supported so the socket can be closed even if all streams haven't been closed.

Modifications:
- Add a configurable timeout for when the graceful shutdown process is attempted.
- Update unit tests to be faster, and use this graceful timeout

Result:
Local endpoint can protect from local or remote issues which prevent the channel from being closed during the graceful shutdown process.
2015-08-20 13:27:18 -07:00
Scott Mitchell
ce6931e0e5 EPOLL exception processing feedback loop
Motivation:
Commit cf171ff525 changed the way read operations were done. This change introduced a feedback loop between fireException and epollInReady.

Modifications:
- All EPOLL*Channel* classes should not call fireException and also continue to read. Instead a read operation should be executed on the eventloop (if the channel's input is not closed, and other conditions are satisfied)

Result:
Exception processing and channelRead will not be in a feedback loop.
Fixes https://github.com/netty/netty/issues/4091
2015-08-20 13:11:02 -07:00
Scott Mitchell
559f1b110a HttpScheme class
Motivation:
The HTTP schemes defined by https://tools.ietf.org/html/rfc7230 don't have a common representation in Netty.

Modifications:
- Add a class to represent HttpScheme

Result:
The HTTP Scheme is now defined in 1 common location.
2015-08-20 09:59:23 -07:00
Scott Mitchell
c2d5a53104 HttpUtil class for Http specific utilities
Motivation:
The HTTP specification defines specific request-targets in https://tools.ietf.org/html/rfc7230#section-5.3. Netty does not have a way to distinguish between these differnt types, and there is currently no obvious location where these types of methods would live.

Modifications:
- Add methods to distinguish request-targets as defined in https://tools.ietf.org/html/rfc7230#section-5.3

Result:
Common utitlity methods exist to inpsect request-targets.
2015-08-20 09:57:47 -07:00
Scott Mitchell
9bc322a6a8 StringUtil not closing Formatter
Motivation:
The StringUtil class creates a Formatter object, but does not close it. There are also a 2 utility methods which would be generally useful.

Modifications:
- Close the Formatter
- Add length and isNullOrEmpty

Result:
No more resource leaks. Additional utility methods.
2015-08-20 09:44:31 -07:00
Scott Mitchell
34dfa7a2d8 DefaultHttp2ConnectionEncoder private constructors on inner classes
Motivation:
DefaultHttp2ConnectionEncoder.FlowControlledHeaders and DefaultHttp2ConnectionEncoder.FlowControlledData have private constructors which may result in static factory methods being generated to construct instances of these classes.

Modifications:
- Make constructors public for these private classes

Result:
Accessor for inner class constructor more correct and no possibiliy of synthetic method generation.
2015-08-19 13:30:24 -07:00
Scott Mitchell
a45e844395 HttpResponseStatus reasonPhrase equals usage
Motivation:
HttpResponseStatus.reasonPhrase returns an AsciiString, but was compared using equals to a String. Other usages of the reasonPhrase also use the toString() method when not necessary.

Modifications:
- Use the contentEquals method

Result:
Correct comparison, and no toString() when not needed.
2015-08-19 13:28:44 -07:00
Vineet Garg
052a171a52 Fixes infinite loop during handshake in SslHandler in Android devices
Motivation:

On Android devices with version less than Lollipop, HarmonyJSSE is used for SSL. After completion of handshake, handshake status is NOT_HANDSHAKING instead of FINISHED. Also encrypting empty buffer after handshake should cause underflow exception and produce 0 bytes, but here it happily encrypts it causing for loop to never break

Modification:

Since 0 bytes should only be consumed in handshake process. Added a condition to break loop when 0 bytes are consumed and handshake status is NOT_HANDSHAKING

Result:

Sucessful ssl handshake on Android devices, no infinite loop now
2015-08-19 22:12:52 +02:00
Trustin Lee
ad0b7ca56d Add a test case for DNS resolver cache for negative loopups
Related issue: #4065
2015-08-18 18:44:51 +09:00
Trustin Lee
1856ab3a35 Make DnsNameResolverTest.testQueryMx() more robust
Motivation:

DNS servers seem to reply with ServFail(2) response code when it is
busy.

Modifications:

- Retry when response code is ServFail instead of failing the test
- Try all DNS servers instead of retrying twice only

Result:

testQueryMx() is less likely to fail due to public DNS server problems
2015-08-18 18:26:11 +09:00
Trustin Lee
fdfe3149ba Provide more control over DnsNameResolver.query() / Add NameResolver.resolveAll()
Related issues:
- #3971
- #3973
- #3976
- #4035

Motivation:

1. Previously, DnsNameResolver.query() retried the request query by its
own. It prevents a user from deciding when to retry or stop. It is also
impossible to get the response object whose code is not NOERROR.

2. NameResolver does not have an operation that resolves a host name
into multiple addresses, like InetAddress.getAllByName()

Modifications:

- Changes related with DnsNameResolver.query()
  - Make query() not retry
    - Move the retry logic to DnsNameResolver.resolve() instead.
  - Make query() fail the promise only when I/O error occurred or it
    failed to get a response
  - Add DnsNameResolverException and use it when query() fails so that
    the resolver can give more information about the failure
  - query() does not cache anymore.

- Changes related with NameResolver.resolveAll()
  - Add NameResolver.resolveAll()
  - Add SimpleNameResolver.doResolveAll()

- Changes related with DnsNameResolver.resolve() and resolveAll()
  - Make DnsNameResolveContext abstract so that DnsNameResolver can
    decide to get single or multiple addresses from it
  - Re-implement cache so that the cache works for resolve() and
    resolveAll()
  - Add 'traceEnabled' property to enable/disable trace information

- Miscellaneous changes
  - Use ObjectUtil.checkNotNull() wherever possible
  - Add InternetProtocolFamily.addressType() to remove repetitive
    switch-case blocks in DnsNameResolver(Context)
  - Do not raise an exception when decoding a truncated DNS response

Result:

- Full control over query()
- A user can now retrieve all addresses via (Dns)NameResolver.resolveAll()
- DNS cache works only for resolve() and resolveAll() now.
2015-08-18 17:40:13 +09:00
Trustin Lee
fd9ca8bbb3 Fix compilation errors 2015-08-18 12:42:33 +09:00
Ivan Bahdanau
08b73bf914 Unhealthy channel is not offered back to the pool.
Motivation:
When releasing unhealthy channel back to a pool we don't have to offer it since on acquire it will be discarded anyways.
 Also checking healthiness at release is a good idea so we don't end up having tons of unhealthy channels in the pool(unless they became unhealthy after being offered)

Modifications:
private SimpleChannelPool.offerIfHealthy() method added that is called from SimpleChannelPool.doReleaseChannel(). SimpleChannelPool.offerIfHealthy() offers channel back to pool only if channel is healthy.
Otherwise it throws setFailure exception to the promise.

 Result:
The pool is now much cleaner and not spammed with unhealthy channels.

Added ability to choose if channel health has to be validated on release by passing boolean flag.

Motivation:
Depending on performance preferences and individual use cases sometimes we would like to be able force health check of a channel at release time and do not offer it back to the pool. Other times we would want to just release channel and offer it back to the pool and check health only when we try to acquire that channel from the pool. See more details here: https://github.com/netty/netty/issues/4077#issuecomment-130461684

Modifications:
Future<Void> release(Channel channel, Promise<Void> promise, boolean offerHealthyOnly);
The offerHealthyOnly boolean flag allows developers to choose whether to do channel validation before offering it back to pool or not.
Appropriate modifications made to hierarchy of implementations of ChannelPool. offerHealthyOnly=true will force channel health to be checked before offering back to pool. offerHealthyOnly=false  will ignore channel health check and will just try just offer it back to the pool
 offerHealthyOnly=true by default.

Result:
Channel health check before offer back to pool is controlled by a flag now.

Code changed to satisfy checkstyle requirements.

Motivation:
Code needs to satisfy checkstyle requirements.

Modifications:
 SimpleChannelPool.java:279 line split to be less then 120 characters.
 SimpleChannelPool.java:280:31 space added after '{'
 SimpleChannelPool.java:282:17 space added after '{'
 SimpleChannelPoolTest.java:198 - extra white space line removed.

Result:
Code satisfies checkstyle requirements.

 offerHealthyOnly is passed as a constructor parameter now.

Motivation:
Instead of passing offerHealthyOnly as a method parameter it is better to pass it in as SimpleChannelPool or FixedChannelPool constructor.

Modifications:
 Redundant release method that takes offerHealthyOnly removed from ChannelPool.
 offerHealthyOnly parameter added to constructor for FixedChannelPool and SimpleChannelPool.

Result:
SimpleChannelPool and FixedChannelPool are now take offerHealthyOnly as a constructor parameter. Default behavior is: offerHealthyOnly=true.

Code changed to satisfy checkstyle requirements.

Motivation:
Code needs to satisfy checkstyle requirements.

Modifications:
 SimpleChannelPool.java:84: line made to be no longer then 120 characters.
 SimpleChannelPool.java:237: extra white space line removed.

Result:
Code satisfies checkstyle requirements.

Tests do not need to be too  copled to the code. Exception message should not be validated

Motivation:
We don't need our tests to be too coupled to the code. Exception type validation in tests is just good enough.

Modifications:
Exception validation message removed from SimpleChannelPoolTest.testUnhealthyChannelIsNotOffered() test.

Result:
The SimpleChannelPoolTest test is less coupled to the code now.

Stack trace set to empty for UNHEALTHY_NON_OFFERED_TO_POOL.

Motivation:
We don't need stack trace for UNHEALTHY_NON_OFFERED_TO_POOL.

Modifications:
Added  UNHEALTHY_NON_OFFERED_TO_POOL.setStackTrace(EmptyArrays.EMPTY_STACK_TRACE) to static init block.

Result:
UNHEALTHY_NON_OFFERED_TO_POOL's stack trace set to empty.

Minor code re-factorings.

Motivation:
For better code readability we need to apply several minor code re-factorings.

Modifications:
javadocs true -> {@code true}
offerHealthyOnly variable name changed to releaseHeathCheck
<p/> -> <p> in javadocs
offerHealthyOnly removed from doReleaseChannel as it not needed there.

Result:
Code quality is improved.

Code changed to satisfy checkstyle requirements.

Motivation:
Code needs to satisfy checkstyle requirements.

Modifications:
SimpleChannelPool.java:87: line made to be no longer then 120 characters.

Result:
Code satisfies checkstyle requirements.

Pull request needs to contain only necessary changes

Motivation:
The pull request should not contain unnecessary changes that are not needed as part of required functionality of pull request.

Modifications:
private void doReleaseChannel(final Channel channel, final Promise<Void> promise) - >  private void doReleaseChannel(Channel channel, Promise<Void> promise)

Result:
Pull request contains less unnecessary modifications.
2015-08-17 15:43:08 -07:00
Scott Mitchell
beab89bec5 Remove Erroneous imports
Motivation:
As part of merging / cherry-picking there were some Erroneous imports added to AbstractMemcacheObjectAggregator.

Modifications:
- Remove Imports from AbstractMemcacheObjectAggregator.

Result:
Code now builds.
2015-08-17 10:12:43 -07:00
Scott Mitchell
a7135e8677 HttpObjectAggregator doesn't check content-length header
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.
2015-08-17 09:26:50 -07:00
Brendt Lucas
e796c99b23 Add unit tests for HTTP and SPDY headers
Motivation:

When attempting to retrieve a SPDY header using an AsciiString key, if the header was inserted using a String based key, the lookup would fail. Similarly, the lookup would fail if the header was inserted with an AsciiString key, and retrieved using a String key. This has been fixed with the header simplification commit (1a43923aa8).

Extra unit tests have been added to protect against this issue occurring in the future.  The tests check that a header added using String or AsciiString can be retrieved using AsciiString or String respectively.

Modifications:

Added more unit tests

Result:

Protect against issue #4053 happening again.
2015-08-17 08:51:14 -07:00
Scott Mitchell
ba6ce5449e Headers Performance Boost and Interface Simplification
Motivation:
A degradation in performance has been observed from the 4.0 branch as documented in https://github.com/netty/netty/issues/3962.

Modifications:
- Simplify Headers class hierarchy.
- Restore the DefaultHeaders to be based upon DefaultHttpHeaders from 4.0.
- Make various other modifications that are causing hot spots.

Result:
Performance is now on par with 4.0.
2015-08-17 08:50:11 -07:00
Jonas Konrad
e6ca9882a1 Add support for abstract domain sockets
Motivation:

Because of java custom UTF encoding, it was previously impossible to use
nul-bytes in domain socket names, which is required for abstract domain
sockets.

Modifications:

- Pass the encoded string byte array to the native code
- Modify native code accordingly to work with nul-bytes in the the
array.
- Move the string encoding to UTF-8 in java code.

Result:

Unix domain socket addresses will work properly if they contain nul-
bytes. Address encoding for these addresses changes from UTF-8-like to
real UTF-8.
2015-08-16 20:03:23 +02:00
Trustin Lee
ea18f73a88 Upgrade jacoco-maven-plugin to the latest version 2015-08-16 19:24:50 +09:00
Trustin Lee
75af257a62 Ignore EpollReuseAddrTest.testMultipleBindDatagramChannel
Motivation:

It keeps failing on some of our CI machines for an unknown reason.

Modifications:

Ignore it temporarily

Result:

Less noise in CI result
2015-08-15 09:07:17 +09:00
Norman Maurer
fd27c403d3 [#4010] Correctly handle whitespaces in HttpPostMultipartRequestDecoder
Motivation:

Due not using a cast we insert 32 and not a whitespace into the String.

Modifications:

Correclty cast to char.

Result:

Correct handling of whitespaces.
2015-08-14 21:16:42 +02:00
Scott Mitchell
d2683c3911 EPOLLRDHUP infinite loop
Motivation:
If  is enabled and a channel is half closed it is possible for the EPOLL event loop to get into an infinite loop by continuously being woken up on the EPOLLRDHUP event.

Modifications:
- Ensure that the EPOLLRDHUP event is unregistered for to prevent infinite loop.

Result:
1 less infinite loop.
2015-08-13 12:15:32 -07:00
Scott Mitchell
c65ef4fed7 Update npn and alpn dependencies
Motivation:
New versions of alpn-boot and npn-boot have been released.

Modifications:
- Update pom to pull in new versions.

Result:
Dependencies more up to date.
2015-08-13 10:05:13 -07:00
Scott Mitchell
a4261d481c Eclipse SPDY docs moved
Motivation:
We provide a hyperlink to the docs for SPDY if the runtime is not setup correctly to help users. These docs have moved.

Modifications:
- Update the hyperlink to point to the new doc location.

Result:
Users are able to find docs more easily.
2015-08-13 09:45:06 -07:00
Scott Mitchell
2d4bef9b18 Http2ConnectionHandler not flushing on writabilityChange
Motivation:
The Http2ConnectionHandler was writing pending bytes, but was not flushing. This may result in deadlock.

Modifications:
- Http2ConnectionHandler must writePendingBytes and also flush.

Result:
Data is now flushed after writabilityChange writes more data to underlying layers.
2015-08-13 09:43:00 -07:00
Norman Maurer
deda3f5b65 Add GCC optimization flag when compile native transport
Motivation:

We not set any optimization flag when compile native transport

Modification:

Add -O3 to CFLAGS to have GCC do optimizations

Result:

Ship optimized native code
2015-08-13 12:31:20 +02:00
Trustin Lee
cfd7b391c8 Add a property to disable osgi testsuite run 2015-08-13 09:53:26 +09:00
Norman Maurer
5ac84760c4 Allow to create SslContext from existing PrivateKey / X509Certificate
Motivation:

Sometimes the user already has a PrivateKey / X509Certificate which should be used to create a new SslContext. At the moment we only allow to construct it via Files.

Modifications:

- Add new methods to the SslContextBuilder to allow creating a SslContext from PrivateKey / X509Certificate
- Mark all public constructors of *SslContext as @Deprecated, the user should use SslContextBuilder
- Update tests to us SslContextBuilder.

Result:

Creating of SslContext is possible with PrivateKay/X509Certificate
2015-08-12 15:05:58 +02:00
Norman Maurer
9d417c1626 Fixing compile error, introduce by 5d011b8895 2015-08-12 14:25:45 +02:00
Ivan Bahdanau
5d011b8895 Improve the logic around acquire channel function is improved.
Motivation:
The acquire channel function resulted in calling itself several times in case when channel polled from the pool queue was unhealthy, which resulted FixedChannelPool to be called several times which in it's turn caused FixedChannelPool.acquire() to be called and resulted into acquireChannelCount to be unnecessary increased.
Example use case:
1) Create FixedChannelPool instance with one channel in the pool: new FixedChannelPool(cb, handler, 1)
2) Acquire channel A from the pool
3) close the channel A
4) Return it back to the pool
5) Acquire channel from the same pool again
Expected result:
new channel created and acquired, channel A that has been closed discarded and removed from the pool from being unhealthy
Actual result:
Channel A had been removed from the pool, how ever the new channel had never be acquired, instead the request to acquire had been added to the pending queue in FixedChannelPool and the acquireChannelCount is increased by one. The reason is that at the time when SimpleChannelPool figured out that the channel was unhealthy called FixedChannelPool.acquire to try to acquire new channel, how ever the request was added to the pendingTakQueue because by the time when FixedChannelPool.acquire was called, the acquireChannelCount was already "1" so new channel ould not be created cause of maxChannelsLimit=1.

Modifications:
The suggested approach modifies the SimpleChannelPool in a way so that when channel detected to be unhealthy it calls private method SimpleChannelPool.acquireHealthyFromPoolOrNew() which guarantees that SimpleChannelPool actually either finds a healthy channel in the pool and returns it or causes the promise.cause() in case when new channel was failed to be created.

 Result:
The  ```acquiredChannelCount``` is now calculated correctly as a result of SimpleChannelPool.acquire() of not being recursive on overridable acquire method.
2015-08-12 06:35:56 +02:00
Matteo Merli
0be53f296f MemoryRegionCache$Entry objects are not recycled
Motivation:

Even though MemoryRegionCache$Entry instances are allocated through a recycler they are not properly recycled,
leaving a lot of instances to be GCed along with Recycler$DefaultHandle objects.
Fixes #4071

Modification:

Recycle Entry when done using it.

Result:

Less GCed objects.
2015-08-10 21:29:25 +02:00
Scott Mitchell
cf171ff525 maxBytesPerRead channel configuration
Motiviation:
The current read loops don't fascilitate reading a maximum amount of bytes. This capability is useful to have more fine grain control over how much data is injested.

Modifications:
- Add a setMaxBytesPerRead(int) and getMaxBytesPerRead() to ChannelConfig
- Add a setMaxBytesPerIndividualRead(int) and getMaxBytesPerIndividualRead to ChannelConfig
- Add methods to RecvByteBufAllocator so that a pluggable scheme can be used to control the behavior of the read loop.
- Modify read loop for all transport types to respect the new RecvByteBufAllocator API

Result:
The ability to control how many bytes are read for each read operation/loop, and a more extensible read loop.
2015-08-05 23:59:54 -07:00
Scott Mitchell
b714297a44 HttpObjectDecoder half close behavior
Motivation:
In the event an HTTP message does not include either a content-length or a transfer-encoding header [RFC 7230](https://tools.ietf.org/html/rfc7230#section-3.3.3) states the behavior must be treated differently for requests and responses. If the channel is half closed then the HttpObjectDecoder is not invoking decodeLast and thus not checking if messages should be sent up the pipeline.

Modifications:
- Add comments to clarify regular decode default case.
- Handle the ChannelInputShutdownEvent in the HttpObjectDecoder and evaluate if messages need to be generated.

Result:
Messages are generated on half closed, and comments clarify existing logic.
2015-08-05 09:04:59 -07:00
Jakob Buchgraber
6fd0a0c55f Faster and more memory efficient headers for HTTP, HTTP/2, STOMP and SPYD. Fixes #3600
Motivation:

We noticed that the headers implementation in Netty for HTTP/2 uses quite a lot of memory
and that also at least the performance of randomly accessing a header is quite poor. The main
concern however was memory usage, as profiling has shown that a DefaultHttp2Headers
not only use a lot of memory it also wastes a lot due to the underlying hashmaps having
to be resized potentially several times as new headers are being inserted.

This is tracked as issue #3600.

Modifications:
We redesigned the DefaultHeaders to simply take a Map object in its constructor and
reimplemented the class using only the Map primitives. That way the implementation
is very concise and hopefully easy to understand and it allows each concrete headers
implementation to provide its own map or to even use a different headers implementation
for processing requests and writing responses i.e. incoming headers need to provide
fast random access while outgoing headers need fast insertion and fast iteration. The
new implementation can support this with hardly any code changes. It also comes
with the advantage that if the Netty project decides to add a third party collections library
as a dependency, one can simply plug in one of those very fast and memory efficient map
implementations and get faster and smaller headers for free.

For now, we are using the JDK's TreeMap for HTTP and HTTP/2 default headers.

Result:

- Significantly fewer lines of code in the implementation. While the total commit is still
  roughly 400 lines less, the actual implementation is a lot less. I just added some more
  tests and microbenchmarks.

- Overall performance is up. The current implementation should be significantly faster
  for insertion and retrieval. However, it is slower when it comes to iteration. There is simply
  no way a TreeMap can have the same iteration performance as a linked list (as used in the
  current headers implementation). That's totally fine though, because when looking at the
  benchmark results @ejona86 pointed out that the performance of the headers is completely
  dominated by insertion, that is insertion is so significantly faster in the new implementation
  that it does make up for several times the iteration speed. You can't iterate what you haven't
  inserted. I am demonstrating that in this spreadsheet [1]. (Actually, iteration performance is
  only down for HTTP, it's significantly improved for HTTP/2).

- Memory is down. The implementation with TreeMap uses on avg ~30% less memory. It also does not
  produce any garbage while being resized. In load tests for GRPC we have seen a memory reduction
  of up to 1.2KB per RPC. I summarized the memory improvements in this spreadsheet [1]. The data
  was generated by [2] using JOL.

- While it was my original intend to only improve the memory usage for HTTP/2, it should be similarly
  improved for HTTP, SPDY and STOMP as they all share a common implementation.

[1] https://docs.google.com/spreadsheets/d/1ck3RQklyzEcCLlyJoqDXPCWRGVUuS-ArZf0etSXLVDQ/edit#gid=0
[2] https://gist.github.com/buchgr/4458a8bdb51dd58c82b4
2015-08-04 17:12:24 -07:00