Commit Graph

331 Commits

Author SHA1 Message Date
Scott Mitchell
79f2e3604e HTTP/2 close only send GO_AWAY if one has not already been sent
Motivation:
Http2ConnectionHandler will always send a GO_AWAY when the channel is closed. This may cause problems if the user is attempting to control when GO_AWAY is sent and the content of the GO_AWAY.

Modifications:
- When the channel is closed Http2ConnectionHandler should only send a GO_AWAY if one has not already been sent

Result:
The user has more control over when GO_AWAY is sent
Fixes https://github.com/netty/netty/issues/5307
2016-06-06 11:18:30 -07:00
Norman Maurer
844976a0a2 Ensure the same ByteBufAllocator is used in the EmbeddedChannel when compress / decompress. Related to [#5294]
Motivation:

The user may specify to use a different allocator then the default. In this case we need to ensure it is shared when creating the EmbeddedChannel inside of a ChannelHandler

Modifications:

Use the config of the "original" Channel in the EmbeddedChannel and so share the same allocator etc.

Result:

Same type of buffers are used.
2016-05-31 09:08:33 +02:00
Norman Maurer
6ca49d1336 [maven-release-plugin] prepare for next development iteration 2016-05-25 19:16:44 +02:00
Norman Maurer
446b38db52 [maven-release-plugin] prepare release netty-4.1.0.Final 2016-05-25 19:14:15 +02:00
nmittler
79a06eaede Fix NPE in Http2ConnectionHandler.onHttpServerUpgrade
Motivation:

Performing a server upgrade with a new initial flow control window will cause an NPE in the DefaultHttp2RemoteFlowController. This is due to the fact that the monitor does not check whether or not the channel is writable.

Modifications:

Added a check for channel writability before calling `writePendingBytes`. Also fixed a unit test that was supposed to be testing this :).

Result:

Fixes #5301
2016-05-25 09:13:06 -07:00
Norman Maurer
7b25402e80 Add CompositeByteBuf.addComponent(boolean ...) method to simplify usage
Motivation:

At the moment the user is responsible to increase the writer index of the composite buffer when a new component is added. We should add some methods that handle this for the user as this is the most popular usage of the composite buffer.

Modifications:

Add new methods that autoamtically increase the writerIndex when buffers are added.

Result:

Easier usage of CompositeByteBuf.
2016-05-21 19:52:16 +02:00
Scott Mitchell
1cb706ac93 HTTP/2 HPACK Header Name Validation and Trailing Padding
Motivation:
The HPACK code currently disallows empty header names. This is not explicitly forbidden by the HPACK RFC https://tools.ietf.org/html/rfc7541. However the HTTP/1.x RFC https://tools.ietf.org/html/rfc7230#section-3.2 and thus HTTP/2 both disallow empty header names, and so this precondition check should be moved from the HPACK code to the protocol level.
HPACK also requires that string literals which are huffman encoded must be treated as an encoding error if the string has more than 7 trailing padding bits https://tools.ietf.org/html/rfc7541#section-5.2, but this is currently not enforced.

Result:
- HPACK to allow empty header names
- HTTP/1.x and HTTP/2 header validation should not allow empty header names
- Enforce max of 7 trailing padding bits

Result:
Code is more compliant with the above mentioned RFCs
Fixes https://github.com/netty/netty/issues/5228
2016-05-17 13:42:16 -07:00
Trustin Lee
3a9f472161 Make retained derived buffers recyclable
Related: #4333 #4421 #5128

Motivation:

slice(), duplicate() and readSlice() currently create a non-recyclable
derived buffer instance. Under heavy load, an application that creates a
lot of derived buffers can put the garbage collector under pressure.

Modifications:

- Add the following methods which creates a non-recyclable derived buffer
  - retainedSlice()
  - retainedDuplicate()
  - readRetainedSlice()
- Add the new recyclable derived buffer implementations, which has its
  own reference count value
- Add ByteBufHolder.retainedDuplicate()
- Add ByteBufHolder.replace(ByteBuf) so that..
  - a user can replace the content of the holder in a consistent way
  - copy/duplicate/retainedDuplicate() can delegate the holder
    construction to replace(ByteBuf)
- Use retainedDuplicate() and retainedSlice() wherever possible
- Miscellaneous:
  - Rename DuplicateByteBufTest to DuplicatedByteBufTest (missing 'D')
  - Make ReplayingDecoderByteBuf.reject() return an exception instead of
    throwing it so that its callers don't need to add dummy return
    statement

Result:

Derived buffers are now recycled when created via retainedSlice() and
retainedDuplicate() and derived from a pooled buffer
2016-05-17 11:16:13 +02:00
Norman Maurer
68cd670eb9 Remove ChannelHandlerInvoker
Motivation:

We tried to provide the ability for the user to change the semantics of the threading-model by delegate the invoking of the ChannelHandler to the ChannelHandlerInvoker. Unfortunually this not really worked out quite well and resulted in just more complexity and splitting of code that belongs together. We should remove the ChannelHandlerInvoker again and just do the same as in 4.0

Modifications:

Remove ChannelHandlerInvoker again and replace its usage in Http2MultiplexCodec

Result:

Easier code and less bad abstractions.
2016-05-17 11:14:00 +02:00
Norman Maurer
979dc3e3e4 Fix one more possible leak as a follow up of 341a235fea 2016-05-13 18:38:37 +02:00
Norman Maurer
341a235fea Fix possible leaks in Http2ServerDowngraderTest
Motivation:

We need to ensure we release all ReferenceCounted objects during tests to not leak.

Modifications:

Fix possible leaks by calling release()

Result:

No more leaks in tests.
2016-05-13 17:38:26 +02:00
Norman Maurer
4b24b0aac8 Annotate Http2ServerDowngrader with @UnstableApi
Motivation:

Everything in the http2 package should be considered unstable for now

Modifications:

Add missing annotation on Http2ServerDowngrader

Result:

Clearly mark class as unstable.
2016-05-13 08:41:46 +02:00
Moses Nakamura
4d2e91a10d codec-http2: Stop leaking in header downgrader test
Motivation:

We're leaking requests in our Http2ServerDowngrader tests when we
allocate a buffer using the local allocator.

Modification:

Release the request later when the request is constructed with the local
allocator.

Result:

Less leaky tests.
2016-05-10 09:04:52 +02:00
Scott Mitchell
d580245afc DefaultHttp2Connection.close Reentrant Modification
Motivation:
The DefaultHttp2Conneciton.close method accounts for active streams being iterated and attempts to avoid reentrant modifications of the underlying stream map by using iterators to remove from the stream map. However there are a few issues:

- While iterating over the stream map we don't prevent iterations over the active stream collection
- Removing a single stream may actually remove > 1 streams due to closed non-leaf streams being preserved in the priority tree which may result in NPE

Preserving closed non-leaf streams in the priority tree is no longer necessary with our current allocation algorithms, and so this feature (and related complexity) can be removed.

Modifications:
- DefaultHttp2Connection.close should prevent others from iterating over the active streams and reentrant modification scenarios which may result from this
- DefaultHttp2Connection should not keep closed stream in the priority tree
  - Remove all associated code in DefaultHttp2RemoteFlowController which accounts for this case including the ReducedState object
  - This includes fixing writability changes which depended on ReducedState
- Update unit tests

Result:
Fixes https://github.com/netty/netty/issues/5198
2016-05-09 14:16:30 -07:00
Moses Nakamura
9ce84dcb21 Http2 to Http1.1 converter on new Http2 server API
Motivation:

http/2 and http/1.1 have similar protocols, and it's useful to be able
to implement a single server against a single interface. There's an
injection from http/1.1 messages to http/2 ones, so it makes sense to
make folks program against http/1.1 and upgrade them under the hood.

Modifications:

added a MessageToMessageCodec<Http2StreamFrame, HttpObject> which turns
every kind of Http2StreamFrame domain object into an HttpObject domain
object, and then back again on the way out.  This one is specialized for
servers, but it should be straightforward to make a symmetric one for
clients, or else extend this one.

Result:

fixes #5199, and it's now simple to make your Http2MultiplexCodec speak
Http1.1
2016-05-09 13:24:46 -07:00
Norman Maurer
9229ed98e2 [#5088] Add annotation which marks packages/interfaces/classes as unstable
Motivation:

Some codecs should be considered unstable as these are relative new. For this purpose we should introduce an annotation which these codecs should us to be marked as unstable in terms of API.

Modifications:

- Add UnstableApi annotation and use it on codecs that are not stable
- Move http2.hpack to http2.internal.hpack as it is internal.

Result:

Better document unstable APIs.
2016-05-09 15:16:35 +02:00
Scott Mitchell
b17ee6066f HTTP/2 Log Cleanup
Motivation:
The format of some log lines used the printf style formatting instead of the logger {} formatting for variables. This would lead to printing out the literal printf tokens instead of substituting the value.

Modifications:
- Fix the format string for log statements which use printf style formatting

Result:
Logs actually capture the value of the variables instead of fixed string tokens.
2016-05-06 10:52:00 -07:00
Scott Mitchell
467e5a1019 DefaultHttp2FrameReader stops reading if stream error
Motivation:
DefaultHttp2FrameReader will stop reading data if any exception is thrown. However some exceptions are recoverable and we will lose data if we don't continue reading. For example some stream errors are recoverable.

Modifications:
- DefaultHttp2FrameReader should attempt to continue reading if a stream error is encountered.

Result:
Fixes https://github.com/netty/netty/issues/5186
2016-04-30 10:41:06 -07:00
Trustin Lee
38d05abd84 Reorganize imports 2016-04-14 18:58:47 +09:00
Norman Maurer
d698746609 Add ByteBuf.asReadOnly()
Motivation:

We lately added ByteBuf.isReadOnly() which allows to detect if a buffer is read-only or not. We should add ByteBuf.asReadOnly() to allow easily access a read-only version of a buffer.

Modifications:

- Add ByteBuf.asReadOnly()
- Deprecate Unpooled.unmodifiableBuffer(Bytebuf)

Result:

More consistent api.
2016-04-14 10:51:20 +02:00
Norman Maurer
7c734fcf73 Fix resource leak in tests introduced by 69070c37ba. 2016-04-14 10:13:55 +02:00
Trustin Lee
0b078314b2 Add ByteBuf.isReadOnly()
Motivation:

It is sometimes useful to determins if a buffer is read-only.

Modifications:

Add ByteBuf.isReadOnly()

Result:

One more feature
2016-04-13 21:41:27 +09:00
Norman Maurer
572bdfb494 [maven-release-plugin] prepare for next development iteration 2016-04-10 08:37:18 +02:00
Norman Maurer
c6121a6f49 [maven-release-plugin] prepare release netty-4.1.0.CR7 2016-04-10 08:36:56 +02:00
Norman Maurer
6e919f70f8 [maven-release-plugin] rollback the release of netty-4.1.0.CR7 2016-04-09 22:13:44 +02:00
Norman Maurer
4cdd51509a [maven-release-plugin] prepare release netty-4.1.0.CR7 2016-04-09 22:05:34 +02:00
Trustin Lee
3b941c2a7c [maven-release-plugin] prepare for next development iteration 2016-04-02 01:25:05 -04:00
Trustin Lee
7368ccc539 [maven-release-plugin] prepare release netty-4.1.0.CR6 2016-04-02 01:24:55 -04:00
Norman Maurer
cee38ed2b6 [maven-release-plugin] prepare for next development iteration 2016-03-29 16:45:13 +02:00
Norman Maurer
9cd9e7daeb [maven-release-plugin] prepare release netty-4.1.0.CR5 2016-03-29 16:44:33 +02:00
Scott Mitchell
61cfdd7671 e24a5d8 compile error
Motivation:
e24a5d8 was cherry-picked but had a compile error.

Modifications:
- Fix the compile error in e24a5d8

Result:
Build now compiles.
2016-03-25 12:51:13 -07:00
Eric Anderson
e24a5d8839 Map HTTP/2 Streams to Channels
Motivation:

This allows using handlers for Streams in normal Netty-style. Frames are
read/written to the channel as messages, not directly as a
callback/method call. Handlers allow mixing and can ease HTTP/1 and
HTTP/2 interoperability by eventually supporting HTTP/1 handlers in
HTTP/2 and vise versa.

Modifications:

New handler Http2MultiplexCodec that converts from the current HTTP/2
API to a message-based API and child channels for streams.

Result:

The basics are done for server-side: new streams trigger creation of new
channels in much the same appearance to how new connections trigger new
channel creation. The basic frames HEADERS and DATA are handled, but
also GOAWAY and RST_STREAM.

Inbound flow control is implemented, but outbound is not. That will be
done later, along with not completing write promises on the child
channel until the write actually completes on the parent.

There is not yet support for outbound priority/weight, push promises,
and many other features.

There is a generic Object that may be set on stream frames. This also
paves the way for client-side support which needs a way to refer to
yet-to-be-created streams (due to how HEADERS allocates a stream id, and
the allocation order must be the same as transmission order).
2016-03-25 12:14:44 -07:00
Carsten Varming
d6bf388343 Prevent nepotism with generational GCs.
Motivation:

If a single Encoder object is promoted to the old generation then every object
reachable from the promoted object will eventually be promoted as well. A queue
illustrates the problem very well. Say a sequence of inserts and deletions
generate an object graph:

   A -> B -> C -> D -> E -> F -> G -> H,

the head of the queue is E, the tail of the queue is H, and A, B, C, D are
dead. If all queue nodes are in the young generation, then a young gc will
clean up the object graph and leave us with:

   E -> F -> G -> H

on the other hand, if B and C were previously promoted to the old generation,
then a young collection assumes the refernece from C to D is from a live object
(this is a key result of generational gc, no need to mark the old generation).
Hence the young collection assumes the refence to D is a gc root and leave us
with the object graph:

   B-> C -> D -> E -> F -> G -> H.

Eventually D, E, F, G, H, and all queue nodes ever seen from this point on will
be promoted, regardless of their global live or dead status. It is generally
trivial to fix nepotism issues by simply breaking the reference chain after
dequeuing a node.

Currently Encoder objects do not null their references when removed from the
hash map. We have observed a 20X increase in promoted Encoder objects due to
nepotism.

Modifications:

Null before, after, and next fields when removing Encoder objects from maps.

Result:

Fewer promoted Encoder objects, fewer Encoder objects in the old generation,
shorter young collection times, old collections spaced further apart (nepotism
is just really bad). Enjoy.
2016-03-23 17:27:00 +01:00
Norman Maurer
28d03adbfe [maven-release-plugin] prepare for next development iteration 2016-03-21 11:51:50 +01:00
Norman Maurer
4653dc1d05 [maven-release-plugin] prepare release netty-4.1.0.CR4 2016-03-21 11:51:12 +01:00
Scott Mitchell
fc099292fd HTTP/2 DefaultHttp2ConnectionEncoder data frame size incorrect if error
Motivation:
If an error occurs during a write operation then DefaultHttp2ConnectionEncoder.FlowControlledData will clear the CoalescingBufferQueue which will reset the queue's readable bytes to 0. To recover from an error the DefaultHttp2RemoteFlowController will attempt to return bytes to the flow control window, but since the frame has reset its own size this will lead to invalid flow control accounting.

Modifications:
- DefaultHttp2ConnectionEncoder.FlowControlledData should not reset its size if an error occurs

Result:
No more flow controller errors due to DefaultHttp2ConnectionEncoder.FlowControlledData setting its size to 0 if an error occurs.
2016-03-18 11:38:01 -07:00
Norman Maurer
8ec594c6eb Change HttpServerUpgradeHandler.UpgradeCodec to allow aborting upgrade
Motivation:

HttpServerUpgradeHandler.UpgradeCodec.prepareUpgradeResponse should allow to abort the upgrade and so just continue with using HTTP. Beside this we should only pass in the response HttpHeaders as this is inline with the docs.

Modifications:

- UpgradeCodec.prepareUpgradeResponse now allows to return a boolean and so allows to specifiy if the upgrade should take place.
- Change the param from FullHttpResponse to HttpHeaders to be inline with the javadocs.

Result:

More flexible and correct handling of upgrades.
2016-03-18 17:01:59 +01:00
Norman Maurer
ed9d6c79bc [#4972] Remove misleading argument from HttpServerUpgradeHandler.UpgradeCodec.upgradeTo
Motivation:

upgradeTo(...) takes the response as paramater, but the respone itself was already written to the Channel. This gives the user the impression the response can be changed or even act on it which may not be safe anymore once it was written and has been released.

Modifications:

Remove the response param from the method.

Result:

Less confusion and safer usage.
2016-03-17 10:50:07 +01:00
Scott Mitchell
6a2425b846 HTTP/2 SimpleChannelPromiseAggregator don't fail fast
Motivation:
Http2Codec.SimpleChannelPromiseAggregator currently fails fast if as soon as a tryFailure or setFailure method is called. This can lead to write operations which pass the result of SimpleChannelPromiseAggregator.newPromise to multiple channel.write calls throwing exceptions due to the promise being already done. This behavior is not expected by most of the Netty codecs (SslHandler) and can also create unexpected leaks in the http2 codec (DefaultHttp2FrameWriter).

Modifications:
- Http2Codec.SimpleChannelPromiseAggregator shouldn't complete the promise until doneAllocatingPromises is called
- Usages of Http2Codec.SimpleChannelPromiseAggregator should be adjusted to handle the change in behavior
- What were leaks in DefaultHttp2FrameWriter should be fixed to catch any other cases where ctx.write may throw

Result:
SimpleChannelPromiseAggregator won't generate promises which are done when newPromise is called.
2016-03-14 11:00:21 -07:00
Scott Mitchell
45849b2fa8 Deprecate PromiseAggregator
Motivation:
PromiseAggregator's API allows for the aggregate promise to complete before the user is done adding promises. In order to support this use case the API structure would need to change in a breaking manner.

Modifications:
- Deprecate PromiseAggregator and subclasses
- Introduce PromiseCombiner which corrects these issues

Result:
PromiseCombiner corrects the deficiencies in PromiseAggregator.
2016-03-14 10:53:30 -07:00
Scott Mitchell
404666d247 HTTP/2 ByteBufUtil.writeUtf8 cleanup
Motiviation:
691bc1690e made writeUtf8 consistent with String.getBytes() so that it never throws.
94f27be59b provided a writeUtf8 method which takes a ByteBufAllocator to do an appropriately sized buffer allocation.

Result:
- Assume writeUtf8 will not throw in HTTP/2 codec
- Use the new writeUtf8 method

Result:
Cleaner code in codec-http2.
2016-03-11 14:24:45 -08:00
Scott Mitchell
bd6040a36e HTTP/2 DefaultHttp2Connection NPE
Motivation:
If while iterating the active streams a close operation occurs this will be queued and process after the iteration has completed to avoid a concurrent modification exception. However it is possible that during the iteration the stream which was closed could have been removed from the priority tree and its parent would be set to null. Then after the iteration completes the close operation will attempt to dereference the parent and results in a NPE.

Modifications:
- pending close operations should verify the stream's parent is not null before processing the event

Result:
No More NPE.
2016-03-11 07:41:04 -08:00
Scott Mitchell
900353af52 HTTP/2 Reduce Log Level
Motivation:
83c4aa6ad8 changed the log level to warn, but should have changed to debug.

Modifications:
- Change the log level to debug in Http2ConnectionHandler if the GO_AWAY fails to send. The write failure could be the result of the channel already being closed.

Result:
Fixes https://github.com/netty/netty/issues/4930.
2016-03-04 17:25:04 -08:00
Scott Mitchell
4b5b230802 HTTP/2 DefaultHttp2HeadersDecoder weighted average error
Motiviation:
cfcee5798d introduced code to resize the headers based upon a weighted average. The weight used for new entries was initialized using integer arithmetic when it should have been floating point arithmetic and so new values contribute 0 weight.

Modifications:
- Cast to float when initializing

Result:
Weighted average does not give 0 weight to new headers in DefaultHttp2HeadersDecoder.
2016-03-02 15:07:38 -08:00
Scott Mitchell
c4fbc0642d HTTP/2 stream removed from map before onStreamClosed called
Motivation:
The interface contract of Http2Connection.Listener.onStreamClosed says that the stream will be removed from the active stream map, and not necessarily the stream map. If the channel becomes inactive we may remove from the stream map before calling onStreamClosed.

Modifications:
- Don't remove from the stream map during iteration until after onStreamClosed is called

Result:
Expectations of onStreamClosed interface are not violated
2016-02-29 08:46:22 -08:00
Norman Maurer
ca443e42e0 [maven-release-plugin] prepare for next development iteration 2016-02-19 23:00:11 +01:00
Norman Maurer
f39eb9a6b2 [maven-release-plugin] prepare release netty-4.1.0.CR3 2016-02-19 22:59:52 +01:00
Scott Mitchell
83c4aa6ad8 HTTP/2 Writes GO_AWAY on channelInactive
Motivation:
Http2ConnectionHandler inherits from ByteToMessageDecoder. ByteToMessageDecoder.channelInactive will attempt to decode any remaining data by calling the abstract decode method. If the Http2ConnectionHandler is in server mode, and no data has been exchanged yet, it will try to treat this data as an invalid connection preface and write a GO_AWAY. This is noisy in the logs and creates an illusion that there is a protocol violation when there has not been.

Modifications:
- If the channel is inactive the connection preface decode should not be executed.

Result:
Log files don't include misleading error messages related to connection preface errors.
2016-02-18 19:47:42 -08:00
Brendt Lucas
41d0a81691 Use ByteBufAllocator to allocate ByteBuf for FullHttpMessage Motivation: When converting SPDY or HTTP/2 frames to HTTP/1.x, netty always used an unpooled heap ByteBuf.
Modifications:
When constructing the FullHttpMessage pass in the ByteBuf to use via the ByteBufAllocator assigned via the context.

Result:
The ByteBuf assigned to the FullHttpMessage can now be configured as a pooled/unpooled, direct/heap based ByteBuf via the ByteBufAllocator used.
2016-02-17 19:55:52 -08:00
Moses Nakamura
f0f0b69d90 fixed "sensative" typo to read "sensitive" 2016-02-17 08:18:11 -08:00
Scott Mitchell
06e29e0d1b HTTP/2 codec may not always call Http2Connection.onStreamRemoved
Motivation:
Http2Connection.onStreamRemoved is not always called if Http2Connection.onStreamAdded is called. This is problematic as users may rely on the onStreamRemoved method to be called to release ByteBuf objects and do other cleanup.

Modifications:
- Http2Connection.close will remove all streams existing streams and prevent new ones from being created
- Http2ConnectionHandler will call the new close method in channelInactive

Result:
Http2Connection.onStreamRemoved is always called when Http2Connection.onStreamRemoved is called to preserve the Http2Connection guarantees.
Fixes https://github.com/netty/netty/issues/4838
2016-02-12 16:01:37 -08:00
Scott Mitchell
56e6e07b25 HTTP/2 RST_STREAM Regression f990f99
Motivation:
Commit f990f99 introduced a bug into the RST_STREAM processing that would prevent a RST_STREAM from being sent when it should have been. The promise would be marked as successful even though the RST_STREAM frame would never be sent.

Modifications:
- Fix conditional in Http2ConnectionHandler.resetStream to allow reset streams to be sent in all stream states besides IDLE.

Result:
RST_STREAM frames are now sent when they are supposed to be sent
Fixes https://github.com/netty/netty/issues/4856
2016-02-10 13:47:53 -08:00
Norman Maurer
75a2ddd61c [maven-release-plugin] prepare for next development iteration 2016-02-04 16:51:44 +01:00
Norman Maurer
7eb3a60dba [maven-release-plugin] prepare release netty-4.1.0.CR2 2016-02-04 16:37:06 +01:00
Scott Mitchell
7a7160f176 HTTP/2 Buffer Leak if UTF8 Conversion Fails
Motivation:
Http2CodecUtil uses ByteBufUtil.writeUtf8 but does not account for it
throwing an exception. If an exception is thrown because the format is
not valid UTF16 encoded UTF8 then the buffer will leak.

Modifications:
- Make sure the buffer is released if an exception is thrown
- Ensure call sites of the Http2CodecUtil.toByteBuf can tolerate and
  exception being thrown

Result:
No leak if exception data can not be converted to UTF8.
2016-02-02 11:22:17 -08:00
Scott Mitchell
f990f9983d HTTP/2 Don't Flow Control Iniital Headers
Motivation:
Currently the initial headers for every stream is queued in the flow controller. Since the initial header frame may create streams the peer must receive these frames in the order in which they were created, or else this will be a protocol error and the connection will be closed. Tolerating the initial headers being queued would increase the complexity of the WeightedFairQueueByteDistributor and there is benefit of doing so is not clear.

Modifications:
- The initial headers will no longer be queued in the flow controllers

Result:
Fixes https://github.com/netty/netty/issues/4758
2016-02-01 13:37:43 -08:00
Scott Mitchell
5fb18e3415 InboundHttp2ToHttpAdapter leak and logic improvements
Motivation:
In HttpConversionUtil's toHttpRequest and toHttpResponse methods can
allocate FullHttpMessage objects, and if an exeception is thrown during
the header conversion then this object will not be released. If a
FullHttpMessage is not fired up the pipeline, and the stream is closed
then we remove from the map, but do not release the object. This leads
to a ByteBuf leak. Some of the logic related to stream lifetime management
and FullHttpMessage also predates the RFC being finalized and is not correct.

Modifications:
- Fix leaks in HttpConversionUtil
- Ensure the objects are released when they are removed from the map.
- Correct logic and unit tests where they are found to be incorrect.

Result:
Fixes https://github.com/netty/netty/issues/4780
Fixes https://github.com/netty/netty/issues/3619
2016-02-01 12:38:40 -08:00
Trustin Lee
4d6ab1d30d Fix missing trailing data on HTTP client upgrade
Motivation:

When HttpClientUpgradeHandler upgrades from HTTP/1 to another protocol,
it performs a two-step opertion:

1. Remove the SourceCodec (HttpClientCodec)
2. Add the UpgradeCodec

When HttpClientCodec is removed from the pipeline, the decoder being
removed triggers channelRead() event with the data left in its
cumulation buffer. However, this is not received by the UpgradeCodec
becuase it's not added yet. e.g. HTTP/2 SETTINGS frame sent by the
server can be missed out.

To fix the problem, we need to reverse the steps:

1. Add the UpgradeCodec
2. Remove the SourceCodec

However, this does not work as expected either, because UpgradeCodec can
send a greeting message such as HTTP/2 Preface. Such a greeting message
will be handled by the SourceCodec and will trigger an 'unsupported
message type' exception.

To fix the problem really, we need to make the upgrade process 3-step:

1. Remove/disable the encoder of SourceCodec
2. Add the UpgradeCodec
3. Remove the SourceCodec

Modifications:

- Add SourceCodec.prepareUpgradeFrom() so that SourceCodec can remove or
  disable its encoder
- Implement HttpClientCodec.prepareUpgradeFrom() properly
- Miscellaneous:
  - Log the related channel as well When logging the failure to send a
    GOAWAY

Result:

Cleartext HTTP/1-to-HTTP/2 upgrade works again.
2016-02-01 15:52:37 +01:00
Scott Mitchell
11bcb8790c Http2Connection stream id generation to support queueing
Motivation:
StreamBufferingEncoder provides queueing so that MAX_CONCURRENT_STREAMS is not violated. However the stream id generation provided by Http2Connection.nextStreamId() only returns the next stream id that is expected on the connection and does not account for queueing. The codec should provide a way to generate the next stream id for a given endpoint that functions with or without queueing.

Modifications:
- Change Http2Connection.nextStreamId to Http2Connection.incrementAndGetNextStreamId

Result:
Http2Connection can generate the next stream id in queued and non-queued scenarios.
Fixes https://github.com/netty/netty/issues/4704
2016-01-29 11:37:17 -08:00
Scott Mitchell
78b508a7eb AbstractHttp2ConnectionHandlerBuilder validateHeaders cannot be set with encoder/decoder
Motivation:
If validateHeaders is set in combination with the encoder/decoder it will be silently ignored. We should enforce the constraint that validateHeaders and encoder/decoder are mutually exclusive.

Modifications:
- Make sure either validateHeaders can be set or encoder/decoder.

Result:
AbstractHttp2ConnectionHandlerBuilder does not allow conflicting options to be set.
2016-01-29 00:33:45 -08:00
Scott Mitchell
e8850072e2 HTTP/2 DefaultHttp2RemoteFlowController frame merging with padding bug
Motivation:
DefaultHttp2RemoteFlowController does not correctly account for the padding in the event frames are merged. This causes the internal stat of DefaultHttp2RemoteFlowController to become corrupt and can result in attempting to write frames when there are none.

Modifications:
- Update DefaultHttp2RemoteFlowController to account for frame sizes not necessarily adding together.

Result:
DefaultHttp2RemoteFlowController internal state does not become corrupt when padding is present.
Fixes https://github.com/netty/netty/issues/4573
2016-01-27 14:52:00 -08:00
Norman Maurer
1c417e5f82 [maven-release-plugin] prepare for next development iteration 2016-01-21 15:35:55 +01:00
Norman Maurer
c681a40a78 [maven-release-plugin] prepare release netty-4.1.0.CR1 2016-01-21 15:28:21 +01:00
Alex Petrov
7bcae8919d Log the current channel in Http2FrameLogger
Motivation:
Currently it's impossible to distinguish which
connection the corresponding logged message	is
related to.

Modifications:
Http2FrameLogger is extended to support channel
id logging, usages in Inbound/Outbound Frame
Loggers are adjusted accordingly.

Result:
Logger outputs the channel id.
2016-01-18 10:52:08 +01:00
Scott Mitchell
7dba13f276 HttpConversionUtil remove throws from method signature
Motivation:
HttpConversionUtil.toHttp2Headers currently has a throws Exception as part of the signature. This comes from the signature of ByteProcessor.process, but is not necessary because the ByteProcessor used does not throw.

Modifications:
- Remove throws Exception from the signature of HttpConversionUtil.toHttp2Headers.

Result:
HttpConversionUtil.toHttp2Headers interface does not propagate a throws Exception when it is used.
2016-01-15 10:53:34 +01:00
Norman Maurer
8716b9d4bd Revert "Fix unnecessary boxing and incorrect Serializable"
This reverts commit 0ae6f17285.
2015-12-31 14:48:10 +01:00
Xiaoyan Lin
0ae6f17285 Fix unnecessary boxing and incorrect Serializable
Motivation:

- AbstractHttp2ConnectionHandlerBuilder.encoderEnforceMaxConcurrentStreams can be the primitive boolean
- SpdySession.StreamComparator should not be Serializable since SpdySession is not Serializable

Modifications:

Use boolean instead and remove Serializable

Result:

- Minor improvement for AbstractHttp2ConnectionHandlerBuilder
- StreamComparator is not Serializable any more
2015-12-31 10:45:24 +01:00
Xiaoyan Lin
475d901131 Fix errors reported by javadoc
Motivation:

Javadoc reports errors about invalid docs.

Modifications:

Fix some errors reported by javadoc.

Result:

A lot of javadoc errors are fixed by this patch.
2015-12-27 08:36:45 +01:00
Xiaoyan Lin
a96d52fe66 Fix javadoc links and tags
Motivation:

There are some wrong links and tags in javadoc.

Modifications:

Fix the wrong links and tags in javadoc.

Result:

These links will work correctly in javadoc.
2015-12-26 08:34:31 +01:00
Scott Mitchell
80cff236e4 HTTP/2 UniformStreamByteDistributor negative window shouldn't write
Motivation:
If the stream window is negative UniformStreamByteDistributor may write data. This is prohibited by the RFC https://tools.ietf.org/html/rfc7540#section-6.9.2.

Modifications:
- UniformStreamByteDistributor should use StreamState.isWriteAllowed()

Result:
UniformStreamByteDistributor is more complaint with HTTP/2 RFC.
Fixes https://github.com/netty/netty/issues/4545
2015-12-23 10:14:24 -08:00
Scott Mitchell
7b2f55ec2f HTTP/2 Remove RemoteFlowController.streamWritten
Motivation:
RemoteFlowController.streamWritten is not currently required. We should remove it to keep interfaces minimal.

Modifications:
- Remove RemoteFlowController.streamWritten

Result:
1 Less method in RemoteFlowController interface.
Fixes https://github.com/netty/netty/issues/4600
2015-12-22 16:59:40 -08:00
Scott Mitchell
72accceeac HTTP/2 remove PriorityStreamByteDistributor
Motivation:
PriorityStreamByteDistributor is now obsolete and can be replaced by WeightedFairQueueByteDistributor.

Modifications:
- Remove PriorityStreamByteDistributor and use WeightedFairQueueByteDistributor by default.

Result:
PriorityStreamByteDistributor no longer has to be maintained and is replaced by a better algorithm.
2015-12-21 08:56:50 -08:00
Scott Mitchell
9ac430f16f HTTP/2 DefaultHttp2RemoteFlowController Stream writability notification broken
Motivation:
DefaultHttp2RemoteFlowController.ListenerWritabilityMonitor no longer reliably detects when a stream's writability change occurs.

Modifications:
- Ensure writiability is reliabily reported by DefaultHttp2RemoteFlowController.ListenerWritabilityMonitor
- Fix infinite loop issue (https://github.com/netty/netty/issues/4588) detected when consolidating unit tests

Result:
Reliable stream writability change notification, and 1 less infinite loop in UniformStreamByteDistributor.
Fixes https://github.com/netty/netty/issues/4587
2015-12-21 10:01:33 +01:00
Trustin Lee
b39380ad83 Revamp InboundHttp2ToHttpAdapter builder API
Related: #4572 #4574

Motivation:

Consistency in our builder API design

Modifications:

- Add AbstractInboundHttp2ToHttpAdapterBuilder
- Replace the old 'Builder's with InboundHttp2ToHttpAdapterBuilder and
  InboundHttp2ToHttpPriorityAdapterBuilder

Result:

Builder API consistency
2015-12-18 12:44:46 +09:00
Scott Mitchell
904e70a4d4 HTTP/2 Weighted Fair Queue Byte Distributor
Motivation:
PriorityStreamByteDistributor uses a homegrown algorithm which distributes bytes to nodes in the priority tree. PriorityStreamByteDistributor has no concept of goodput which may result in poor utilization of network resources. PriorityStreamByteDistributor also has performance issues related to the tree traversal approach and number of nodes that must be visited. There also exists some more proven algorithms from the resource scheduling domain which PriorityStreamByteDistributor does not employ.

Modifications:
- Introduce a new ByteDistributor which uses elements from weighted fair queue schedulers

Result:
StreamByteDistributor which is sensitive to priority and uses a more familiar distribution concept.
Fixes https://github.com/netty/netty/issues/4462
2015-12-17 11:17:02 -08:00
Trustin Lee
2202e8f967 Revamp the Http2ConnectionHandler builder API
Related: #4572

Motivation:

- A user might want to extend Http2ConnectionHandler and define his/her
  own static inner Builder class that extends
  Http2ConnectionHandler.BuilderBase. This introduces potential
  confusion because there's already Http2ConnectionHandler.Builder. Your
  IDE will warn about this name duplication as well.
- BuilderBase exposes all setters with public modifier. A user's Builder
  might not want to expose them to enforce it to certain configuration.
  There's no way to hide them because it's public already and they are
  final.
- BuilderBase.build(Http2ConnectionDecoder, Http2ConnectionEncoder)
  ignores most properties exposed by BuilderBase, such as
  validateHeaders, frameLogger and encoderEnforceMaxConcurrentStreams.
  If any build() method ignores the properties exposed by the builder,
  there's something wrong.
- A user's Builder that extends BuilderBase might want to require more
  parameters in build(). There's no way to do that cleanly because
  build() is public and final already.

Modifications:

- Make BuilderBase and Builder top-level so that there's no duplicate
  name issue anymore.
  - Add AbstractHttp2ConnectionHandlerBuilder
  - Add Http2ConnectionHandlerBuilder
  - Add HttpToHttp2ConnectionHandlerBuilder
- Make all builder methods in AbstractHttp2ConnectionHandlerBuilder
  protected so that a subclass can choose which methods to expose
- Provide only a single build() method
  - Add connection() and codec() so that a user can still specify
    Http2Connection or Http2Connection(En|De)coder explicitly
  - Implement proper state validation mechanism so that it is prevented
    to invoke conflicting setters

Result:

Less confusing yet flexible builder API
2015-12-17 14:08:13 +09:00
Norman Maurer
f4386fb8e9 Allow to set Http2HeaderEncoder.SensitivityDetector in the Http2ConnectionHandler
Motivation:

Some times the user wants to set a Http2HeaderEncoder.SensitivityDetector when building a Http2ConnectionHandler.

Modifications:

Allow to set Http2HeaderEncoder.SensitivityDetector via builder.

Result:

More flexible building of Http2ConnectionHandler possible.
2015-12-10 14:34:04 +01:00
Norman Maurer
c6d43667d4 Add Http2HeadersEncoder.ALWAYS_SENSITIVE instance
Motivation:

We already provide a NEVER_SENSITIVE instance,we should add ALWAYS_SENSITIVE as well.

Modifications:

Add ALWAYS_SENSITIVE instance which will always return true when check for sesitive.

Result:

User can reuse code.
2015-12-10 12:59:04 +01:00
Scott Mitchell
6257091d12 HttpConversionUtil does not account for COOKIE compression
Motivation:
The HTTP/2 RFC allows for COOKIE values to be split into individual header elements to get more benefit from compression (https://tools.ietf.org/html/rfc7540#section-8.1.2.5). HttpConversionUtil was not accounting for this behavior.

Modifications:
- Modify HttpConversionUtil to support compressing and decompressing the COOKIE values

Result:
HttpConversionUtil is compatible with https://tools.ietf.org/html/rfc7540#section-8.1.2.5)
Fixes https://github.com/netty/netty/issues/4457
2015-12-08 20:00:31 -08:00
nmittler
8cd259896e No HTTP/2 RST_STREAM if no prior HEADERS were sent
Motivation:

Because we flow control HEADERS frames, it's possible that an intermediate error can result in a RST_STREAM frame being sent for a frame that the other endpoint is not yet aware of. This is a violation of the spec and will either result in spammy logs at the other endpoint or broken connections.

Modifications:

Modified the HTTP/2 handler so that it only sends RST_STREAM if it has sent at least one HEADERS frame to the remote endpoint for the stream.

Result:

Fixes #4465
2015-11-25 13:46:32 -08:00
nmittler
dbaeb3314e Allow HTTP2 frame writer to accept arbitrarily large frames
Motivation:

The encoder is currently responsible for chunking frames when writing in order to conform to max frame size. The frame writer would be a better place for this since it could perform a reuse the same promise aggregator for all the write and could also perform a single allocation for all of the frame headers.

Modifications:

Modified `DefaultHttp2FrameWriter` to perform the chunking and modified the contract in the `Http2FrameWriter` interface. Modified `DefaultHttp2ConnectionEncoder` to send give all allocated bytes to the writer.

Result:

Fixes #3966
2015-11-24 11:44:06 -08:00
nmittler
79ab756fa3 Use a single queue in UniformStreamByteDistributor
Motivation:

The UniformStreamByteDistributor currently processes all zero-length frames, regardless of add order. This means that we would always send HEADERS for all streams, possibly taking away bandwidth for streams that actually have data.

Modifications:

Empty frames are now treated the same as any other frame except that the algorithm will pop off the any empty frames at the head of the queue.

Result:

Empty frames require no extra processing.
2015-11-24 08:11:23 -08:00
Scott Mitchell
cfcee5798d Adjustable size of DefaultHeaders array
Motivation:
DefaultHeaders creates an array of size 16 for all headers. This may waste a good deal of memory if applications only have a small number of headers. This memory may be critical when the number of connections grows large.

Modifications:
- Make the size of the array for DefaultHeaders configurable

Result:
Applications can control the size of the DefaultHeaders array and save memory.
2015-11-23 15:38:08 -08:00
nmittler
2a2059d976 Adding UniformStreamByteDistributor
Motivation:

The current priority algorithm can yield poor per-stream goodput when either the number of streams is high or the connection window is small. When all priorities are the same (i.e. priority is disabled), we should be able to do better.

Modifications:

Added a new UniformStreamByteDistributor that ignores priority entirely and manages a queue of streams.  Each stream is allocated a minimum of 1KiB on each iteration.

Result:

Improved goodput when priority is not used.
2015-11-19 16:49:12 -08:00
nmittler
96f9b0b91b Remote flow controller incorrectly updates stream state
Motivation:

The `DefaultHttp2RemoteFlowController` does not correctly determine `hasFrame` when updating the stream state for the distributor. Adding a check to enforce `hasFrame` when `streamableBytes > 0` causes several test failures.

Modifications:

Modified `DefaultHttp2RemoteFlowController` to simplify the writing logic and to correct the bookkeeping for `hasFrame`.

Result:

The distributors are always called with valid arguments.
2015-11-18 11:32:18 -08:00
nmittler
8accc52b03 Forking Twitter's hpack
Motivation:

The twitter hpack project does not have the support that it used to have.  See discussion here: https://github.com/netty/netty/issues/4403.

Modifications:

Created a new module in Netty and copied the latest from twitter hpack master.

Result:

Netty no longer depends on twitter hpack.
2015-11-14 10:13:32 -08:00
Norman Maurer
2ecce8fa56 [maven-release-plugin] prepare for next development iteration 2015-11-10 22:59:33 +01:00
Norman Maurer
6a93f331d3 [maven-release-plugin] prepare release netty-4.1.0.Beta8 2015-11-10 22:50:57 +01:00
Scott Mitchell
2f81364522 DefaultHttp2HeadersTest updates
Motivation:
Recently a bug was found in DefaultHttp2Headers where the state of the headers could be corrupted due to the extra tracking to make pseudo headers first during iteration. Unit tests did not catch this bug.

Modifications:
- Update unit tests to cover more methods

Result:
Unit tests for DefaultHttp2Headers have better code coverage.
2015-11-07 10:14:00 -08:00
Louis Ryan
7cc320ce47 Fix memory leak in DefaultHttp2Headers
Motivation:

Memory leak makes headers non-reusable.

Modifications:

Correctly reset firstNonPseudo header reference

Result:

No leak
2015-11-06 07:08:57 -08:00
nmittler
6504d52b94 Add HTTP/2 local flow control option for auto refill
Motivation:

For many HTTP/2 applications (such as gRPC) it is necessary to autorefill the connection window in order to prevent application-level deadlocking.

Consider an application with 2 streams, A and B.  A receives a stream of messages and the application pops off one message at a time and makes a request on stream B. However, if receiving of data on A has caused the connection window to collapse, B will not be able to receive any data and the application will deadlock.  The only way (currently) to get around this is 1) use multiple connections, or 2) manually refill the connection window.  Both are undesirable and could needlessly complicate the application code.

Modifications:

Add a configuration option to DefaultHttp2LocalFlowController, allowing it to autorefill the connection window.

Result:

Applications can configure HTTP/2 to avoid inter-stream deadlocking.
2015-11-05 15:47:10 -08:00
Scott Mitchell
91b8ef3d10 HTTP/2 PriorityStreamByteDistributor exceptions and reentry
Motivation:
PriorityStreamByteDistributor saves exception state and attempts to reset state. This could be simplified by just throwing a connection error and closing the connection. PriorityStreamByteDistributor also does not handle or detect re-entry in the distribute method.

Motivation:
- PriorityStreamByteDistributor propagate an INTERNAL_ERROR if an exception occurs during writing
- PriorityStreamByteDistributor to handle re-entry on the write method

Result:
PriorityStreamByteDistributor exception code state simplified, and re-entry is detected.
2015-11-03 13:22:11 -08:00
Trustin Lee
8f334885ef Reject the first SETTINGS ack on HTTP/2 Preface
Motivation:

Http2ConnectionHandler verifies if the first frame after the preface is
a SETTINGS frame.  However, it does not reject the SETTING ack frame
which is not expected actually.

Modifications:

Reject a SETTINGS-ack frame as well

Result:

When the first frame is a SETTINGS-ack frame, connection does not
proceed to further frame handling. (simplicity)
2015-11-03 11:23:54 +09:00
Scott Mitchell
19658e9cd8 HTTP/2 Headers Type Updates
Motivation:
The HTTP/2 RFC (https://tools.ietf.org/html/rfc7540#section-8.1.2) indicates that header names consist of ASCII characters. We currently use ByteString to represent HTTP/2 header names. The HTTP/2 RFC (https://tools.ietf.org/html/rfc7540#section-10.3) also eludes to header values inheriting the same validity characteristics as HTTP/1.x. Using AsciiString for the value type of HTTP/2 headers would allow for re-use of predefined HTTP/1.x values, and make comparisons more intuitive. The Headers<T> interface could also be expanded to allow for easier use of header types which do not have the same Key and Value type.

Motivation:
- Change Headers<T> to Headers<K, V>
- Change Http2Headers<ByteString> to Http2Headers<CharSequence, CharSequence>
- Remove ByteString. Having AsciiString extend ByteString complicates equality comparisons when the hash code algorithm is no longer shared.

Result:
Http2Header types are more representative of the HTTP/2 RFC, and relationship between HTTP/2 header name/values more directly relates to HTTP/1.x header names/values.
2015-10-30 15:29:44 -07:00
Scott Mitchell
d66520db1b Http2ConnectionHandler.BaseBuilder exception cleanup
Motivation:
Http2ConnectionHandler.BaseBuilder is constructing objects which have 'close' methods, but is not calling these methods in the event of an exception.

Modifications:
- Objects which implement 'close' should have this method called if an exception is thrown and the build operation can not complete normally.

Result:
Objects are closed even if the build process encounters an error.
2015-10-29 11:09:51 -07:00
Norman Maurer
077af8c019 Remove encoderMaxConcurrentStreams
Motivation:

Remove encoderMaxConcurrentStreams(...) and use the default settings. Also throw an exception if server mode is used.

Modifications:

- Remove encoderMaxConcurrentStreams(...) method
- Throw exception if server mode is used and trying to enforce conncurrent streams.

Result:

Correctly support settings stuff via builder
2015-10-15 10:11:39 +02:00
Norman Maurer
e419533498 Remove unused parameter from method declaration.
Motivation:

We had an unused paramter on a method, we should just remove it to keep code clean.

Modifications:

- Remove parameter
- Fix typo in javadoc

Result:

Cleanup done.
2015-10-07 09:18:22 +02:00
Norman Maurer
2ff2806ada [maven-release-plugin] prepare for next development iteration 2015-10-02 09:03:29 +02:00
Norman Maurer
5a43de10f7 [maven-release-plugin] prepare release netty-4.1.0.Beta7 2015-10-02 09:02:58 +02:00
Scott Mitchell
06c3ae07a0 DefaultHttp2ConnectionDecoder write ping buffer
Motivation:
DefaultHttp2ConnectionDecoder writes a ACK when receiving a ping frame and sends the same data buffer it received. The data buffer is also passed to the listener, but the indexes are shared between the send and the listener. We should ensure the indexes are independent for these two operations.

Modifications:
- Call slice on the buffer that is being sent

Result:
Listener now has access to a buffer that will not appear to be already consumed.
2015-10-02 11:40:21 -07:00
Scott Mitchell
284e3702d8 Http2ConnectionHandler Builder instead of constructors
Motivation:
Using the builder pattern for Http2ConnectionHandler (and subclasses) would be advantageous for the following reasons:
1. Provides the consistent construction afforded by the builder pattern for 'optional' arguments. Users can specify these options 1 time in the builder and then re-use the builder after this.
2. Enforces that the Http2ConnectionHandler's internals (decoder Http2FrameListener) are initialized after construction.

Modifications:
- Add an extensible builder which can be used to build Http2ConnectionHandler objects
- Update classes which inherit from Http2ConnectionHandler

Result:
It is easier to specify options and construct Http2ConnectionHandler objects.
2015-10-01 13:51:03 -07:00
Scott Mitchell
1485a87e25 Http2ConnectionHandler and Http2FrameListener cyclic dependency
Motivation:
It is often the case that implementations of Http2FrameListener will want to send responses when data is read. The Http2FrameListener needs access to the Http2ConnectionHandler (or the encoder contained within) to be able to send responses. However the Http2ConnectionHandler requires a Http2FrameListener instance to be passed in during construction time. This creates a cyclic dependency which can make it difficult to cleanly accomplish this relationship.

Modifications:
- Add Http2ConnectionDecoder.frameListener(..) method to set the frame listener. This will allow the listener to be set after construction.

Result:
Classes which inherit from Http2ConnectionHandler can more cleanly set the Http2FrameListener.
2015-09-30 15:41:15 -07:00
Scott Mitchell
0e9545e94d Http2RemoteFlowController stream writibility listener
Motivation:
For implementations that want to manage flow control down to the stream level it is useful to be notified when stream writability changes.

Modifications:
- Add writabilityChanged to Http2RemoteFlowController.Listener
- Add isWritable to Http2RemoteFlowController

Result:
The Http2RemoteFlowController provides notification when writability of a stream changes.
2015-09-28 13:47:24 -07:00
nmittler
7ab132f28a Making HTTP/2 stream byte assignment pluggable
Motivation:

The DefaultHttp2RemoteFlowController has become very large and is getting difficult to understand and maintain. It is also desirable for some applications to be able to disable the priority algorithm altogether for performance reasons.

Modifications:

Abstract the stream byte assignment logic (renamed allocation->assignment for clarity) behind an interface `StreamByteAssigner` with a single implementation `PriorityStreamByteAssigner`.

Result:

Goes some way towards supporting #4246
2015-09-25 14:00:12 -07:00
Scott Mitchell
93011dd315 DefaultHttp2RemoteFlowController not allocating all available bytes
Motivation:
DefaultHttp2RemoteFlowController's allocation algorithm may not allocate all bytes that are available in the connection window. If the 'fair share' based upon weight is not fully used by sibling nodes it was not correctly re-distributed to other sibilings which may be able to utilize part / all of that share.

Modifications:
- Add a unit test which demonstrates the issue.
- Modify the allocation algorithm to ensure all available bytes are allocated.

Result:
Fixes https://github.com/netty/netty/issues/4266
2015-09-25 11:12:04 -07:00
Scott Mitchell
c372f69118 http2.HttpConversionUtil :authority conversion error
Motiviation:
The http2 spec https://tools.ietf.org/html/rfc7540#section-8.1.2.3 states that the :authority header should be copied into the HOST header when converting from HTTP/2 to HTTP/1.x. We currently have an extension header to preserve the authority.

Modifications:
- Remove AUTHORITY extension header
- HTTP/2 :authority should map to HOST header when converting to HTTP/1.x.

Result:
More spec compliant.
2015-09-23 17:06:52 -07:00
Scott Mitchell
ed4928f62a StreamBufferingEncoderTest leak
Motivation:
Buffer leak in StreamBufferingEncoderTest

Modifications:
- Make sure buffers are released in StreamBufferingEncoderTest

Result:
Fixes https://github.com/netty/netty/issues/4230
2015-09-23 16:49:39 -07:00
Scott Mitchell
edb91afcd6 Http2LifecycleManager.onException rename
Motivation:
Http2LifecycleManager.onException takes a Throwable as a paramter and not an Exception. There are also onConnectionError and onStreamError methods in the codec. We should rename this method to onError for consistency and clarity.

Modifications:
- Rename Http2LifecycleManager.onException to Http2LifecycleManager.onError

Result:
More consistent and clarified interface.
2015-09-23 16:48:36 -07:00
Scott Mitchell
24c9407080 DefaultHttp2RemoteFlowController may not write all pending bytes
Motivation:
DefaultHttp2RemoteFlowController attempts to write as many bytes as possible to transition the channel to not writable, and then relies on notification of channelWritabilityChange to continue writing. However the amount of bytes written by DefaultHttp2RemoteFlowController may not be the same number of bytes that is actually written to the channel due to other ChannelHandlers (SslHandler, compression, etc...) in the pipeline. This means there is a potential for the DefaultHttp2RemoteFlowController to be waiting for a channel writaiblity change event that will never come, and thus not write all queued data.

Modifications:
- DefaultHttp2RemoteFlowController should write pending bytes until there are no more, or until the channel is not writable.

Result:
DefaultHttp2RemoteFlowController will write all pending data.
Fixes https://github.com/netty/netty/issues/4242
2015-09-23 16:39:24 -07:00
nmittler
ec20902613 Don't set HTTP/2 flow controller ctx to null
Motivation:

We currently set the flow controller ChannelHandlerContexts to null when the channel becomes inactive. This is bad :)

Modifications:

Just remove that code in Http2ConnectionHandler

Result:

Fixes #4240
2015-09-22 07:25:42 -07:00
fratboy
6241bb059c [#4244] Convert urlencoded uri to http2 path correctly
Motivation:

HttpConversionUtil.toHttp2Headers does not convert urlencoded uri to http2 path properly.

Modifications:

Use getRawPath(), getRawQuery(), getRawFragment() in java.net.URI when converts to http2 path

Result:

HttpConversionUtil.toHttp2Headers does not urldecode uri unproperly.
2015-09-21 16:30:59 -07:00
Scott Mitchell
c7e3f6c6fd HTTP/2 defines using String instead of CharSequence
Motivation:
Http2CodecUtils has some static variables which are defined as Strings instead of CharSequence. One of these defines is used as a header name and should be AsciiString.

Modifications:
- Change the String defines in Http2CodecUtils to CharSequence

Result:
Types are more consistently using CharSequence and adding the upgrade header will require less work.
2015-09-16 14:55:33 -07:00
Scott Mitchell
1d4d5fe312 DefaultHttp2Headers should throw exception of type Http2Exception
Motivation:
The DefaultHttp2Headers code is throwing a IllegalArgumentException if an invalid character is detected. This is being ignored by the HTTP/2 codec instead of generating a GOAWAY.

Modifications:
- Throw a Http2Exception of type PROTOCOL_ERROR in accordance with https://tools.ietf.org/html/rfc7540#section-8.1.2.6
- Update examples which were building invalid headers

Result:
More compliant with https://tools.ietf.org/html/rfc7540#section-8.1.2.6
2015-09-16 13:47:05 -07:00
Scott Mitchell
15450af2e7 DefaultHttp2FrameWriter ping payload size check
Motivation:
The HTTP/2 spec states that the ping frame length must be 8 and is otherwise an error https://tools.ietf.org/html/rfc7540#section-6.7. The DefaultHttp2FrameReader enforces this, but the DefaultHttp2FrameWriter allows invalid frames to be written. We should not allow invalid ping frames to be written to the network.

Modifications:
- DefaultHttp2FrameWriter checks the frame size to be 8, or throws an exception

Result:
Fixes https://github.com/netty/netty/issues/3721
2015-09-16 10:25:07 -07:00
Scott Mitchell
59600f1812 HTTP/2 to HTTP/1.x headers conversion more accessible
Motivation:
Currently there is a HttpConversionUtil.addHttp2ToHttpHeaders which requires a FullHttpMessage, but this may not always be available. There is no interface that can be used with just Http2Headers and HttpHeaders.

Modifications:
- add an overload for HttpConversionUtil.addHttp2ToHttpHeaders which does not take FullHttpMessage

Result:
An overload for HttpConversionUtil.addHttp2ToHttpHeaders exists which does not require FullHttpMessage.
2015-09-16 10:02:48 -07:00
Scott Mitchell
ba11879c9f HTTP/2 codec heap buffer usage
Motivation:
The HTTP/2 codec has a few static buffers sent over the network which are allocated on the heap. This results in a copy operation when the buffer is sent out on the network.

Modifications:
- Ensure these static buffers are allocated using direct memory.

Result:
No copy operation necessary when writing static buffers to network.
2015-09-14 13:12:02 -07:00
Scott Mitchell
f89dfb0bd5 Deprecation cleanup for HTTP headers
Motivaion:
The HttpHeaders and DefaultHttpHeaders have methods deprecated due to being removed in future releases, but no replacement method to use in the current release. The deprecation policy should not be so aggressive as to not provide any non-deprecated method to use.

Modifications:
- Remove deprecated annotations and javadocs from methods which are the best we can do in terms of matching the master's api for 4.1

Result:
There should be non-deprecated methods available for HttpHeaders in 4.1.
2015-09-09 14:30:21 -07:00
Scott Mitchell
47726991b2 HTTP/2 Header Name Validation
Motivation:
The HTTP/2 header name validation was removed, and does not currently exist.

Modifications:
- Header name validation for HTTP/2 should be restored and set to the default mode of operation.

Result:
HTTP/2 header names are validated according to https://tools.ietf.org/html/rfc7540
2015-09-09 13:59:08 -07:00
Scott Mitchell
50d1f0a680 Http2Headers.iterator() comment correction
Motivation:
The javadoc comments on Http2Headers.iterator() are incorrect.

Modifications:
- Correct and clarify the javadoc for Http2Headers.iterator()

Result:
Javadoc for Http2Headers.iterator() is more correct.
2015-09-04 12:43:26 -07:00
Norman Maurer
34de2667c7 [maven-release-plugin] prepare for next development iteration 2015-09-02 11:45:20 +02:00
Norman Maurer
2eb444ec1d [maven-release-plugin] prepare release netty-4.1.0.Beta6 2015-09-02 11:36:11 +02:00
Scott Mitchell
41ee9148e5 HTTP/2 InboundHttp2ToHttpAdapterTest serverChannel NPE
Motivation:
InboundHttp2ToHttpAdapterTest.bootstrapEnv does not wait for the serverConnectedChannel to be initialized before returning. Some methods rely only this behavior and throw a NPE because it may not be set.

Modifications:
- Add a CountDownLatch to ensure the serverConnectedChannel is initialized

Result:
No more NPE.
2015-09-01 10:38:04 -07:00
Scott Mitchell
0736a3bc35 HTTP/2 SimplePromiseAggregator tryFailure not consistent with setFailure
Motivation:
The SimplePromiseAggregator.setFailure allows a failure to occur before newPromise is called, but tryFailure doesn't. These methods should be consistent.

Modifications:
- tryFailure should use the same logic as setFailure

Result:
Consistent failure routines.
2015-09-01 10:35:10 -07:00
Scott Mitchell
50cc647804 DefaultPropertyKey private member variable accessed outside scope
Motivation:
DefaultPropertyKey.index is currently private and accessed outside the class's scope.

Modifications:
- Change access level to package private

Result:
No chance of synthetic method generation for accessing this field
2015-08-28 08:54:11 -07:00
Scott Mitchell
0365927951 HTTP/2 InboundHttp2ToHttpAdapterTest race condition
Motivation:
The latches in InboundHttp2ToHttpAdapterTest were volatile and reset during the tests. This resulted in race conditions and sometimes the tests would be waiting on old latches that were not the same latches being counted down when messages were received.

Modifications:
- Remove volatile latches from tests

Result:
More reliable tests with less race conditions.
2015-08-28 08:50:08 -07:00
Scott Mitchell
b6a4f5de9d Refactor of HttpUtil and HttpHeaderUtil
Motivation:
There currently exists http.HttpUtil, http2.HttpUtil, and http.HttpHeaderUtil. Having 2 HttpUtil methods can be confusing and the utilty methods in the http package could be consolidated.

Modifications:
- Rename http2.HttpUtil to http2.HttpConversionUtil
- Move http.HttpHeaderUtil methods into http.HttpUtil

Result:
Consolidated utilities whose names don't overlap.
Fixes https://github.com/netty/netty/issues/4120
2015-08-27 08:49:58 -07: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
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
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
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
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
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
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
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
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
Scott Mitchell
7bd6472804 HTTP/2 DataCompressionHttp2Test test complete race condition
Motivation:
The DataCompressionHttp2Test was exiting prematurely leading to unit test failures.

Modifications:
- Fix the race condition so the test does not evaluate final conditions until all expected events occur

Result:
Unit test no longer fails
2015-08-04 13:57:40 -07:00
nmittler
94f65ed7ff Use standard syntax for logging HTTP/2 stream ID.
Motivation:

When looking through the logs for entries pertaining to a specific stream, it's difficult because header entries use the syntax "streamId:<id>" but all other entries use "streamId=<id>". We should make all of the entries consistent.

Modifications:

Changed header entries to use "streamId=<id>" to match the other entries.

Result:

Easier HTTP/2 log navigation.
2015-07-28 09:54:14 -07:00
nmittler
296649cfc8 Make PrimitiveCollections generated for all primitive maps.
Motivation:

We should support XXXCollections methods for all primitive map types.

Modifications:

Removed PrimitiveCollections and added a template for XXXCollections.

Result:

Fixes #4001
2015-07-27 06:59:23 -07:00
nmittler
93fc3c6e45 Make IntObjectHashMap extend Map
Motivation:

It would be useful to support the Java `Map` interface in our primitive maps.

Modifications:

Renamed current methods to "pXXX", where p is short for "primitive". Made the template for all primitive maps extend the appropriate Map interface.

Result:

Fixes #3970
2015-07-22 15:52:27 -07:00
Brendt Lucas
57d28dd421 Support conversion of HttpMessage and HttpContent to HTTP/2 Frames
Motivation:

HttpToHttp2ConnectionHandler only converts FullHttpMessage to HTTP/2 Frames. This does not support other use cases such as adding a HttpContentCompressor to the pipeline, which writes HttpMessage and HttpContent.

Additionally HttpToHttp2ConnectionHandler ignores converting and sending HTTP trailing headers, which is a bug as the HTTP/2 spec states that they should be sent.

Modifications:

Update HttpToHttp2ConnectionHandler to support converting HttpMessage and HttpContent to HTTP/2 Frames.
Additionally, include an extra call to writeHeaders if the message includes trailing headers

Result:

One can now write HttpMessage and HttpContent (http chunking) down the pipeline and they will be converted to HTTP/2 Frames.  If any trailing headers exist, they will be converted and sent as well.
2015-07-21 20:17:27 +02:00
nmittler
0d73907c58 Fixing NPE in StreamBufferingEncoderTest
Motivation:

The bufferingNewStreamFailsAfterGoAwayReceived method currently causes an NPE.

Modifications:

Fixed the test so that a valid ByteBuf is passed in.

Result:

The test no longer throws an NPE.
2015-07-20 13:47:26 -07:00
Scott Mitchell
74dd7f85ca HTTP/2 Thread Context Interface Clarifications
Motivation:
It is currently assumed that all usages of the HTTP/2 codec will be from the same event loop context. If the methods are used outside of the assumed thread context then unexpected behavior is observed. This assumption should be more clearly communicated and enforced in key areas.

Modifications:
- The flow controller interfaces have assert statements and updated javadocs indicating the assumptions.

Result:
Interfaces more clearly indicate thread context limitations.
2015-07-17 12:40:36 -07:00
Scott Mitchell
d7cdc469bc HTTP/2 CompressorHttp2ConnectionEncoder bug
Motivation:
The CompressorHttp2ConnectionEncoder is attempting to attach a property to streams before the exist.

Modifications:
- Allow the super class to create the streams before attempting to attach a property to the stream.

Result:
CompressorHttp2ConnectionEncoder is able to set the property and access the compressor.
2015-07-17 09:59:55 -07:00
Scott Mitchell
9747ffe5fc HTTP/2 Flow Controller should use Channel.isWritable()
Motivation:
See #3783

Modifications:
- The DefaultHttp2RemoteFlowController should use Channel.isWritable() before attempting to do any write operations.
- The Flow controller methods should no longer take ChannelHandlerContext. The concept of flow control is tied to a connection and we do not support 1 flow controller keeping track of multiple ChannelHandlerContext.

Result:
Writes are delayed until isWritable() is true. Flow controller interface methods are more clear as to ChannelHandlerContext restrictions.
2015-07-16 14:38:48 -07:00
Ryo Okubo
aaba1b9ed5 Accept over 2^31-1 MAX_HEADER_LIST_SIZE
Motivation:

The MAX_HEADER_LIST_SIZE of SETTINGS is represented by
unsigned 32-bit value and this value isn't limited in RFC7540.
But in current implementation, its stored to int variable so
over 2^31-1 value is recognized as minus and handled as
PROTOCOL_ERROR.

Modifications:

If a value of MAX_HEADER_LIST_SIZE is larger than 2^31-1, its
handled as 2^31-1

Result:

Over 2^31-1 MAX_HEADER_LIST_SIZE is became acceptable
2015-07-10 10:58:41 -07:00
Louis Ryan
60c59f39af Use CoalescingBufferQueue to merge data writes on a stream in HTTP2 instead of CompositeByteBuf
Motivation:

Slicing a mutable CompositeByteBuf is not the appropriate mechanism to use to track and release buffers that have been written to a channel.
In particular buffers passed over an Embedded or LocalChannel are retained after the ChannelPromise is completed and listening to the
promise to consolidate a CompositeBuffer breaks slices taken from the composite as the offset indices have changed.

In addition CoalescingBufferQueue handles taking arbitrarily sized slices of a sequence of buffers more efficiently.

Modifications:

Convert FlowControlledData to use a CoalescingBufferQueue to handle merging data writes.

Result:

HTTP2 works over LocalChannel and code is considerably simpler.
2015-07-09 10:47:18 -07:00
zhangduo
e949dcd94f Allow numBytes == 0 when calling Http2LocalFlowController.consumeBytes.
Motivation:

Sometimes people use a data frame with length 0 to end a stream(such as jetty http2-server). So it is possible that data.readableBytes and padding are all 0 for a data frame, and cause an IllegalArgumentException when calling flowController.consumeBytes.

Modifications:

Return false when numBytes == 0 instead of throwing IllegalArgumentException.

Result:

Fix IllegalArgumentException.
2015-07-09 08:43:52 -07:00
nmittler
6e044b082c Proper shutdown of HTTP2 encoder when channelInactive
Motivation:

The problem is described in https://github.com/grpc/grpc-java/issues/605. Basically, when using `StreamBufferingEncoder` there is a chance of creating zombie streams that never get closed.

Modifications:

Change `Http2ConnectionHandler`'s `channelInactive` handling logic to shutdown the encoder/decoder before shutting down the active streams.

Result:

Fixes https://github.com/grpc/grpc-java/issues/605
2015-07-09 07:36:42 -07:00
Norman Maurer
61b9da470a [#3945] Http2ConnectionHandler breaks channelReadComplete pipeline notification
Motivation:

Http2ConnectionHandler missed to forward channelReadComplete(...) events.

Modifications:

Ensure we notify the next handler in the pipeline via ctx.fireChannelReadComplete().

Result:

Correctly forwarding of event.
2015-07-08 08:04:02 +02:00
Scott Mitchell
ecacd11b06 HTTP/2 limit header accumulated size
Motivation:
The Http2FrameListener requires that the Http2FrameReader accumulate ByteBuf objects. There is currently no way to limit the amount of bytes that is accumulated.

Motiviation:
- DefaultHttp2FrameReader supports maxHeaderSize which will fail fast as soon as the maximum size is exceeded.
- DefaultHttp2HeadersDecoder will respect the return value of endHeaderBlock() and fail if the max size is exceeded.

Result:
Frames which carry header data now respect a maximum number of bytes that can be accumulated.
2015-07-07 13:25:03 -07:00