Split keepalive from pipelining in HTTP connections (bug https://bugs.ec...

[jlorgal]

...lipse.org/bugs/show_bug.cgi?id=434402). A new property 'pipelining' is added to HttpClient to disable HTTP pipelining (by default, pipelining is true to maintain backwards compatibility)

[ddimensia]

By defaulting this to true all non-Java clients are forced into pipelining enabled. An http server that closes it's connections will cause any pipelined request after the first to fail with a timeout. Since the pipelining support is effectively broken shouldn't this have been defaulted to false?

[purplefox]

It default to false in Vert.x 2.0 to preserve previous defaults and not break peoples applications when they upgrade.

BTW What is broken about pipelining support?

Also... in Vert.x 3.0 the HTTP client has been completely rewritten and doesn't exhibit this behaviour.

[ddimensia]

Well, there's actually a bug in keep alvie support, specifically it doesn't respect the 'Connection: close' header returned in a response. In that case it shouldn't put the connection back into the pool and shouldn't pass the connection to a waiter in the wait queue.

I have a test that uses a connection pool size of one that fires two requests simultaneously to a server that passes back 'Connection: close' (and subsequently closes the connection). In that case every other request gets a request timeout because the non-persistent connection is being passed directly to the waiting request.

Regarding pipelining specifically, I think it's been pointed out elsewhere that the Vertx pipelining implementation doesn't follow the RFC. Specifically it does perform a resend on connection close, or at least I haven't found the code and if it did do that I wouldn't get a request timeout in the test listed above with pipelining enabled.

Beyond whether Vert.x's implementation of pipelining is correct or not, pipelining isn't always well supported be servers and there are lots of implications in using it. A caller needs to understand which of it's calls are idempotent so that it knows it won't have an unintended side effect due to pipelining. Because of this I would think that enabling pipelining should be a consciences effort by the developer using the library and not a default. Also, since the non-Java language binders don't currently expose the setPipelining method you're forcing it on all non-Java users with no ability to disable it but to also disable keep alive.

[ddimensia]

Tim, please see #876 for a solution to some of the issues with server side closed connections, the Connection: close header and pipelining. It doesn't do the resend of pipelined requests but it does provide the caller with more insight into the failure rather than simply getting a request timeout with no knowledge if it was because of a connection close or not.