Motivation:
maven-antrun-plugin does not redirect stdin, and thus it's impossible to
run interactive examples such as securechat-client and telnet-client.
org.codehaus.mojo:exec-maven-plugin redirects stdin, but it buffers
stdout and stderr, and thus an application output is not flushed timely.
Modifications:
Deploy a forked version of exec-maven-plugin which flushes output
buffers in a timely manner.
Result:
Interactive examples work. Launches faster than maven-antrun-plugin.
Motivation:
We do not provide such methods that provide the default name of the handlers. It had to be noticed and removed during the review phase, but we failed to do so.
Modifications:
Deprecate getName() static methods in the SOCKS codec classes
Result:
Getting closer to other codecs in API design.
Motivation:
exec-maven-plugin does not flush stdout and stderr, making the console
output from the examples invisible to users
Modification:
Use maven-antrun-plugin instead
Result:
A user sees the output from the examples immediately.
Motivation:
The examples have not been updated since long time ago, showing various
issues fixed in this commit.
Modifications:
- Overall simplification to reduce LoC
- Use system properties to get options instead of parsing args.
- Minimize option validation
- Just use System.out/err instead of Logger
- Do not pass config as parameters - just access it directly
- Move the main logic to main(String[]) instead of creating a new
instance meaninglessly
- Update netty-build-21 to make checkstyle not complain
- Remove 'throws Exception' clause if possible
- Line wrap at 120 (previously at 80)
- Add an option to enable SSL for most examples
- Use ChannelFuture.sync() instead of await()
- Use System.out for the actual result. Use System.err otherwise.
- Delete examples that are not very useful:
- websocket/html5
- websocketx/sslserver
- localecho/multithreaded
- Add run-example.sh which simplifies launching an example from command
line
Result:
Shorter and simpler examples. A user can focus more on what it actually
does than miscellaneous stuff. A user can launch an example very
easily.
Motivation:
According to TLS ALPN draft-05, a client sends the list of the supported
protocols and a server responds with the selected protocol, which is
different from NPN. Therefore, ApplicationProtocolSelector won't work
with ALPN
Modifications:
- Use Iterable<String> to list the supported protocols on the client
side, rather than using ApplicationProtocolSelector
- Remove ApplicationProtocolSelector
Result:
Future compatibility with TLS ALPN
Motivation:
- OpenSslEngine and JDK SSLEngine (+ Jetty NPN) have different APIs to
support NextProtoNego extension.
- It is impossible to configure NPN with SslContext when the provider
type is JDK.
Modification:
- Implement NextProtoNego extension by overriding the behavior of
SSLSession.getProtocol() for both OpenSSLEngine and JDK SSLEngine.
- SSLEngine.getProtocol() returns a string delimited by a colon (':')
where the first component is the transport protosol (e.g. TLSv1.2)
and the second component is the name of the application protocol
- Remove the direct reference of Jetty NPN classes from the examples
- Add SslContext.newApplicationProtocolSelector
Result:
- A user can now use both JDK SSLEngine and OpenSslEngine for NPN-based
protocols such as HTTP2 and SPDY
Motivation:
- In unwrap(), it does not check if the current index of dsts has
reached at its end offset, resulting in IndexOutOfBoundsException.
- SSLEngine does not update the position of the source buffer correctly
when SSL.writeToSSL() returns a negative value.
Modifications:
Fix them all
Result:
Less bugs
Motivation:
Previous fix for the OpenSslEngine compatibility issue (#2216 and
18b0e95659) was to feed SSL records one by
one to OpenSslEngine.unwrap(). It is not optimal because it will result
in more JNI calls.
Modifications:
- Do not feed SSL records one by one.
- Feed as many records as possible up to MAX_ENCRYPTED_PACKET_LENGTH
- Deduplicate MAX_ENCRYPTED_PACKET_LENGTH definitions
Result:
- No allocation of intemediary arrays
- Reduced number of calls to SSLEngine and thus its underlying JNI calls
- A tad bit increase in throughput, probably reverting the tiny drop
caused by 18b0e95659
Motivation:
Although 4cff4b99fd introduced
OpenSslEngine and its helper classes, a user has to write two different
copies of SSL initialization code that does pretty much same job,
because the initialization procedure between JDK SSLEngine and
OpenSslEngine are different.
Modifications:
- Replace OpenSslContextBuilder with SslContext which provides the
unified API for creating an SSL context
- SslContext allows you to create a new SSLEngine or a new SslHandler
with your PKCS#8 key and X.509 certificate chain.
- Merge OpenSslBufferPool into SslBufferPool
- Add an option to preallocate the pool
- Add an option to allocate direct buffers
- When OpenSSL is in use, preallocate direct buffers, which is close
to what OpenSslBufferPool does.
- Add JdkSslContext which is a simple wrapper of JDK's SSLContext
- The specified PKCS#8 key and X.509 certificate chain are converted
to JDK KeyStore in instantiation time.
- Like OpenSslServerContext, it uses sensible default cipher suites now.
- A user does not specify certPath and caPath separately anymore. He or
she has to merge them into a single file. I find this more logical
because previously ca file's first entry and cert file were always same.
- Clean up SSL tests to demonstrate the advantage of this change
- AbstractSocketSsl*Test now uses SslContext.new*Context() to
configure both the client and the server side. We did this only for
the server side previously and had to use different certificates for
JDK SSLEngine and OpenSslEngine, but not anymore.
- Add ApplicationProtocolSelector to ensure the future support for NPN
(NextProtoNego) and ALPN (Application Layer Protocol Negotiation) on
the client-side.
- Add SimpleTrustManagerFactory to help a user write a
TrustManagerFactory easily, which should be useful for those who need
to write an alternative verification mechanism. For example, we can
use it to implement an unsafe TrustManagerFactory that accepts
self-signed certificates for testing purposes.
- Add InsecureTrustManagerFactory and FingerprintTrustManager for quick
and dirty testing
- Add SelfSignedCertificate class which generates a self-signed X.509
certificate very easily.
- Update all our examples to use SslContext.newClient/ServerContext()
- Found that OpenSslEngine performs unnecessary memory copy - optimized
it.
- SslHandler now logs the chosen cipher suite when handshake is
finished.
Result:
- Cleaner unified API for configuring an SSL client and an SSL server
regardless of its internal implementation.
- When native libraries are available, OpenSSL-based SSLEngine
implementation is selected automatically to take advantage of its
performance benefit.
- Examples take advantage of this modification and thus are cleaner.
Motivation:
Some users already use an SSLEngine implementation in finagle-native. It
wraps OpenSSL to get higher SSL performance. However, to take advantage
of it, finagle-native must be compiled manually, and it means we cannot
pull it in as a dependency and thus we cannot test our SslHandler
against the OpenSSL-based SSLEngine. For an instance, we had #2216.
Modifications:
- Pull netty-tcnative in as an optional dependency.
http://netty.io/wiki/forked-tomcat-native.html
- Backport NativeLibraryLoader from 4.0
- Move OpenSSL-based SSLEngine implementation into our code base.
- Copied from finagle-native; originally written by @jpinner et al.
- Overall cleanup by @trustin.
- Run all SslHandler tests with both default SSLEngine and OpenSslEngine
Motivation:
At the moment there are two issues with HashedWheelTimer:
* the memory footprint of it is pretty heavy (250kb fon an empty instance)
* the way how added Timeouts are handled is inefficient in terms of how locks etc are used and so a lot of context-switching / condition can happen.
Modification:
Rewrite HashedWheelTimer to use an optimized bucket implementation to store the submitted Timeouts and a queue to handover the timeouts. So memory foot-print of the buckets itself is reduced a lot as the bucket uses a double-linked-list. Beside this we use Atomic*FieldUpdater where-ever possible to improve the memory foot-print and performance.
Result:
Lower memory-footprint and better performance
Motivation:
Before we aggregated the full text in the WebSocket08FrameDecoder just to fill in the ContinuationWebSocketFrame.aggregatedText(). The problem was that there was no upper-limit and so it would be possible to see an OOME if the remote peer sends a TextWebSocketFrame + a never ending stream of ContinuationWebSocketFrames. Furthermore the aggregation does not really belong in the WebSocket08FrameDecoder, as we provide an extra ChannelHandler for this anyway (WebSocketFrameAggregator).
Modification:
Remove the ContinuationWebSocketFrame.aggregatedText() method and corresponding constructor. Also refactored WebSocket08FrameDecoder a bit to me more efficient which is now possible as we not need to aggregate here.
Result:
No more risk of OOME because of frames.
Motivation:
When writing data from a server before the ssl handshake completes may not be written at all to the remote peer
if nothing else is written after the handshake was done.
Modification:
Correctly try to write pending data after the handshake was complete
Result:
Correctly write out all pending data
Motivation:
In the Internet Protocol, the valid port number range is from 1 to 65535
(inclusive on the both side.) However, SocksCmdRequest and SocksCmdResponse
refuses to construct itself when the port number 65535 is specified. Beside
this it excepts 0 as port number which should not allowed.
Modification:
* Not raise an exception when the specified port number is 65535.
* Raise an exception when the specified port number is 0
Result:
Fixes#2428
Motivation:
In the Internet Protocol, the valid port number range is from 1 to 65535
(inclusive on the both side.) However, SocksCmdRequest refuses to
construct itself when the port number 65535 is specified.
Modification:
Do not raise an exception when the specified port number is 65535.
Result:
Fixes#2428
Motivation:
At the moment the bossExecutor is not shutdown correctly. So the threads may never exit.
Modifications:
Correctly shutdown bossExecutor.
Result:
Threads are correctly stopped.
Motivation:
The names of SslHandler.unwrap*() methods are somewhat ambiguous.
Modifications:
- Rename unwrap() to unwrapMultiple() and unwrapNonApp()
- Rename unwrap0() to unwrapSingle()
Result:
Readability
Motivation:
Some SSLEngine implementations violate the contract and raises an
exception when SslHandler feeds an input buffer that contains multiple
SSL records to SSLEngine.unwrap(), while the expected behavior is to
decode the first record and return.
Modification:
Modify SslHandler.decode() to keep the lengths of each record and feed
SSLEngine.unwrap() record by record to work around the forementioned
issue.
Result:
SslHandler now works OpenSSLEngine from finagle-native. Performance
impact remains unnoticeable. Fixes#2116
Motivation:
Some Android SSLEngine implementations skip FINISHED handshake status
and go straightly into NOT_HANDSHAKING. This behavior blocks SslHandler
from notifying its handshakeFuture, because we do the notification when
SSLEngine enters the FINISHED state.
Modification:
When the current handshake state is NOT_HANDSHAKING and the
handshakeFuture is not fulfilled yet, treat NOT_HANDSHAKING as FINISHED.
Result:
Better Android compatibility
Motivation:
Currently, the SPDY frame encoding and decoding code is based upon
the ChannelHandler abstraction. This requires maintaining multiple
versions for 3.x and 4.x (and possibly 5.x moving forward).
Modifications:
The SPDY frame encoding and decoding code is separated from the
ChannelHandler and SpdyFrame abstractions. Also test coverage is
improved.
Result:
SpdyFrameCodec now implements the ChannelHandler abstraction and is
responsible for creating and handling SpdyFrame objects.
Motivation:
The Junit test for issue #2305 was incomplete for V3.9 since the request was not setup using setChunked(true) and therefore the chunks were ignored.
Modifications:
Add the chunk true property to the request and add the final LAST_CHUNK to the chunks offered to the decoder.
Result:
Now the request is correctly and fully decoded by the decoder in this Junit test case.
Motivation:
If the last item analyzed in a previous received HttpChunk/HttpContent was a part of an attribute's name, the read index was not set to the new right place and therefore raizing an exception in some case (since the "new" name analyzed is empty, which is not allowed so the exception).
What appears there is that the read index should be reset to the last valid position encountered whatever the case. Currently it was set when only when there is an attribute not already finished (name is ok, but content is possibly not).
Therefore the issue is that elements could be rescanned multiple times (including completed elements) and moreover some bad decoding can occur such as when in a middle of an attribute's name.
Modifications:
To fix this issue, since "firstpos" contains the last "valid" read index of the decoding (when finding a '&', '=', 'CR/LF'), we should add the setting of the read index for the following cases:
'lastchunk' encountered, therefore finishing the current buffer
any other cases than current attribute is not finished (name not found yet in particular)
So adding for this 2 cases:
undecodedChunk.readerIndex(firstpos);
Result:
Now the decoding is done once, content is added from chunk/content to chunk/content, name is decoded correctly even if in the middle of 2 chunks/contents.
A Junit test code was added: testChunkCorrect that should not raized any exception.
Motivation:
JdkZlibEncoder does not generate a GZIP header when a user does not write anything at all, ending up with a 10-byte footer only.
Modification:
Modify finishEncode() method to write the GZIP header if it was not written before.
Result:
Encoding 0-byte content result in 20-byte GZIP stream now.
Motivation:
Use simple retry mechanism when try to connect to peer so a slow-startup will not produce an error
Modification:
Add new Servlet params which allow to configure retry count and wait time. Default behaviour is the same as before
Result:
It is now possible to configure retry when the peer is not up yet and the Servlet is init.
Motivation:
CONTRIBUTING.md is useful only because it lets Github show a user the
link to it so the user can check what information we need before
submitting a bug report. However, Github does not do the same for a
pull request submission form, and thus there's no reason to keep the
information about how to submit a good pull request in CONTRIBUTING.md.
Modification:
Replace the section about issuing a pull request with the link to the
official developer guide.
Result:
CONTRIBUTING.md is easier to maintain.
Motivation:
We often receive a bug report or a pull request which do not give us
enough information. If CONTRIBUTING.md exists in the repository, Github
will display some notice in the beginning of the issue submission form,
which might increase the overall quality of the bug reports and pull
requests.
Modification:
Write CONTRIBUTING.md
Result:
Potentially higher-quality bug reports and pull requests
