Commit Graph

112 Commits

Author SHA1 Message Date
Scott Mitchell
7f9fb95702 HTTP/2 Unit Test race condition
Motivation:
The Http2ConnectionRoundtripTest.noMoreStreamIdsShouldSendGoAway unit test had a race condition where it would sometimes receive a SETINGS_ACK message that was not anticipated. This caused the test to fail because of bad test code.

Modifications:
The bad unit test should be updated to handle the message exchange for a good connection setup, and then the GO_AWAY frame.

Result:
Http2ConnectionRoundtripTest.noMoreStreamIdsShouldSendGoAway should no longer sporadically fail.
2014-12-09 16:24:21 -05:00
nmittler
124983afb5 Refactoring HTTP/2 Flow Control interfaces.
Motivation:

The terminology used with inbound/outbound is a little confusing since
it's not discussed in the spec. We should switch to using local/remote
instead. Also there is some asymmetry between the inbound/outbound
interfaces which could probably be cleaned up.

Modifications:

Changing the interface names and making a common Http2FlowController
interface for most of the methods.

Result:

The HTTP/2 flow control interfaces should be more clear.
2014-12-08 09:16:06 -08:00
Scott Mitchell
2d10b252f9 HTTP/2 Inbound Flow Control Connection Window Issues
Motivation:
The inbound flow control code was returning too many bytes to the connection window.  This was resulting in GO_AWAYs being generated by peers with the error code indicating a flow control issue.  Bytes were being returned to the connection window before the call to returnProcessedBytes. All of the state representing the connection window was not updated when a local settings event occurred.

Modifications:
The DefaultHttp2InboundFlowController will be updated to correct the above defects.
The unit tests will be updated to reflect the changes.

Result:
Inbound flow control algorithm does not cause peers to send flow control errors for the above mentioned cases.
2014-12-04 14:10:11 -05:00
Trustin Lee
078072632a Fix dependency issues with hamcrest
Motivation:

We use 3 (!) libraries to build mock objects - easymock, mockito, jmock.
Mockito and jMock pulls in the different versions of Hamcrest, and it
conflicts with the version pulled by jUnit.

Modifications:

- Replace mockito-all with mockito-core to avoid pulling in outdated
  jUnit and Hamcrest
- Exclude junit-dep when pulling in jmock-junit4, because it pulls an
  outdated Hamcrest version
- Pull in the hamcrest-library version used by jUnit explicitly

Result:

No more dependency hell that results in NoSuchMethodError during the
tests
2014-12-04 17:53:35 +09:00
Scott Mitchell
deb815f6cb HTTP/2 Prohibitied Cihpers Allowed
Motivation:
The Http2SecurityUtil class lists a few ciphers that are explicitly prohibited by the HTTP/2 specification because of their characteristics.

Modifications:
Remove the ciphers that are prohibited.

Results:
Cipher suite used for HTTP/2 codec is compatible with HTTP/2 spec.
2014-11-24 19:06:14 -05:00
Scott Mitchell
198f8fa95e HTTP/2 throw statement missed
Motivation:
The DefaultHttp2FrameWriter has an exception generated but is missing the throw keyword.

Modifications:
Insert the missing throw keyword.

Result:
Exception thrown when it was intended to be thrown.
2014-11-23 20:45:47 -05:00
Scott Mitchell
a8e5fb12fa HTTP/2 Draft 15
Motivation:
A new draft of the HTTP/2 spec has been released.

Modifications:
Make updates as defined in https://tools.ietf.org/html/draft-ietf-httpbis-http2-15

Result:
HTTP/2 codec is draft 15 compliant.
2014-11-23 13:00:00 -05:00
nmittler
c9e5238ea6 HTTP/2 inbound flow control requests too many bytes in WINDOW_UPDATE.
Motivation:
The DefaultHttp2InboundFlowController uses processedBytes to determine
when to send the WINDOW_UPDATE, but uses window to determine the delta
to send in the request. This is incorrect since we shouldn't be
requesting bytes that haven't been processed.

Modifications:
Changed DefaultHttp2InboundFlowController to use processedBytes in the
calculation of the delta to send in the WINDOW_UPDATE request.

Result:
Inbound flow control only asks for bytes that have been processed in
WINDOW_UPDATE.
2014-11-21 08:15:27 -08:00
Trustin Lee
87537ce397 Add HttpStatusClass
Related: #3157

Motivation:

It should be convenient to have an easy way to classify an
HttpResponseStatus based on the first digit of the HTTP status code, as
defined in the RFC 2616:

- Information 1xx
- Success 2xx
- Redirection 3xx
- Client Error 4xx
- Server Error 5xx

Modification:

- Add HttpStatusClass
- Add HttpResponseStatus.codeClass() that returns the class of the HTTP
  status code
- Remove HttpResponseStatus.isInformational()

Result:

It's easier to determine the class of an HTTP status
2014-11-21 10:54:52 +09:00
Scott Mitchell
dc9ca1a2b8 HTTP/2 Compressor buffer leak
Motivation:
The HTTP/2 compressor does not release the input buffer when compression is done. This results in buffer leaks.

Modifications:
- Release the buffer in the HTTP/2 compressor
- Update tests to reflect the correct state

Result:
1 less buffer leak.
2014-11-20 20:10:26 -05:00
Scott Mitchell
908b68da03 HTTP/2 Decompress/Compress buffer leak
Motivation:
The interface for HTTP/2 onDataRead states that buffers will be released by the codec.  The decompressor and compressor methods are not releasing buffers created during the decompression/compression process.

Modifications:
After onDataRead calls the decompressor and compressor classes will release the data buffer.

Result:
HTTP/2 compressor/decompressors are consistent with onDataRead interface assumptions.
2014-11-20 18:42:18 -05:00
Scott Mitchell
9173a2fb7c HTTP/2 Frame Listener Comment Correction
Motivation:
Some of the comments in HTTP/2 Frame Listener interface are misleading.

Modifications:
Clarify comments in Http2FrameListener.

Result:
Http2FrameListener onDataRead comments are clarified.
2014-11-20 13:04:23 -05:00
Daniel Bevenius
6a30c9534b Adding a propagateSettings flag to InboundHttp2ToHttpAdapter.
Motivation:
When DefaultHttp2FrameReader has read a settings frame, the settings
will be passed along the pipeline. This allows a client to hold off
sending data until it has received a settings frame. But for a server it
will always have received a settings frame and the usefulness of this
forwarding of settings is less useful. This also causes a debug message
to be logged on the server side if there is no channel handler to handle
the settings:

[nioEventLoopGroup-1-1] DEBUG io.netty.channel.DefaultChannelPipeline -
Discarded inbound message {INITIAL_WINDOW_SIZE=131072,
MAX_FRAME_SIZE=16384} that reached at the tail of the pipeline. Please
check your pipeline configuration.

Modifications:
Added a builder for the InboundHttp2ToHttpAdapter and
InboundHttp2PriortyAdapter and a new parameter named 'propagateSettings'
to their constructors.

Result:
It is now possible to control whether settings should be passed along
the pipeline or not.
2014-11-20 16:28:54 +01:00
Idel Pivnitskiy
9465db25ba Small performance improvements
Motivation:

Found performance issues via FindBugs and PMD.

Modifications:

- Removed unnecessary boxing/unboxing operations in DefaultTextHeaders.convertToInt(CharSequence) and DefaultTextHeaders.convertToLong(CharSequence). A boxed primitive is created from a string, just to extract the unboxed primitive value.
- Added a static modifier for DefaultHttp2Connection.ParentChangedEvent class. This class is an inner class, but does not use its embedded reference to the object which created it. This reference makes the instances of the class larger, and may keep the reference to the creator object alive longer than necessary.
- Added a static compiled Pattern to avoid compile it each time it is used when we need to replace some part of authority.
- Improved using of StringBuilders.

Result:

Performance improvements.
2014-11-19 23:44:25 -05:00
Scott Mitchell
aef49202b7 HTTP/2 Initial Settings Statics
Motivation:
The HTTP/2 codec currently does not expose the boundaries of the initial settings. This could be useful for applications to define their own initial settings.

Modifications:
Add new static final variables to Http2CodecUtil and make Http2Settings use these in the bounds checks.

Result:
Applications can use the max (or min) variables to initialize their settings.
2014-11-19 13:56:41 -05:00
nmittler
1079d02ef5 HTTP/2 inbound flow controller always throws connection error
Motivation:
When the inbound flow controller recognizes that the flow control window
has been violated on a stream (not connection-wide), it throws a
connection error.

Modifications:
Changed the DefaultHttp2InboundFlowController to properly throw
connection error if the connection window is violated and stream error
if a stream window is violated.

Result:
inbound flow control throws the correct error for window violations.
2014-11-19 09:50:44 -08:00
Scott Mitchell
9ecc08dd0f HTTP/2 Rename HTTP to HTTP2 object write converter
Motivation:
The current name of the class which converts from HTTP objects to HTTP/2 frames contains the text Http2ToHttp. This is misleading and opposite of what is being done.

Modifications:
Rename this class name to be HttpToHttp2.

Result:
Class names that more clearly identify what they do.
2014-11-19 08:58:47 -05:00
Scott Mitchell
b83f385017 HTTP/2 Decompress Flow Control
Motivation:
The current decompression frame listener currently opts-out of application level flow control. The application should still be able to control flow control even if decompression is in use.

Modifications:
- DecompressorFrameListener will maintain how many compressed bytes, decompressed bytes, and processed by the listener bytes.  A ratio will be used to translate these values into application level flow control amount.

Result:
HTTP/2 decompressor delegates the application level flow control to the listener processing the decompressed data.
2014-11-19 08:35:45 -05:00
nmittler
e7efe2b929 Fixing HTTP/2 processed byte accounting during exception
Motivation:
Currently when an exception occurs during a listener.onDataRead
callback, we return all bytes as processed. However, the listener may
choose to return bytes via the InboundFlowState object rather than
returning the integer. If the listener returns a few bytes and then
throws, we will attempt to return too many bytes.

Modifications:
Added InboundFlowState.unProcessedBytes() to indicate how many
unprocessed bytes are outstanding.

Updated DefaultHttp2ConnectionDecoder to compare the unprocessed bytes
before and after the listener.onDataRead callback when an exception was
encountered.  If there is a difference, it is subtracted off the total
processed bytes to be returned to the flow controller.

Result:
HTTP/2 data frame delivery properly accounts for processed bytes through
an exception.
2014-11-18 12:35:13 -08:00
Scott Mitchell
c8a1d077b5 HTTP/2 Priority Algorithm Restructure
Motivation:
The current priority algorithm uses 2 different mechanisms to iterate the priority tree and send the results of the allocation.  The current algorithm also uses a two step phase where the priority tree is traversed and allocation amounts are calculated and then all active streams are traversed to send for any streams that may or may not have been allocated bytes.

Modifications:
- DefaultHttp2OutboundFlowController will allocate and send (when possible) in the same looping structure.
- The recursive method will send only for the children instead of itself and its children which should simplify the recursion.

Result:
Hopefully simplified recursive algorithm where the tree iteration determines who needs to send and less iteration after the recursive calls complete.
2014-11-14 19:59:03 -05:00
nmittler
700ac93b15 Motivation:
Currently the DefaultHttp2InboundFlowController only supports the
ability to turn on and off "window maintenance" for a stream. This is
insufficient for true application-level flow control that may only want
to return a few bytes to flow control at a time.

Modifications:

Removing "window maintenance" interface from
DefaultHttp2InboundFlowController in favor of the new interface.

Created the Http2InboundFlowState interface which extends Http2FlowState
to add the ability to return bytes for a specific stream.

Changed the onDataRead method to return an integer number of bytes that
will be immediately returned to flow control, to support use cases that
want to opt-out of application-level inbound flow control.

Updated DefaultHttp2InboundFlowController to use 2 windows per stream.
The first, "window", is the actual flow control window that is
decremented as soon as data is received. The second "processedWindow"
is a delayed view of "window" that is only decremented after the
application returns the processed bytes. It is processedWindow that is
used when determining when to send a WINDOW_UPDATE to restore part of
the inbound flow control window for the stream/connection.

Result:

The HTTP/2 inbound flow control interfaces support application-level
flow control.
2014-11-14 09:54:43 -08:00
Idel Pivnitskiy
543daa3a9b Clean up code of HTTP/2 codec
Motivation:

Too many warnings from IntelliJ IDEA code inspector, PMD and FindBugs.

Modifications:

- Removed unnecessary casts, braces, modifiers, imports, throws on methods, etc.
- Added static modifiers where it is possible.
- Fixed incorrect links in javadoc.

Result:

Better code.
2014-11-13 23:57:59 -05:00
nmittler
f23f3b9617 Fix bug in HTTP/2 outbound flow control
Motivation:

The outbound flow controller logic does not properly reset the allocated
bytes between successive invocations of the priority algorithm.

Modifications:

Updated the priority algorithm to reset the allocated bytes for each
stream.

Result:

Each call to the priority algorithm now starts with zero allocated bytes
for each stream.
2014-11-13 14:40:55 -08:00
Daniel Bevenius
02f883d833 Allow DefaultHttp2Headers to be forced to lowercase.
Motivation:
I came across an issue when I was adding/setting headers and mistakenly
used an upper case header name. When using the http2 example that ships
with Netty this was not an issue. But when working with a browser that
supports http2, in my case I was using Firefox Nightly, I'm guessing
that it interprets the response as invalid in accordance with the
specifiction
https://tools.ietf.org/html/draft-ietf-httpbis-http2-14#section-8.1.2

"However, header field names MUST be converted to lowercase prior
to their encoding in HTTP/2.  A request or response containing
uppercase header field names MUST be treated as malformed"

This PR suggests converting to lowercase to be the default.

Modifications:
Added a no-args constructor that defaults to forcing the key/name to
lowercase, and providing a second constructor to override this behaviour
if desired.

Result:
It is now possible to specify a header like this:
Http2Headers headers = new DefaultHttp2Headers(true)
    .status(new AsciiString("200"))
    .set(new AsciiString("Testing-Uppercase"), new AsciiString("some value"));

And the header written to the client will then become:
testing-uppercase:"some value"
2014-11-11 16:36:20 +01:00
Scott Mitchell
685075f1a0 HTTP/2 Data Compressor
Motivation:
The HTTP/2 codec currently does not provide an interface to compress data. There is an analogous case to this in the HTTP codec and it is expected to be used commonly enough that it will be beneficial to have the feature in the http2-codec.

Modifications:
- Add a class which extends DefaultHttp2ConnectionEncoder and provides hooks to an EmbeddedChannel
- Add a compressor element to the Http2Stream interface
- Update unit tests to utilize the new feature

Result:
HTTP/2 codec supports data compression.
2014-11-06 14:45:05 -05:00
nmittler
ea2d471b9b Closure of stream with pending frames causes GO_AWAY.
Motivation:

The current logic in DefaultHttp2OutboundFlowController for handling the
case of a stream shutdown results in a Http2Exception (not a
Http2StreamException). This results in a GO_AWAY being sent for what
really could just be a stream-specific error.

Modifications:

Modified DefaultHttp2OutboundFlowController to set a stream exception
rather than a connection-wide exception.  Also using the error code of
INTERNAL_ERROR rather than STREAM_CLOSED, since it's more appropriate
for this case.

Result:

Should not be triggering GO_AWAY when a stream closes prematurely.
2014-11-02 20:39:06 -08:00
nmittler
1794f424d1 Keeping HTTP/2 HEADERS frames in-place WRT DATA frames.
Motivation:

Currently due to flow control, HEADERS frames can be written
out-of-order WRT DATA frames.

Modifications:

When data is written, we preserve the future as the lastWriteFuture in
the outbound flow controller.  The encoder then uses the lastWriteFuture
such that headers are only written after the lastWriteFuture completes.

Result:

HEADERS/DATA write order is correctly preserved.
2014-11-02 20:36:05 -08:00
Scott Mitchell
4a86dce2ca HTTP/2 to HTTP/1 non-ascii headers reset stream
Motivation:
The HTTP/2 specification indicates that when converting from HTTP/2 to HTTP/1.x and non-ascii characters are detected that an error should be thrown.

Modifications:
- The ASCII validation is already done but the exception that is raised is not properly converted to a RST_STREAM error.

Result:
- If HTTP/2 to HTTP/1.x translation layer is in use and a non-ascii header is received then a RST_STREAM frame should be sent in response.
2014-11-02 19:39:09 -05:00
Scott Mitchell
d5042baf58 HTTP/2 OutboundFlowControl negative window excpetion
Motivation:
The DefaultOutboundFlowController was attempting to write frames with a negative length.  This resulted in attempting to allocate a buffer of negative size and thus an exception.

Modifications:
- Don't allow DefaultOutboundFlowController to write negative length buffers.

Result:
No more negative length writes which resulted in IllegalArgumentExceptions.
2014-10-31 16:37:05 -04:00
Trustin Lee
ceb06dc1b1 Remove non-standard header values from HttpHeaderValues
Motivation:

x-gzip and x-deflate are not standard header values, and thus should be
removed from HttpHeaderValues, which is meant to provide the standard
values only.

Modifications:

- Remove X_DEFLATE and X_GZIP from HttpHeaderValues
- Move X_DEFLATE and X_GZIP to HttpContentDecompressor and
  DelegatingDecompressorFrameListener
  - We have slight code duplication here, but it does less harm than
    having non-standard constant.

Result:

HttpHeaderValues contains only standard header values.
2014-11-01 02:51:34 +09:00
Trustin Lee
f793f395d6 Replace HttpHeaders.Names/Values with HttpHeaderNames/Values
Related: 4ce994dd4f

Motivation:

In 4.1, we were not able to change the type of the HTTP header name and
value constants from String to AsciiString due to backward compatibility
reasons.

Instead of breaking backward compatibility in 4.1, we introduced new
types called HttpHeaderNames and HttpHeaderValues which provides the
AsciiString version of the constants, and then deprecated
HttpHeaders.Names/Values.

We should make the same changes while deleting the deprecated classes
activaly.

Modifications:

- Remove HttpHeaders.Names/Values and RtspHeaders
- Add HttpHeaderNames/Values and RtspHeaderNames/Values
  - Make HttpHeaderValues.WEBSOCKET lowercased because it's actually
    lowercased in all WebSocket versions but the oldest one
- Do not use AsciiString.equalsIgnoreCase(CharSeq, CharSeq) if one of
  the parameters are AsciiString
  - Avoid using AsciiString.toString() repetitively
    - Change the parameter type of some methods from String to
      CharSequence

Result:

A user who upgraded from 4.0 to 4.1 first and removed the references to
the deprecated classes and methods can easily upgrade from 4.1 to 5.0.
2014-11-01 02:41:56 +09:00
Scott Mitchell
99376c4391 HTTP/2 Send GOAWAY if no more stream ids
Motivation:
If the http2 encoder has exhausted all available stream IDs a GOAWAY frame is not sent. Once the encoder detects the a new stream ID has rolled over past the last stream ID a GOAWAY should be sent as recommended in section [5.1.1](https://tools.ietf.org/html/draft-ietf-httpbis-http2-14#section-5.1.1).

Modifications:
-This condition is already detected but it just needs to result in a GOAWAY being sent.
-Add a subclass of Http2Exception so the encoder can detect this special case.
-Add a unit test which checks that the GOAWAY is sent/received.

Result:
Encoder attempting to use the first 'rolled over' stream id results in a GOAWAY being sent.
2014-10-26 08:48:23 -04:00
Daniel Bevenius
7c43d9b84a Minor update to Http2LifecycleManager#closeLocalSide javadoc
Modifications:
Changed "remote" to "local" in javadoc for closeLocalSide.
2014-10-26 11:20:58 +01:00
Trustin Lee
36b41570a4 Overall clean-up on Headers and its subtypes
Motivation:

- There are still various inspector warnings to fix.
- ValueConverter.convert() methods need to end with the type name like
  other methods in Headers, such as setInt() and addInt(), for more
  consistency

Modifications:

- Fix all inspector warnings
- Rename ValueConverter.convert() to convert<type>()

Result:

- Cleaner code
- Consistency
2014-10-22 15:08:12 +09:00
Scott Mitchell
2374e17c6e Netty Headers Class Restructure and Algorithm Updates
Motivation:
Headers within netty do not cleanly share a common class hierarchy.  As a result some header types support some operations
and don't support others.  The consolidation of the class hierarchy will allow for maintenance and scalability for new codec.
The existing hierarchy also has a few short comings such as it is not clear when data conversions are happening.  This
could result unintentionally getting back a collection or iterator where a conversion on each entry must happen.

The current headers algorithm also prepends all elements which means to find the first element or return a collection
in insertion order often requires a complete traversal followed by a collections.reverse call.

Modifications:
-Provide a generic base class which provides all the implementation for headers in netty
-Provide an extension to this class which allows for name type conversions to happen (to accommodate legacy CharSequence to String conversions)
-Update the headers interface to clarify when conversions will happen.
-Update the headers data structure so that appends are done to avoid unnecessary iteration or collection reversal.

Result:
-More unified class hierarchy for headers in netty
-Improved headers data structure and algorithms
-headers API more clearly identify when conversions are required.
2014-10-21 13:04:08 -04:00
Scott Mitchell
c1e398a92c HTTP/2 Stream ID unit tests
Motivation:
There should be a unit test for when the stream ID wraps around and is 'too large' or negative.
The lack of unit test masked an issue where this was not being throw.

Modifications:
Add a unit test to cover the case where creating a remote and local stream where stream id is 'too large'

Result:
Unit test scope increases.
2014-10-20 16:36:47 -04:00
nmittler
5759f1d396 Changing stream verification to throw Http2StreamException.
Motivation:

Currently when receiving DATA/HEADERS frames, we throw Http2Exception (a
connection error) instead of Http2StreamException (stream error).  This
is incorrect according to the HTTP/2 spec.

Modifications:

Updated various places in the encoder and decoder that were out of spec
WRT connection/state checking.

Result:

Stream state verification is properly handled.
2014-10-18 01:13:30 +02:00
nmittler
e809f97136 Upgrading HTTP/2 hpack to latest version
Motivation:

Twitter hpack has upgraded to 0.9.1, we should upgrade to the latest.

Modifications:

Updated the parent pom to specify the dependency version. Updated the
http2 pom to use the version specified by the parent.

Result:

HTTP/2 updated to the latest hpack release.
2014-10-16 08:59:08 -07:00
Scott Mitchell
c8c69ae300 HTTP/2 Server Example Not Using Flow Controller
Motiviation:
The HTTP/2 server example is not using the outbound flow control.  It is instead using a FrameWriter directly.
This can lead to flow control errors and other comm. related errors

Modifications:
-Force server example to use outbound flow controller

Result:
-Server example should use follow flow control rules.
2014-10-13 13:44:31 +02:00
nmittler
ad259a456e Fixing race condition in HTTP/2 test
Motivation:

InboundHttp2ToHttpAdapterTest swaps non-volatile CountDownLatches in
handlers, which seems to cause a race condition that can lead to missing
messages.

Modifications:

Make CountDownLatch variables in InboundHttp2ToHttpAdapterTest volatile.

Result:

InboundHttp2ToHttpAdapterTest should be more stable.
2014-10-13 13:28:48 +02:00
nmittler
74e72ea752 Cleaning up GOAWAY methods.
Motivation:

The current GOAWAY methods are in each endpoint and are a little
confusing since their called connection.<endpoint>.goAwayReceived().

Modifications:

Moving GOAWAY methods to connection with more clear names
goAwaySent/goAwayReceived.

Result:

The GOAWAY method names are more clear.
2014-10-13 13:27:39 +02:00
Scott Mitchell
ce817e0d30 NullPointerException fix in http/2 codec
Motivation:
There is a NPE due to the order of builder initialization in the  class.

Modifications:
-Correct the ordering of initialization and building to avoid NPE.

Result:
No more NPE in  construction.
2014-10-09 18:13:57 -04:00
nmittler
034699268c Restoring access to HTTP/2 decoder's listener.
Motivation:

This was lost in recent changes, just adding it back in.

Modifications:

Added listener() accessor to Http2ConnectionDecoder and the default
impl.

Result:

The Http2FrameListener can be obtained from the decoder.
2014-10-09 12:42:49 -07:00
nmittler
1429543436 Simplifying and centralizing HTTP/2 exception handling logic
Motivation:

Currently, Http2LifecycleManager implements the exception handling logic
which makes it difficult to extend or modify the exception handling
behavior.  Simply overriding exceptionCaught() will only affect one of
the many possible exception paths. We need to reorganize the exception
handling code to centralize the exception handling logic into a single
place that can easily be extended by subclasses of
Http2ConnectionHandler.

Modifications:

Made Http2LifecycleManager an interface, implemented directly by
Http2ConnectionHandler. This adds a circular dependency between the
handler and the encoder/decoder, so I added builders for them that allow
the constructor of Http2ConnectionHandler to set itself as the lifecycle
manager and build them.

Changed Http2LifecycleManager.onHttpException to just
onException(Throwable) to simplify the interface. This method is now the
central control point for all exceptions. Subclasses now only need to
override onException() to intercept any exception encountered by the
handler.

Result:

HTTP/2 has more extensible exception handling, that is less likely to
see exceptions vanish into the ether.
2014-10-09 11:46:06 -07:00
nmittler
276b826b59 Fixing concurrency issue with HTTP/2 test mocking
Motivation:

Some tests occasionally appear unstable, throwing a
org.mockito.exceptions.misusing.UnfinishedStubbingException. Mockito
stubbing does not work properly in multi-threaded environments, so any
stubbing has to be done before the threads are started.

Modifications:

Modified tests to perform any custom stubbing before the client/server
bootstrap logic executes.

Result:

HTTP/2 tests should be more stable.
2014-10-04 19:12:49 -07:00
nmittler
f8890768be Adding missing asserts to HTTP/2 tests
Motivation:

Some tests do not properly assert that all requests have been
sent/received, so the failures messages may be misleading.

Modifications:

Adding missing asserts to HTTP/2 tests for awaiting requests and
responses.

Result:

HTTP/2 tests properly assert message counts.
2014-10-02 11:00:01 -07:00
Scott Mitchell
b785128349 HTTP/2 unit test missed syncrhonized collection
Motiviation:
PR https://github.com/netty/netty/pull/2948 missed a collection to synchronize in the HTTP/2 unit tests.

Modifications:
synchronize the collection that was missed

Result:
Missed collection is syncronized and initial size is corrected
2014-10-02 09:37:19 -04:00
nmittler
6f55a5b8e8 Motivation:
The HTTP/2 tests have been unstable, in particular the
Http2ConnectionRoundtripTest.

Modifications:

Modified fields in Http2TestUtil to be volatile.

Result:

Tests should (hopefully) be more stable.
2014-10-01 12:51:12 -07:00
nmittler
2b7f344a01 Fixing bugs in HTTP/2 pipeline exception handling
Motivation:

HTTP/2 codec does not properly test exception passed to
exceptionCaught() for instanceof Http2Exception (since the exception
will always be wrapped in a PipelineException), so it will never
properly handle Http2Exceptions in the pipeline.

Also if any streams are present, the connection close logic will execute
twice when a pipeline exception. This is because the exception logic
calls ctx.close() which then triggers the handleInActive() logic to
execute.  This clears all of the remaining streams and then attempts to
run the closeListener logic (which has already been run).

Modifications:

Changed exceptionCaught logic to properly extract Http2Exception from
the PipelineException.  Also added logic to the closeListener so that is
only run once.

Changed Http2CodecUtil.toHttp2Exception() to avoid NPE when creating
an exception with cause.getMessage().

Refactored Http2ConnectionHandler to more cleanly separate inbound and
outbound flows (Http2ConnectionDecoder/Http2ConnectionEncoder).

Added a test for verifying that a pipeline exception closes the
connection.

Result:

Exception handling logic is tidied up.
2014-09-30 16:31:42 -07:00
Scott Mitchell
741ea7766c HTTP/2 Unit Tests Synchronized Collections
Motivation:
The HTTP/2 unit tests are collecting responses read events which are happening in a multithreaded environment.
These collections are currently not synchronized or thread safe and are resulting in verification failures.

Modifications:
-Modify unit tests that use collections to store results for verifiction to be thread safe

Result:
Tests should not fail because of syncrhonization issues while verifying expected results.
2014-09-29 16:17:57 -04:00