Improve styx client

[mikkokar]

Description

Provide a new HttpClient interface, and provide an implementation in StyxHttpClient class.

The new interface allows:

• sender to choose the HTTP protocol (http or https) per request basis.
• allows requests to be sent either in streaming or full format.
• allows responses to be consumed either as streaming or full responses. However the client doesn't yet support this).

Also fixes a thread leak in styx end-to-end test suite which occurred in StyxMetrics class due to client not being closed.

Changes to NettyConnectionFactory

The new client interface allows styx client consumer to choose whether to send a request over http or secure https protocol.

However NettyConnectionFactory class is problematic because it only provides connections that are either bound to http or https protocol. Therefore a factory that creates http connections cannot (ever) be used to send https traffic. This is fine for Styx proxy but doesn't work well with a stand-alone client.

Therefore NettyConnectionFactory and NettyConnection classes are changed so that a class consumer can choose if it wants an http or an https channel. That is, the factory has just enough information to build "bare" netty channels, and the actual channel configuration is injected by the consumer in a createConnection call.

[kvosper]

This interface is inside HttpClient, but HttpClient does not use it.

[kvosper]

Storing the builder violates immutability.

[mikkokar]

It does. But it saves lots of repeated code.

The transactionParameters stores the default client settings at the HTTP client. These settings are inherited into HTTP request transaction where the parameters can be overridden. Storing them as a builder offers a convenient way of inheriting the default settings into a transaction. For two reasons:

• avoids copying parameters into auxiliary data structures
• any copying related to parameter inheritance can be encapsulated in the builder class itself

Perhaps I'll need to rename transactionParameters to defaultClientConfiguration.

[kvosper]

We have been consistent in our policy toward immutability so far, we cannot justify violating that just to type less code.

Besides, preserving immutability is the entire purpose of using the builder pattern. If we wanted to write mutable code we wouldn't use builders at all.

As a way to reduce code without introducing mutability, we could clone the builder, though there would still be internal mutability.

[mikkokar]

The builder pattern in fact in Java is used as a substitute for optional constructor arguments (See Effective Java by Joshua Bloch). The problem with Java is that the constructors don't scale well with large numbers of parameters. It has little to do with immutability (although some).

Having said that. The class at the moment indeed has a problem with immutability. But this is not a problem with storing the builder itself. The problem is that the builder is not cloned (as you perhaps noticed?) before it is passed in to the StyxHttpClient constructor.

I will add a call to the clone method.

[dvlato]

Why don't you use the Builder(builder) constructor that the Builder provides instead of clone()? Cloneable is not something we want to encourage and I would say it's been strongly advised against for years.
Other than that, Builder is used to avoid creating 'incomplete objects' (that's why we cannot use 'Java Beans' for that) and also avoid the limitations of constructors and static factories (that they do not provide either named or optional parameters) so we can construct objects in a clearer/more friendly way (it's usually complex objects, but we are using it in most cases). Even though it's true that it's one of the advantages of Builders, immutability can be achieved by using constructors or static factories...

[mikkokar]

Hey all. I mistakenly referred to clone. The Builder already implements a method called copy that replicates the builder object. The client is instantiated from the build object by:

            return new StyxHttpClient(connectionFactory, this.copy());

Apologies for the confusion.

[mikkokar]

The client class is now immutable from the outside. At least in terms of Builder object.

[kvosper]

Disabled test.

[mikkokar]

Well it is disabled. Unless you have any good ideas how to address my concerns regarding absolute URL testing I will just remove the test.

Although this is really lame excuse, leaving it disabled at least shows we have considered this scenario. :-/