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.
(Ported @luciferous's changes against 3.10)
Motivation:
The current implementation of the encoder writes each character of the
String as a single byte to the buffer, however not all characters are
mappable to a single byte.
Modifications:
If a character is outside the ASCII range, it's converted to '?'.
Result:
A safer encoder for String to ASCII, which substitutes unmappable
characters with'?'.
Motivation:
Not knowing which unit is used for the maxContentLength of the HttpObjectAggregator when reading the Javadoc is annoying and can be a source of bugs.
Modifications:
Added the mention "in bytes"
Result:
Javadoc is clear.
Related: #3445
Motivation:
HttpObjectDecoder.HeaderParser does not reset its counter (the size
field) when it failed to find the end of line. If a header is split
into multiple fragments, the counter is increased as many times as the
number of fragments, resulting an unexpected TooLongFrameException.
Modifications:
- Add test cases that reproduces the problem
- Reset the HeaderParser.size field when no EOL is found.
Result:
One less bug
Motivation:
Currently CORS can be configured to support a 'null' origin, which can
be set by a browser if a resources is loaded from the local file system.
When this is done 'Access-Control-Allow-Origin' will be set to "*" (any
origin). There is also a configuration option to allow credentials being
sent from the client (cookies, basic HTTP Authentication, client side
SSL). This is indicated by the response header
'Access-Control-Allow-Credentials' being set to true. When this is set
to true, the "*" origin is not valid as the value of
'Access-Control-Allow-Origin' and a browser will reject the request:
http://www.w3.org/TR/cors/#resource-requests
Modifications:
Updated CorsHandler's setAllowCredentials to check the origin and if it
is "*" then it will not add the 'Access-Control-Allow-Credentials'
header.
Result:
Is is possible to have a client send a 'null' origin, and at the same
time have configured the CORS to support that and to allow credentials
in that combination.
Motivation:
At the moment if you want to return a HTTP header containing multiple
values you have to set/add that header once with the values wanted. If
you used set/add with an array/iterable multiple HTTP header fields will
be returned in the response.
Note, that this is indeed a suggestion and additional work and tests
should be added. This is mainly to bring up a discussion.
Modifications:
Added a flag to specify that when multiple values exist for a single
HTTP header then add them as a comma separated string.
In addition added a method to StringUtil to help escape comma separated
value charsequences.
Result:
Allows for responses to be smaller.
Motivation:
To use WebSocketClientHandshaker / WebSocketServerHandshaker it's currently a requirement of having a HttpObjectAggregator in the ChannelPipeline. This is not a big deal when a user only wants to server WebSockets but is a limitation if the server serves WebSockets and normal HTTP traffic.
Modifications:
Allow to use WebSocketClientHandshaker and WebSocketServerHandshaker without HttpObjectAggregator in the ChannelPipeline.
Result:
More flexibility
Motivation:
SonarQube (clinker.netty.io/sonar) reported a resource which may not have been properly closed in all situations in AbstractDiskHttpData.
Modifications:
- Ensure file channels are closed in the presence of exceptions.
- Correct instances where local channels were created but potentially not closed.
Result:
Less leaks. Less SonarQube vulnerabilities.
Motivation:
`HttpResponseDecoder` and `HttpRequestDecoder` in the event when the max configured sizes for HTTP initial line, headers or content is breached, sends a `DefaultHttpResponse` and `DefaultHttpRequest` respectively. After this `HttpObjectDecoder` gets into `BAD_MESSAGE` state and ignores any other data received on this connection.
The combination of the above two behaviors, means that the decoded response/request are not complete (absence of sending `LastHTTPContent`). So, any code, waiting for a complete message will have to additionally check for decoder result to follow the correct semantics of HTTP.
If `HttpResponseDecoder` and `HttpRequestDecoder` creates a Full* invalid message then the request/response is a complete HTTP message and hence obeys the HTTP contract.
Modification:
Modified `HttpRequestDecoder`, `HttpResponseDecoder`, `RtspRequestDecoder` and `RtspResponseDecoder` to return Full* messages from `createInvalidMessage()`
Result:
Fixes the wrong behavior of sending incomplete messages from these codecs
In testEncodingSingleCookieV0():
Let's assume we encoded a cookie with MaxAge=50 when currentTimeMillis
is 10999.
Because the encoder will not encode the millisecond part for Expires,
the timeMillis value of the encoded Expires field will be 60000. (If we
did not dropped the millisecond part, it would be 60999.)
Encoding a cookie will take some time, so currentTimeMillis will
increase slightly, such as to 11001.
diff = (60000 - 11001) / 1000 = 48999 / 1000 = 48
maxAge - diff = 50 - 48 = 2
Due to losing millisecond part twice, we end up with the precision
problem illustrated above, and thus we should increase the tolerance
from 1 second to 2 seconds.
/cc @slandelle
Motivation:
Internet Explorer doesn't honor Set-Cookie header Max-Age attribute. It only honors the Expires one.
Modification:
Always generate an Expires attribute along the Max-Age one.
Result:
Internet Explorer compatible expiring cookies. Close#1466.
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
Motivation:
HttpContentDecoder had the following issues:
- For chunked content, the decoder set invalid "Content-Length" header
with length of the first decoded chunk.
- Decoding of FullHttpRequests put both the original conent and decoded
content into output. As result, using HttpObjectAggregator before the
decoder lead to errors.
- Requests with "Expect: 100-continue" header were not acknowleged:
the decoder didn't pass the header message down the handler's chain
until content is received. If client expected "100 Continue" response,
deadlock happened.
Modification:
- Invalid "Content-Length" header is removed; handlers down the chain can either
rely on LastHttpContent message or ask HttpObjectAggregator to add the header.
- FullHttpRequest is split into HttpRequest and HttpContent (decoded) parts.
- Header (HttpRequest) part of request is sent down the chain as soon as it's received.
Result:
The issues are fixed, unittest is added.
Motivation:
Pull request for RFC6265 support had some unused flag first in ClientCookieDecoder.
Modification:
Remove unused flag first.
Result:
Cleaner code.
Motivation:
Rfc6265Client/ServerCookieEncoder is a better replacement of the old
Client/ServerCookieEncoder, and thus there's no point of keeping both.
Modifications:
- Remove the old Client/ServerCookieEncoder
- Remove the 'Rfc6265' prefix from the new cookie encoder/decoder
classes
- Deprecate CookieDecoder
Result:
We have much better cookie encoder/decoder implementation now.
Motivation:
Currently Netty supports a weird implementation of RFC 2965.
First, this RFC has been deprecated by RFC 6265 and nobody on the
internet use this format.
Then, there's a confusion between client side and server side encoding
and decoding.
Typically, clients should only send name=value pairs.
This PR introduces RFC 6265 support, but keeps on supporting RFC 2965 in
the sense that old unused fields are simply ignored, and Cookie fields
won't be populated. Deprecated fields are comment, commentUrl, version,
discard and ports.
It also provides a mechanism for safe server-client-server roundtrip, as
User-Agents are not supposed to interpret cookie values but return them
as-is (e.g. if Set-Cookie contained a quoted value, it should be sent
back in the Cookie header in quoted form too).
Also, there are performance gains to be obtained by not allocating the
attribute name Strings, as we only want to match them to find which POJO
field to populate.
Modifications:
- New RFC6265ClientCookieEncoder/Decoder and
RFC6265ServerCookieEncoder/Decoder pairs that live alongside old
CookieEncoder/Decoder pair to not break backward compatibility.
- New Cookie.rawValue field, used for lossless server-client-server
roundtrip.
Result:
RFC 6265 support.
Clean separation of client and server side.
Decoder performance gain:
Benchmark Mode Samples Score Error
Units
parseOldClientDecoder thrpt 20 2070169,228 ± 105044,970
ops/s
parseRFC6265ClientDecoder thrpt 20 2954015,476 ± 126670,633
ops/s
This commit closes#3221 and #1406.
Motivation:
HttpPostMultipartRequestDecoder threw an ArrayIndexOutOfBoundsException
when trying to decode Content-Disposition header with filename
containing ';' or protected \\".
See issue #3326 and #3327.
Modifications:
Added splitMultipartHeaderValues method which cares about quotes, and
use it in splitMultipartHeader method, instead of StringUtils.split.
Result:
Filenames can contain semicolons and protected \\".
Motivation:
HttpResponseStaus, HttpMethod and HttpVersion have methods that return
AsciiString. There's no need for object-to-string conversion.
Modifications:
Use codeAsText(), name(), text() instead of setInt() and setObject()
Result:
Efficiency
Motivation:
The SpdyHttpDecoder was modified to support pushed resources that are
divided into multiple frames. The decoder accepts a pushed
SpdySynStreamFrame containing the request headers, followed by a
SpdyHeadersFrame containing the response headers.
Modifications:
This commit modifies the SpdyHttpEncoder so that it encodes pushed
resources in a format that the SpdyHttpDecoder can decode. The encoder
will accept an HttpRequest object containing the request headers,
followed by an HttpResponse object containing the response headers.
Result:
The SpdyHttpEncoder will create a SpdySynStreamFrame followed by a
SpdyHeadersFrame when sending pushed resources.
Motivations:
It seems that slicing a buffer and using this slice to write to CTX will
decrease the initial refCnt to 0, while the original buffer is not yet
fully used (not empty).
Modifications:
As suggested in the ticket and tested, when the currentBuffer is sliced
since it will still be used later on, the currentBuffer is retained.
Add a test case for this issue.
Result:
The currentBuffer still has its correct refCnt when reaching the last
write (not sliced) of 1 and therefore will be released correctly.
The exception does no more occur.
This fix should be applied to all branches >= 4.0.
When handling an oversized message, HttpObjectAggregator does not wait
until the last chunk is received to produce the failed message, making
AggregatedFullHttpMessage.trailingHeaders() return null.
Related: #3019
Motivation:
We have multiple (Full)HttpRequest/Response implementations and only
some of them implements toString() properly.
Modifications:
- Add the reusable string converter for HttpMessages to HttpMessageUtil
- Implement toString() of (Full)HttpRequest/Response implementations
properly using HttpMessageUtil
Result:
Prettier string representation is returned by HttpMessage
implementations.
Motivation:
Even if its against the HTTP RFC there are situations where it may be useful to use other chars then US_ASCII in the headers. We should allow to make it possible by allow the user to override the how headers are encoded.
Modifications:
- Add encodeHeaders(...) method and so allow to override it.
Result:
It's now possible to encode headers with other charset then US_ASCII by just extend the encoder and override the encodeHeaders(...) method.
Motivation:
HEAD requests will have a Content-Length set that doesn't match the
actual length. So we only want to set Content-Length header if it isn't
already set.
Modifications:
If check around setting the Content-Length.
Result:
A HEAD request will no correctly return the specified Content-Length
instead of the body length.
Modifications:
Converted AsciiString into a String by calling toString() method before comparing with equals(). Also added a unit-test to show that it works.
Result:
Major violation is gone. Code is correct.
Motivation:
without this check then given a URI with path /path the resulting URL will be /path?null=
Modifications:
check that getRawQuery doesn't return null and only append if not
Result:
urls of the form /path will not have a null?= appended
Motivations:
The chunkSize might be oversized after comparison (size being > of int
capacity) if file size is bigger than an integer.
Modifications:
Change it to long.
Result:
There is no more int oversized.
Same fix for 4.1 and Master
Motivation:
The new Headers interface contains methods to getTimeMillis but no add/set/contains variants. These should be added for consistency.
Modifications:
- Add three new methods: addTimeMillis, setTimeMillis, containsTimeMillis to the Headers interface.
- Add a new method to the Headers.ValueConverter interface: T convertTimeMillis(long)
- Bring these new interfaces up the class hierarchy
Result:
All Headers classes have setters/getters for timeMillis.
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
Result:
It's easier to determine the class of an HTTP status
Motivation:
I found myself writing AsciiString constants in my code for
response statuses and thought that perhaps it might be nice to have
them defined by Netty instead.
Modifications:
Adding codeAsText to HttpResponseStatus that returns the status code as
AsciiText.
In addition, added the 421 Misdirected Request response code from
https://tools.ietf.org/html/draft-ietf-httpbis-http2-15#section-9.1.2
This response header was renamed in draft 15:
https://tools.ietf.org/html/draft-ietf-httpbis-http2-15#appendix-A.1
But the code itself was not changed, and I thought using the latest would
be better.
Result:
It is now possible to specify a status like this:
new DefaultHttp2Headers().status(HttpResponseStatus.OK.codeAsText());
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.
Motivation:
The SPDY/3.1 spec does not adequate describe how to push resources
from the server. This was solidified in the HTTP/2 drafts by dividing
the push into two frames, a PushPromise containing the request,
followed by a Headers frame containing the response.
Modifications:
This commit modifies the SpdyHttpDecoder to support pushed resources
that are divided into multiple frames. The decoder will accept a
pushed SpdySynStreamFrame containing the request headers, followed by
a SpdyHeadersFrame containing the response headers.
Result:
The SpdyHttpDecoder will create an HttpRequest object followed by an
HttpResponse object when receiving pushed resources.
Motivation:
RFC 2616, 4.3 Message Body states that:
All 1xx (informational), 204 (no content), and 304 (not modified) responses MUST NOT include a
message-body. All other responses do include a message-body, although it MAY be of zero length.
Modifications:
HttpContentEncoder was previously modified to cater for HTTP 100 responses. This check is enhanced to
include HTTP 204 and 304 responses.
Result:
Empty response bodies will not be modified to include the compression footer. This footer messed with Chrome's
response parsing leading to "hanging" requests.
Motivation:
HttpObjectDecoder extended ReplayDecoder which is slightly slower then ByteToMessageDecoder.
Modifications:
- Changed super class of HttpObjectDecoder from ReplayDecoder to ByteToMessageDecoder.
- Rewrote decode() method of HttpObjectDecoder to use proper state machine.
- Changed private methods HeaderParser.parse(ByteBuf), readHeaders(ByteBuf) and readTrailingHeaders(ByteBuf), skipControlCharacters(ByteBuf) to consider available bytes.
- Set HeaderParser and LineParser as static inner classes.
- Replaced not safe actualReadableBytes() with buffer.readableBytes().
Result:
Improved performance of HttpObjectDecoder by approximately 177%.
Motiviation:
The HttpContentEncoder does not account for a EmptyLastHttpContent being provided as input. This is useful in situations where the client is unable to determine if the current content chunk is the last content chunk (i.e. a proxy forwarding content when transfer encoding is chunked).
Modifications:
- HttpContentEncoder should not attempt to compress empty HttpContent objects
Result:
HttpContentEncoder supports a EmptyLastHttpContent to terminate the response.
Motivation:
Headers has getTimeMillis(), not getDate()
Modification:
- Replace HttpHeaders.getDate() with getTimeMillis() so that migration
is smoother
Result:
User code which accesses a date header is easier to migrate
Motivation:
The commit 50e06442c3 changed the type of
the constants in HttpHeaders.Names and HttpHeaders.Values, making 4.1
backward-incompatible with 4.0.
It also introduces newer utility classes such as HttpHeaderUtil, which
deprecates most static methods in HttpHeaders. To ease the migration
between 4.1 and 5.0, we should deprecate all static methods that are
non-existent in 5.0, and provide proper counterpart.
Modification:
- Revert the changes in HttpHeaders.Names and Values
- Deprecate all static methods in HttpHeaders in favor of:
- HttpHeaderUtil
- the member methods of HttpHeaders
- AsciiString
- Add integer and date access methods to HttpHeaders for easier future
migration to 5.0
- Add HttpHeaderNames and HttpHeaderValues which provide standard HTTP
constants in AsciiString
- Deprecate HttpHeaders.Names and Values
- Make HttpHeaderValues.WEBSOCKET lowercased because it's actually
lowercased in all WebSocket versions but the oldest one
- Add RtspHeaderNames and RtspHeaderValues which provide standard RTSP
constants in AsciiString
- Deprecate RtspHeaders.*
- 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:
Backward compatibility is recovered. New classes and methods will make
the migration to 5.0 easier, once (Http|Rtsp)Header(Names|Values) are
ported to master.
Motivation:
The header class hierarchy and algorithm was improved on the master branch for versions 5.x. These improvments should be backported to the 4.1 baseline.
Modifications:
- cherry-pick the following commits from the master branch: 2374e17, 36b4157, 222d258
Result:
Header improvements in master branch are available in 4.1 branch.
Motivation:
The requirement for the masking of frames and for checks of correct
masking in the websocket specifiation have a large impact on performance.
While it is mandatory for browsers to use masking there are other
applications (like IPC protocols) that want to user websocket framing and proxy-traversing
characteristics without the overhead of masking. The websocket standard
also mentions that the requirement for mask verification on server side
might be dropped in future.
Modifications:
Added an optional parameter allowMaskMismatch for the websocket decoder
that allows a server to also accept unmasked frames (and clients to accept
masked frames).
Allowed to set this option through the websocket handshaker
constructors as well as the websocket client and server handlers.
The public API for existing components doesn't change, it will be
forwarded to functions which implicetly set masking as required in the
specification.
For websocket clients an additional parameter is added that allows to
disable the masking of frames that are sent by the client.
Result:
This update gives netty users the ability to create and use completely
unmasked websocket connections in addition to the normal masked channels
that the standard describes.
Motivation:
At the moment the whole HTTP header must be parsed at once which can lead to multiple parsing of the same bytes. We can do better here and allow to parse it in multiple steps.
Modifications:
- Not parse headers multiple times
- Simplify the code
- Eliminate uncessary String[] creations
- Use readSlice(...).retain() when possible.
Result:
Performance improvements as shown in the included benchmark below.
Before change:
[nmaurer@xxx]~% ./wrk-benchmark
Running 2m test @ http://xxx:8080/plaintext
16 threads and 256 connections
Thread Stats Avg Stdev Max +/- Stdev
Latency 21.55ms 15.10ms 245.02ms 90.26%
Req/Sec 196.33k 30.17k 297.29k 76.03%
373954750 requests in 2.00m, 50.15GB read
Requests/sec: 3116466.08
Transfer/sec: 427.98MB
After change:
[nmaurer@xxx]~% ./wrk-benchmark
Running 2m test @ http://xxx:8080/plaintext
16 threads and 256 connections
Thread Stats Avg Stdev Max +/- Stdev
Latency 20.91ms 36.79ms 1.26s 98.24%
Req/Sec 206.67k 21.69k 243.62k 94.96%
393071191 requests in 2.00m, 52.71GB read
Requests/sec: 3275971.50
Transfer/sec: 449.89MB
Motivation:
The 4.1.0-Beta3 implementation of HttpObjectAggregator.handleOversizedMessage closes the
connection if the client sent oversized chunked data with no Expect:
100-continue header. This causes a broken pipe or "connection reset by
peer" error in some clients (tested on Firefox 31 OS X 10.9.5,
async-http-client 1.8.14).
This part of the HTTP 1.1 spec (below) seems to say that in this scenario the connection
should not be closed (unless the intention is to be very strict about
how data should be sent).
http://www.w3.org/Protocols/rfc2616/rfc2616-sec8.html
"If an origin server receives a request that does not include an
Expect request-header field with the "100-continue" expectation,
the request includes a request body, and the server responds
with a final status code before reading the entire request body
from the transport connection, then the server SHOULD NOT close
the transport connection until it has read the entire request,
or until the client closes the connection. Otherwise, the client
might not reliably receive the response message. However, this
requirement is not be construed as preventing a server from
defending itself against denial-of-service attacks, or from
badly broken client implementations."
Modifications:
Change HttpObjectAggregator.handleOversizedMessage to close the
connection only if keep-alive is off and Expect: 100-continue is
missing. Update test to reflect the change.
Result:
Broken pipe and connection reset errors on the client are avoided when
oversized data is sent.
Related: #2983
Motivation:
It is a well known idiom to write an empty buffer and add a listener to
its future to close a channel when the last byte has been written out:
ChannelFuture f = channel.writeAndFlush(Unpooled.EMPTY_BUFFER);
f.addListener(ChannelFutureListener.CLOSE);
When HttpObjectEncoder is in the pipeline, this still works, but it
silently raises an IllegalStateException, because HttpObjectEncoder does
not allow writing a ByteBuf when it is expecting an HttpMessage.
Modifications:
- Handle an empty ByteBuf specially in HttpObjectEncoder, so that
writing an empty buffer does not fail even if the pipeline contains an
HttpObjectEncoder
- Add a test
Result:
An exception is not triggered anymore by HttpObjectEncoder, when a user
attempts to write an empty buffer.
Motivation:
Currently, when the CorsHandler processes a preflight request, or
respondes with an 403 Forbidden using the short-curcuit option, the
HttpRequest is not released which leads to a buffer leak.
Modifications:
Releasing the HttpRequest when done processing a preflight request or
responding with an 403.
Result:
Using the CorsHandler will not cause buffer leaks.
