Commit Graph

45 Commits

Author SHA1 Message Date
Scott Mitchell
970529e1a8 HTTP/2 Priority tree circular link
Motivation:
If an exclusive dependency change stream B should be an exclusive dependency of stream A is requested and stream B is already a child of stream A...then we will add B to B's own children map and create a circular link in the priority tree. This leads to an infinite recursive loop and a stack overflow exception.

Modifications:
-when removeAllChildren is called it should not remove the exclusive dependency.
-unit test to ensure this case is covered.

Result:
No more circular link in the priority tree.
2015-04-15 14:26:05 -07:00
Scott Mitchell
e36c1436b8 ByteString misses encountered during forward port
Motivation:
While forward porting https://github.com/netty/netty/pull/3579 there were a few areas that had not been previously back ported.

Modifications:
Backport the missed areas to ensure consistency.

Result:
More consistent 4.1 and master branches.
2015-04-14 17:11:09 -07:00
Scott Mitchell
9a7a85dbe5 ByteString introduced as AsciiString super class
Motivation:
The usage and code within AsciiString has exceeded the original design scope for this class. Its usage as a binary string is confusing and on the verge of violating interface assumptions in some spots.

Modifications:
- ByteString will be created as a base class to AsciiString. All of the generic byte handling processing will live in ByteString and all the special character encoding will live in AsciiString.

Results:
The AsciiString interface will be clarified. Users of AsciiString can now be clear of the limitations the class imposes while users of the ByteString class don't have to live with those limitations.
2015-04-14 16:35:17 -07:00
nmittler
c388f3f085 Removing Http2StreamRemovalPolicy
Motivation:

Due to a recent flurry of cleanup and fixes, we no longer need the stream removal policy to protect against recently removed streams. We should get rid of it.

Modifications:

Removed Http2StreamRemovalPolicy and everywhere it's used.

Result:

Fixes #3448
2015-04-13 12:18:59 -07:00
Scott Mitchell
4a79c5899c HTTP/2 Connection Listener Unchecked Exceptions
Motivation:
The DefaultHttp2Connection is not checking for RuntimeExceptions when invoking Http2Connection.Listener methods. This is a problem for a few reasons: 1. The state of DefaultHttp2Connection will be corrupted if a listener throws a RuntimeException. 2. If the first listener throws then no other listeners will be notified, which may further corrupt state that is updated as a result of listeners being notified.

Modifications:
- Document that RuntimeExceptions are not supported for Http2Connection.Listener methods, and will be logged as an error.
- Update DefaultHttp2Connection to handle and exception for each listener that is notified, and be sure that 1 listener throwing an exception does not prevent others from being notified.

Result:
More robust DefaultHttp2Connection.
2015-04-13 08:54:14 -07:00
nmittler
a87c86dc0d Change Http2Settings to use char keys.
Motivation:

Now that we have a CharObjectHashMap, we should change Http2Settings to use it.

Modifications:

Changed Http2Settings to extend CharObjectHashMap rather than IntObjectHashMap.

Result:

Http2Settings uses less memory to store keys.
2015-04-10 11:50:24 -07:00
nmittler
e3374e5b1d Removing direct access to HTTP/2 child streams.
Motivation:

We've removed access to the activeStreams collection, we should do the same for the children of a stream to provide a consistent interface.

Modifications:

Moved Http2StreamVisitor to a top-level interface. Removed unnecessary child operations from the Http2Stream interface so that we no longer require a map structure.

Result:

Cleaner and more consistent interface for iterating over child streams.
2015-04-10 08:52:26 -07:00
Scott Mitchell
83ce8a9187 HTTP/2 Prevent modification of activeStreams while iterating
Motivation:
The Http2Connection interface exposes an activeStreams() method which allows direct iteration over the underlying collection. There are a few places that make copies of this collection to avoid modification while iterating, and a few places that do not make copies. The copy operation can be expensive on hot code paths and also we are not consistently iterating over the activeStreams collection.

Modifications:
- The Http2Connection interface should reduce the exposure of the underlying collection and just expose what is necessary for the interface to function.  This is just a means to iterate over the collection.
- The DefaultHttp2Connection should use this new interface and protect it's internal state while iteration is occurring.

Result:
Reduction in surface area of the Http2Connection interface.  Consistent iteration of the set of active streams.  Concurrent modification exceptions are handled in 1 encapsulated spot.
2015-04-07 20:55:48 -07:00
Jakob Buchgraber
d5d932a739 Fix GOAWAY logic in Http2Encoder and Http2Decoder.
Motivation:

1) The current implementation doesn't allow for HEADERS, DATA, PING, PRIORITY and SETTINGS
   frames to be sent after GOAWAY.

2) When receiving or sending a GOAWAY frame, all streams with ids greater than the lastStreamId
   of the GOAWAY frame should be closed. That's not happening.

Modifications:

1) Allow sending of HEADERS and DATA frames after GOAWAY for streams with ids < lastStreamId.
2) Always allow sending PING, PRIORITY AND SETTINGS frames.
3) Allow sending multiple GOAWAY frames with decreasing lastStreamIds.
4) After receiving or sending a GOAWAY frame, close all streams with ids > lastStreamId.

Result:

The GOAWAY handling is more correct.
2015-04-07 20:32:28 -07:00
Scott Mitchell
3ae343b768 HTTP/2 DefaultHttp2Connection recursive call fix
Motivation:
There are methods to manipulate the prioritzable count for streams which have the '0' postfix which are designed to be used during recursion.  However these methods are calling out to an external method without the '0' during the recursive process.  This is doing uneccessary conditional checks during recursion.

Modifications:
Change the decrementPrioritizableForTree to decrementPrioritizableForTree0 while in recursive method.
Change the incrementPrioritizableForTree to incrementPrioritizableForTree0 while in recursive method.

Result:
Less overhead during recursive calls.
2015-04-06 17:12:41 -07:00
Scott Mitchell
86edc88448 HTTP/2 LifecycleManager and Http2ConnectionHandler interface clarifications
Motiviation:
The interface provided by Http2LifecycleManager is not clear as to how the writeXXX methods should behave.  The implementation of this interface from the Http2ConnectionHandler's perspecitve is unclear what writeXXX means in this context.

Modifications:
- Method names in Http2LifecycleManager and Http2ConnectionHandler should be renamed and comments should clarify the interfaces.

Results:
Http2LifecycleManager is more clear and Http2ConnectionHandler's implementation makes sense w.r.t to return values.
2015-04-06 14:34:20 -07:00
Scott Mitchell
609e065fcf HTTP/2 Headers Code Using String instead of AsciiString
Motivation:
The HTTP/2 headers code should be using binary string (currently AsciiString) objects instead of String objects. The DefaultHttp2HeadersEncoder was still using String for sensitiveHeaders.

Modifications:
- Remove the usage of String from DefaultHttp2HeadersEncoder.
- Introduce an interface to determine if a header name/value is sensitive or not to 1. prevent necessarily creating/copying sets. 2. Allow the name/value to be considered when checking if sensitive.

Result:
No more String in DefaultHttp2HeadersEncoder and less required set creation/operations.
2015-04-03 15:55:44 -07:00
Scott Mitchell
9517edd498 HTTP/2 RST_STREAM in IDLE
Motivation:
The spec requires that a RST_STREAM received on an IDLE stream results in a connection error. This is not happening.

Modifications:
Check for this condition when a RST_STREAM is received in DefaultHttp2ConnectionDecoder.

Result:
More spec compliant.  Fixes https://github.com/netty/netty/issues/3573.
2015-04-03 15:53:32 -07:00
Scott Mitchell
e5d01c4caf HTTP/2 HEADERS stream dependency fix
Motivation:
The DefaultHttp2ConnectionDecoder has the setPriority call after the Http2FrameListener is notified of the change. The setPriority call has additional verification logic and may even create the dependency stream and so it must be before the Http2FrameListener is notified.

Modifications:
The DefaultHttp2ConnectionDecoder should treat the setPriority call in the same for the HEADERS and PRIORITY frame (call it before notifying the listener).

Result:
Http2FrameListener should see correct state when a HEADERS frame has a stream dependency that has not yet been created yet.  Fixes https://github.com/netty/netty/issues/3572.
2015-04-03 15:50:30 -07:00
Jakob Buchgraber
e40c27d9ed Avoid object allocations for HTTP2 child streams.
Motivation:

We are allocating a hash map for every HTTP2 Stream to store it's children.
Most streams are leafs in the priority tree and don't have children.

Modification:

 - Only allocate children when we actually use them.
 - Make EmptyIntObjectMap not throw a UnsupportedOperationException on remove, but return null instead (as is stated in it's javadoc).

Result:

Fewer unnecessary allocations.
2015-04-03 11:57:31 -07:00
Jakob Buchgraber
35b9aa9302 Replace LinkedHashSet by ArrayList to avoid iterators.
Motivation:

In a simple load test that creates and closes several 10k streams per second
I have seen Iterator objects using roughly 1.6% of the total committed heap.

Modifications:

Use an ArrayList instead of a LinkedHashSet to store the connection listeners.
That way we can iterate over the list without creating an iterator every time.

Result:

Zero Iterator allocations due to notifying connection listeners.
2015-04-03 20:13:53 +02:00
nmittler
ef729e7021 Allow non-standard HTTP/2 settings
Motivation:

The Http2Settings class currently disallows setting non-standard settings, which violates the spec.

Modifications:

Updated Http2Settings to permit arbitrary settings. Also adjusting the default initial capacity to allow setting all of the standard settings without reallocation.

Result:

Fixes #3560
2015-04-02 11:10:47 -07:00
Scott Mitchell
7f2ddb2162 HTTP/2 Closed Streams Conditional Priority Tree Removal
Motivation:
The HTTP/2 specification allows for closed (and streams in any state) to exist in the priority tree. The current code removes streams from the priority tree as soon as they are closed (subject to the removal policy). This may lead to undesired distribution of resources from the peer's perspective.

Modifications:
- We should only remove streams from the priority tree when they have no descendant streams in a viable state.
- We should track when tree edges change or nodes are removed if inviable nodes can then be removed.

Result:
Priority tree doesn't remove closed streams until descendant are all closed, or there are no descendants.
2015-03-31 16:24:25 -07:00
nmittler
ba9f214303 Removing unnecessary use of Math.ceil in HTTP/2 priority algorithm.
Motivation:

We're currently using Math.ceil which isn't necessary. We should exchange for a lighter weight operation.

Modifications:

Changing the logic to just ensure that we allocate at least one byte to the child rather than always performing a ceil.

Result:

Slight performance improvement in the priority algorithm.
2015-03-31 11:03:16 -07:00
nmittler
9737cc6cc9 Include error code and message in GOAWAY events.
Motivation:

The Connection.Listener GOAWAY event handler currently provides no additional information, requiring applications to hack in other ways to get at the error code and debug message.

Modifications:

Modified the Connection.Listener interface to pass on the error code and message that triggered the GOAWAY.

Result:

Application can now use Connection.Listener for all GOAWAY processing.
2015-03-31 09:18:26 -07:00
nmittler
6fbca14f8a Cleaning up the initialization of Http2ConnectionHandler
Motivation:

It currently takes a builder for the encoder and decoder, which makes it difficult to decorate them.

Modifications:

Removed the builders from the interfaces entirely. Left the builder for the decoder impl but removed it from the encoder since it's constructor only takes 2 parameters. Also added decorator base classes for the encoder and decoder and made the CompressorHttp2ConnectionEncoder extend the decorator.

Result:

Fixes #3530
2015-03-30 11:23:02 -07:00
nmittler
cb63e34bda Removing unnecessary sort in remote flow controller.
Motivation:

The DefaultHttp2RemoteFlowController's priority algorithm doesn't really need to sort the children by weight since it already fairly distributes data based on weight.

Modifications:

Removing the sorting in the priority algorithm and updating one test to allow a small bit of variability in the results.

Result:

Slight improvement on the performance of the priority algorithm.
2015-03-30 09:55:03 -07:00
Scott Mitchell
ab74dccd23 Http/2 Priority on CLOSED stream
Motivation:
The encoder/decoder currently do not handle streams which have previously existed but no longer exist because they were closed. The specification requires supporting this.

Modifications:
- encoder/decoder should tolerate the frame or the dependent frame not existing in the streams map due to the fact that it may have previously existed.

Result:
encoder/decoder are more compliant with the specification.
2015-03-28 19:10:43 -07:00
Scott Mitchell
0d3a6e0511 HTTP/2 Decoder reduce preface conditional checks
Motivation:
The DefaultHttp2ConnectionDecoder class is calling verifyPrefaceReceived() for almost every frame event at all times.
The Http2ConnectionHandler class is calling readClientPrefaceString() on every decode event.

Modifications:
- DefaultHttp2ConnectionDecoder should not have to continuously call verifyPrefaceReceived() because it transitions boolean state 1 time for each connection.
- Http2ConnectionHandler should not have to continuously call readClientPrefaceString() because it transitions boolean state 1 time for each connection.

Result:
- Less conditional checks for the mainstream usage of the connection.
2015-03-28 18:52:35 -07:00
nmittler
bb059c070f Decoupling allocation from writing in HTTP/2 outbound flow control
Motivation:

The current DefaultHttp2RemoteFlowController's writePendingBytes currently operates in 2 passes. The first allocates bytes and optionally writes some frames. The second pass just loops across all active streams and writes all remaining bytes.

If streams can be removed/added as a side effect of writing (EOS or error) then we need to take more care when the write actually occurs. Moving all of the writes to the second loop (across active streams) is simpler since we can just make a copy of the list and not worry about any restructuring of the priority tree that may result.

Modifications:

Modified DefaultHttp2RemoteFlowController.writePendingBytes to only allocate bytes on the first pass and then write any allocated bytes on the second pass.

Result:

Side effects resulting from writing should no longer impact the flow control algorithm.
2015-03-28 14:19:37 -07:00
scottmitch
2dda917f27 Http2DefaultFrameWriter microbenchmark
Motivation:
A microbenchmark will be useful to get a baseline for performance.

Modifications:
- Introduce a new microbenchmark which tests the Http2DefaultFrameWriter.
- Allow benchmarks to run without thread context switching between JMH and Netty.

Result:
Microbenchmark exists to test performance.
2015-03-27 13:10:57 -07:00
Scott Mitchell
23f881b382 Comment punctuation cleanup
Motivation:
Commit d857b16d76 introduced some comments that had no punctuation.

Modifications:
Add punctuation.

Result:
Comments have punctuation.
2015-03-26 13:01:37 -07:00
Scott Mitchell
d857b16d76 Http/2 RST_STREAM frame robustness
Motivation:
The Http2ConnectionHandler writeRstStream method allows RST_STREAM frames to be sent when we do not know about the stream and after a RST_STREAM frame has already been sent.  This may lead to sending frames when we should not according to the HTTP/2 spec. There is also the potential to notify the closeListener multiple times if the closeStream method is called multiple times.

Modifications:
- Prevent RST_STREAM from being sent if we don't know about the stream, or if we already sent the RST_STREAM.
- Prevent the closeListener from being notified multiple times.

Result:
More robust writeRstStream logic in boundary conditions.
2015-03-26 11:47:00 -07:00
Scott Mitchell
6dfa1f2d92 Http2 draft 17
Motivation:
There was a new draft for HTTP/2.  We should support the new draft.

Modifications:
- Review the HTTP/2 draft 17 specification, and update code to reflect changes.

Result:
Support for HTTP/2 draft 17.
2015-03-25 09:02:15 -07:00
Jakob Buchgraber
8e04d706de Have FlowState.cancel take a Throwable and code cleanup.
Motivation:

- In FlowState.write(...) we are currently swalloing an exception.
- In my previous commit I introduced a compiler warning by not making
  a local variabe final.

Modifications:

- Have FlowState.cancel() take a Throwable.
- Make the variable final.

Result:

No more swallowed exceptions and warnings.
2015-03-19 22:01:02 -07:00
Jakob Buchgraber
9bad408de5 HTTP2: Close encoder and decoder on channelInactive and initialize clientPrefaceString on handlerAdded.
Motivation:

- The encoder and decoder should be closed right after the handler releases its resources.
- The clientPrefaceString is allocated in the constructor but releases in handlerRemoved.
  If the handler is never added to the pipeline, the clientPrefaceString will never be
  released.

Modifications:

- Call encoder.close() and decoder.close() on channelInactive.
- Release the clientPrefaceString on handlerRemoved.

Result:

- The encoder and decoder get closed right after the handler's resources are freed.
- It's easier to verify that the clientPrefaceString will also get released.
2015-03-19 13:09:34 -07:00
nmittler
0fe67cfba5 Using public LogLevel for HTTP/2 frame logging.
Motivation:

The Http2FrameLogger is currently using the internal logging classes. We should change this so that it's using the public classes and then converts internally.

Modifications:

Modified Http2FrameLogger and the examples to use the public LogLevel class.

Result:

Fixes #2512
2015-03-17 15:10:35 -07:00
nmittler
c91eaace5e Cleaning up HTTP/2 method names for max_concurrent_streams
Motivation:

The current documentation for Endpoint methods referring to concurrent streams and the SETTINGS_MAX_CONCURRENT_STREAMS setting are a bit confusing.

Modifications:

Renamed a few of the methods and added more clear documentation.

Result:

Fixes #3451
2015-03-16 10:17:39 -07:00
Jakob Buchgraber
fe12d08efe Remove dead code from DefaultHttp2ConnectionEncoder.
Motivation:

There are two writeRstStream methods in the DefaultHttp2ConnectionEncoder.
One of the two is neither used nor part of the Http2FrameWriter interface.

Modifications:

Delete the method.

Result:

Fewer lines of dead code.
2015-03-14 09:02:45 -07:00
Jakob Buchgraber
d5963e069d Remove Frame class from DefaultHttp2RemoteFlowController. Fixes #3488
Motivation:

For every write of a flow controlled frame (HEADERS, DATA) we are allocating
a Frame object that is not necessary anymore as it does not maintain any
state, besides the payload.

Modifications:

Remove the Frame class and directly add the payload to the pending write queue.

Result:

One few object allocation per write of a flow controlled frame.
2015-03-13 15:54:40 -07:00
nmittler
44615f6cb2 Optimizations for Http2FrameLogger
Motivation:

The logger was always performing a hex dump of the ByteBufs regarless whether or not the log would take place.

Modifications:

Fixed the logger to avoid serializing the ByteBufs and calling the varargs method if logging is not enabled.

Result:

The loggers should run MUCH faster when disabled.
2015-03-12 14:20:05 -07:00
Jakob Buchgraber
88beae6838 Fix premature cancelation of pending frames in HTTP2 Flow Control.
Motivation:

If HEADERS or DATA frames are pending due to a too small flow control
window, a frame with the END_STREAM flag set will wrongfully cancel
all pending frames (including itself).

Also see grpc/grpc-java#145

Modifications:

The transition of the stream state to CLOSE / HALF_CLOSE due to a
set END_STREAM flag is delayed until the frame with the flag is
actually written to the Channel.

Result:

Flow control works correctly. Frames with END_STREAM flag will no
longer cancel their preceding frames.
2015-03-10 12:35:13 -07:00
Norman Maurer
fce0989844 [maven-release-plugin] prepare for next development iteration 2015-03-03 02:06:47 -05:00
Norman Maurer
ca3b1bc4b7 [maven-release-plugin] prepare release netty-4.1.0.Beta4 2015-03-03 02:05:52 -05:00
nmittler
daba2b3313 Removing debugging change from unit test.
Motivation:

HttpToHttp2ConnectionHandlerTest was accidentally modified with a
debugging value for WAIT_TIME_SECONDS.

Modifications:

Reverted the change.

Result:

original wait time restored.
2015-02-11 09:07:08 -08:00
Scott Mitchell
8b5f2d7716 Http2DefaultFrameWriter direct write instead of copy
Motivation:
The Http2DefaultFrameWriter copies all contents into a buffer (or uses a CompositeBuffer in 1 case) and then writes that buffer to the socket. There is an opportunity to avoid the copy operations and write directly to the socket.

Modifications:
- Http2DefaultFrameWriter should avoid copy operations where possible.
- The Http2FrameWriter interface should be clarified to indicate that ByteBuf objects will be released.

Result:
Hopefully less allocation/copy leads to memory and throughput performance benefit.
2015-02-06 11:56:14 -08:00
nmittler
bc76bfa199 Consolidating HTTP/2 stream state
Motivation:

Http2Stream has several methods that provide state information. We need
to simplify how state is used and consolidate as many of these fields as
possible.

Modifications:

Since we already have a concept of a stream being active or inactive,
I'm now separating the deactivation of a stream from the act of closing
it.  The reason for this is the case of sending a frame with
endOfStream=true. In this case we want to close the stream immediately
in order to disallow further writing, but we don't want to mark the
stream as inactive until the write has completed since the inactive
event triggers the flow controller to cancel any pending writes on the
stream.

With deactivation separated out, we are able to eliminate most of the
additional state methods with the exception of `isResetSent`.  This is
still required because we need to ignore inbound frames in this case (as
per the spec), since the remote endpoint may not yet know that the
stream has been closed.

Result:

Fixes #3382
2015-02-04 11:53:00 -08:00
louiscryan
8bbfcb05a0 Make flow-controller a write-queue for HEADERS and DATA
Motivation:

Previously flow-controller had to know the implementation details of each frame type in order to write it correctly. That concern is more correctly handled by the encoder. By encapsulating the payload types to be flow-controlled it will be easier to add support for extension types later. This change also fixes #3353.

Modifications:

Add interface FlowControlled which is now delivered to flow-controller.
Implement this interface for HEADERS and DATA
Refactor and improve tests for flow-control.

Result:

Flow control semantics are more cleanly separated for data encoding and implementation is simpler overall.
2015-02-02 10:00:14 -05:00
Adrian Cole
c6bfc92df1 Zero length data frames should apply flow control.
Motivation:
A downstream consumer of Netty failed as emitting zero-length http2 data frames in a unit test resulted in assertion errors in Http2LocalFlowController. Since zero-length frames are valid, an assertion that http2 data frame length must be positive is invalid.

Modifications:

Assertions of data length in Http2LocalFlowController now permit zero.

Result:

Those running netty with assertions on can now emit zero length http2 data frames.
2015-01-23 11:07:46 -05:00
Nitesh Kant
2d24e1f27d Back port HTTP/2 codec from master to 4.1
Motivation:

HTTP/2 codec was implemented in master branch.
Since, master is not yet stable and will be some time before it gets released, backporting it to 4.1, enables people to use the codec with a stable netty version.

Modification:

The code has been copied from master branch as is, with minor modifications to suit the `ChannelHandler` API in 4.x.
Apart from that change, there are two backward incompatible API changes included, namely,

- Added an abstract method:

  `public abstract Map.Entry<CharSequence, CharSequence> forEachEntry(EntryVisitor<CharSequence> visitor)
            throws Exception;`

to `HttpHeaders` and implemented the same in `DefaultHttpHeaders` as a delegate to the internal `TextHeader` instance.

- Added a method:

`FullHttpMessage copy(ByteBuf newContent);`

in `FullHttpMessage` with the implementations copied from relevant places in the master branch.

- Added missing abstract method related to setting/adding short values to `HttpHeaders`

Result:

HTTP/2 codec can be used with netty 4.1
2015-01-23 11:06:11 -05:00