Duplicate Connection headers added to request when connection pooling is disabled

[johnstok]

When disabling connection pooling (via AsyncHttpClientConfig#setAllowPoolingConnection()) requests are invalid:

1. Two Connection headers are present in the request;
2. The Connection header should have the value close to indicate connections will not be re-used.

The following example reproduces the problem:

import com.ning.http.client.AsyncHttpClient;
import com.ning.http.client.AsyncHttpClientConfig;
import com.ning.http.client.ProxyServer;
import com.ning.http.client.Request;
import com.ning.http.client.RequestBuilder;


public class DuplicateConnections {

    public static void main(final String[] args) throws Exception {

        final AsyncHttpClientConfig cf =
            new AsyncHttpClientConfig.Builder()
                .setProxyServer(new ProxyServer("localhost", 8888))
                .setAllowPoolingConnection(false)
                .build();
        final AsyncHttpClient asyncHttpClient = new AsyncHttpClient(cf);

        final Request request =
            new RequestBuilder("GET")
                .setUrl("http://www.google.co.uk/")
                .build();
        asyncHttpClient.executeRequest(request).get();

        asyncHttpClient.close();
    }
}

[johnstok]

Note, using the JDKAsyncHttpProvider there are still 2 connection headers but these are correctly set to close.

[slandelle]

@johnstok I don't think you're seeing two Connectionheaders but one Connection and one Proxy-Connection.
@jfarcand Is there a meaning of disabling connection pooling and sending keep-alive instead of close?

https://github.com/AsyncHttpClient/async-http-client/blob/master/providers/netty/src/main/java/com/ning/http/client/providers/netty/NettyAsyncHttpProvider.java#L702

[slandelle]

As 1.7.12 might be the last one of the 1.7 series, it might be good to have a decision here.

[jfarcand]

@slandelle Should be close IMO. I think it a mistake right from the beginning.

[johnstok]

Is the expected behaviour that if setAllowPoolingConnection is false then Connection should still have the value keep-alive?

At the moment providers behave differently:

Netty:

GET /
  Connection: [keep-alive]
  Host: [localhost:8080]
  Accept: [*/*]
  User-Agent: [NING/1.0]

JDK:

GET /
  Connection: [close]
  Host: [localhost:8080]
  Accept: [*/*]
  User-Agent: [NING/1.0]

My view is that Connection should be set to close.

[slandelle]

@jfarcand Go for close when connection pooling is disabled? If so, I'll open a new issue (that's a different one from this header duplication thing)

[johnstok]

With proxying enabled there are definitely two Connection headers, not a Connection header and a Proxy-Connection header. The below outputs are proxied via Fiddler 2.

Netty:

GET /
  Connection: keep-alive
  Connection: keep-alive
  Host: localhost:8080
  Accept: */*
  User-Agent: NING/1.0

JDK:

GET /
  Connection: close
  Connection: close
  Host: localhost:8080
  Accept: */*
  User-Agent: NING/1.0

[jfarcand]

@slandelle +1

[johnstok]

FYI, the duplicate header issue seems to be a bug in Fiddler 2 such that Proxy-Connection headers are being converted to a second (duplicate) Connection header.

[slandelle]

Thanks for the feedback