Incorrect builder pattern implementation

[ash2k]

RequestBuilder (RequestBuilderBase) implements builder pattern to build instances of Request object. Some issues with it:

• RequestBuilder instance cannot be reused because it holds state for Request object instance - this is not even documented;
• Traditionally an object built by a builder is immutable and is not connected to it's builder in any way. Built Request object is not immutable - can be mutated by the builder even after construction;
• Because of the above Request object is not threadsafe. It's good to fix that too.

The obvious way to fix that issue is to construct new Request instances on each build() and make all it's fields final.

AsyncHttpClientConfig have the same problem with threadsafety - it have mutable fields. Should be final.

[slandelle]

Hi,

Actually, the only way to have RequestBuilder perfectly thread-safe and reusable would be to make it immutable.

pros:

• threadsafe
• reusable

cons:

• API breakage
• more garbage/memory usage/CPU for GC

This might sound selfish, but the current design perfectly suit my needs for my main project, where I can instanciate several thousands of RequestBuilders/Requests per sec, in a context that takes care of synchronization for me.

IMHO, we should document that the current design is not threadsafe, and maybe have an immutable implementation that would live along side the current mutable one once someone really face the problem or contribute it.

[jfarcand]

+1. Added the Documentation tag.

[ash2k]

How can a builder be immutable? =) It accumulates state so it's just impossible (or you will need to create a new immutable instance for each mutation and that doesnt make sense). Request should be immutable and not hold any reference to RequestBuilder (and no reference from RequestBuilder to Request too).
By "reusable RequestBuilder" i meant that it can be (should be) reused in the same thread after construction of some Request. Threads should not share instances of builders, but can share/reuse Request instances.

• No API breakage;
• Possibly LESS garbage because RequestBuilder can be reused - now you should create builder + request objects for each request. With proper builder implementation builder can be reused (at least sometimes) so less garbage.

[slandelle]

How can a builder be immutable?

Yep, create a new immutable instance for each mutation

that doesn't make sense

Yes it does. Try writing a DSL and you'll realize that's the only way to have reusable builders. Reusable means one can write something like this:

Builder rootBuilder = ... // builder configured with common properties
Builder builder1 = rootBuilder.configureSomething();
Builder builder2 = rootBuilder.configureSomethingElse();

Threads should not share instances of builders

Really? Why so?

Possibly LESS garbage

Yep, possibly. More in other use cases. For example, if one doesn't reuse RequestBuilder instances.

Honestly, you're too much focused on the techniques and the use cases you know, so the "impossible", "doesn't make sense", "should", "should not", etc.

What we could do is find a way to make RequestBuilder complex parts reusable, such as setUrl and build, so that anyone can build a builder that suits his needs.

[cowtowncoder]

Immutable builders are quite possible and very useful. Although if you go with that route, you can just drop builder part and keep value immutable, with "mutant factory" methods ("withXxx()" methods that create new instance with one property changed as instructed).

I really would not worry at all about memory churn: this will be short-term garbage collected from YoungGen and cost is typically insignificant in scope of HTTP processing. Further, immutable things are reusable as is, so it is possible to eliminate much/most of even this garbage, for those who want to optimize to 11.

I would be fine with either approach. But I do think that use of separate Builders typically is to divide construction-time mutability (builder) and resulting immutable value object. So term builder tends to refer to mutable intermediate (reusable or not) objects.

[ash2k]

I was trying to say that "Makes no sense" applies not to immutable builders in general sense but in context of AHC. Because (certanly i agree with you here) it creates more garbage but provides immutabilty and threadsafety for builder that is not needed for most users of AHC. So overhead is not worth it.

What i'm proposing is to refactor current implementation to be classical one (mutable builer without any synchronization and immutable object that is built on each build() invocation). Lets compare them:

• Same memory footprint in gerenal case if builder is not reused, potentially less garbage if it is reused (am i missing something here?);
• Same level of threadsafety of builder;
• Same API;
• Improved threadsafety of Request instances via immutability;
• It's not possible to break constructed Request instances by using their builders.

[jfarcand]

@ash2k I'm all for a pull request here agains the 1.8 (master) branch. That way we can measure the change required, and see how compatibility with previous version.

[ash2k]

@jfarcand, will provide a pull request but not sooner than after 10 days. Have no technical ability to do that right now.

[ash2k]

Please, take a look at immutable-builder branch of my fork. Currently it fails tests because tests rely on Request object mutations (they mutate headers, etc). What i propose to do as a next step of refactoring is to make Request's getters that return FluentStringsMap/FluentCaseInsensitiveStringsMap to just return unmodifiable maps:

• this will make mutations on those fields impossible (will thow exceptions) and tests will need to be fixed somehow;
• this will reduce ammount of garbage - currently getters have to make copies of mutable objects to avoid mutations;
• this breaks backwards compatibility. I thought API will not be broken, but looks like those not very good parts have to be broken to be fixed;
• at last i think it makes sense - you don't need fluent operations of those objects to read headers, etc from Requst.

Certanly i do not expect this branch in it's current state to be merged, i just want feedback and advise.

So, what do you think?

Failed tests:   win7DisconnectTest(com.ning.http.client.providers.netty.NettyConnectionPoolTest): Must have received an exception
  postRedirectGet301Test(com.ning.http.client.providers.netty.NettyPostRedirectGetTest): com.ning.http.client.MaxRedirectException: Maximum redirect reached: 5
  postRedirectGet302Test(com.ning.http.client.providers.netty.NettyPostRedirectGetTest): com.ning.http.client.MaxRedirectException: Maximum redirect reached: 5
  postRedirectGet307Test(com.ning.http.client.providers.netty.NettyPostRedirectGetTest): com.ning.http.client.MaxRedirectException: Maximum redirect reached: 5
  postRedirectGet303Test(com.ning.http.client.providers.netty.NettyPostRedirectGetTest): com.ning.http.client.MaxRedirectException: Maximum redirect reached: 5
  postRedirectGet302StrictTest(com.ning.http.client.providers.netty.NettyPostRedirectGetTest): com.ning.http.client.MaxRedirectException: Maximum redirect reached: 5

Tests run: 257, Failures: 6, Errors: 0, Skipped: 0

[slandelle]

Hi,

Thanks for the contribution. I'll review it next week.

As I said, I'd like to have some refactoring beforehand, so that we can maybe provide several implementations, or one can develop his own, without having to fork.

This new implementation will probably be interesting for some people, but for my use case, the current one is the best one, and the one you propose the worst. No silver bullet.

[slandelle]

See #1009.
But we won't go with immutable builders. PITA to implement in Java.