Http client snippet from quickstart does not work

[note]

The following snippet (taken from https://twitter.github.io/finagle/guide/Quickstart.html#using-clients) does not work:

object Client extends App {
  val client: Service[HttpRequest, HttpResponse] =
    Http.newService("www.google.com:80")
  val request =  new DefaultHttpRequest(
    HttpVersion.HTTP_1_1, HttpMethod.GET, "/")
  val response: Future[HttpResponse] = client(request)
  response onSuccess { resp: HttpResponse =>
    println("GET success: " + resp)
  }
  Await.ready(response)
}

Different servers response with different non-ok responses. Adding request.setHeader("Host", "someDomain.com") fixes the problem but it's very astonishing and actually hard to find out - usually you expect code from quickstart to just work.

In HTTP 1.1 header Host is mandatory so I don't know if docs update is needed or should it be fixed on DefaultHttpRequest level.

[mosesn]

Good catch! Could you make a quick PR to update the docs?

[note]

Should not http client be responsible for setting Host header if no such header found in the request? If you decide that not then it is not a problem for me to submit a quick PR for updating docs.

[dschobel]

Having our client add the header sounds like a good change since the RFC is unambiguous about the presence of this header, even with an empty value.

It’s hard to imagine a user not wanting the Host header to be set other than testing malformed requests in which case they can explicitly unset it.

[olix0r]

Http.newService(String) takes a Resolver string which is not strictly a hostname. Due to the way that naming works in finagle, it's tricky for the client to know what the "host" header should be. Perhaps it makes sense to configure this via the client's Stack? Or add special helper builders on the Http object?

[mosesn]

Right, @olix0r's concern is what I'm worried about. We could do a reverse DNS lookup, but it's not necessarily obvious what we should do for multiple ip addresses, especially if they end up having different hosts. We also depend upon the assumption that you can explicitly set your host for ClientBuilder#tls.

The slightly tricky thing about @olix0r's solution is that since the version of the protocol that you're speaking is dependent on the request, not on the client, it's difficult to know in advance whether the client needs to be able to set a hostname or not.

Maybe we could log loudly when you send a request which doesn't conform to the HTTP protocol?

[dschobel]

If the resolver inserts the host value in the Metadata map of the Addrs it emits and we install a http filter that sets the header, that would at least solve the case where we create a new service instance with a fully qualified url, no?

[mosesn]

Maybe? But it means we need to add special logic–do we want to do it only for inet! when there's only one host, or do we want to guess if there's more than one host, do we want to strip http:// prefixes, how precise do the host headers have to be, can we send prefix.twitter.com if we really mean twitter.com, etc. It seems like a huge headache to support, when we can (and in my opinion, should) push it back to the user, who should know the name of the virtual host anyway.

[luciferous]

I agree with @mosesn about pushing it back to the user.

I think we should just fix the docs for now, but also provide a more convenient high level DSL which addresses these concerns.

[mosesn]

:+1: to high level DSL that addresses these concerns. Not 100% sure that should be in finagle proper, might be a separate wrapper (finch-like maybe?). For example, I doubt that it's really all that common that you want to switch between http versions, and then we could require more information at client creation or compilation time.

[dschobel]

@mosesn Is dispatching a known bad request in the hopes of a fully qualified request uri and a permissive http server going to be less surprising to users than trying to fix the request? I think the user is rolling the dice in either case and with the latter you at least have a valid request object. Maybe the right answer is to fail bad requests before dispatching (which is what finatra does).

[vkostyukov]

For now, Finch doesn't do anything special about consuming HTTP (Finagle) services. But there is a ticket for this, which is planned for 0.8.0 release.

[luciferous]

@dschobel This is a slippery slope. If we take on responsibility for guaranteeing the validity of the requests sent, then should we also set Content-Type if there isn't one (not that we could even know this)? Or failing HTTP/1.0 requests with Connection: keep-alive? Or ensuring that the Content-Length actually matches the length of the message content? Or...?

[dschobel]

@luciferous maybe that's why all the api frameworks on top of finagle build their own request builder libs? it might be a sign :)

[olix0r]

Erroring will needlessly break all sorts of services that aren't strictly rfc compliant. Finagle is a transport layer and imo shouldn't be too finicky about this sort of thing. Adding extra opt-in helpers to build http/1.1 compliant clients is a fine idea, and I understand that the support burden for this is high, so I think logging warnings is a good compromise. But please, don't make finagle enforce somewhat arbitrary policy if the application doesn't require it.

Oliver Gould ver@buoyant.io

[luciferous]

@dschobel I'm not denying the existence of the problem, I think we should build this, but it should be separate.

(And since we're observing signs, consider what it means that there are so many frameworks.)

[dschobel]

okay, I buy that it shouldn't be part of the protocol impl.

[note]

I see that it's broader topic, so created a tiny PR with doc update just not to confuse users (I really spend more than hour on this)

[dschobel]

closing since the doc fix has merged. thanks @note.