HttpClient cannot make simple GET requests

[jakehschwartz]

Using the HttpClient, initialized similar to this, certain requests are not sent.

Expected behavior

An HTTP request can be made and is received by the server.

Actual behavior

The server does not get a request and the HttpClient returns a 400 Bad Request with no content string.

Steps to reproduce the behavior

I created a sample project jakehschwartz/finatra-webhook that should have all documentation in it. It attempts to make a request to webhook.site.

[scosenza]

Hi Jake. Can you see if the Firebase example which performs GETs and PUTs works for you? https://github.com/twitter/finatra/blob/develop/examples/twitter-clone/src/main/scala/finatra/quickstart/firebase/FirebaseClient.scala#L23

[jakehschwartz]

Hi Steve, the reason I did not follow that example is that I will be using multiple clients and the HttpClientModule, which the Firebase client example uses, does not allow for multiple clients.

[jakehschwartz]

I think I have found the culprit:

object WebhookHttpClientModule extends TwitterModule {

  @Singleton
  @Provides
  @WebhookHttpClient
  def WebhookHttpClient(mapper: FinatraObjectMapper): HttpClient = {

    val dest = s"${config.webhookDest}:80"
    val service = RichHttpClient.newClientService(dest = dest)

    new HttpClient(httpService = service, mapper = mapper)
  }
}

To fix this, I changed the line that initialized the HttpClient to

new HttpClient(httpService = service, mapper = mapper, hostname = dest)

Maybe that field should not be optional?

[yufangong]

@jakehschwartz Thanks for sharing the solution! I believe the default hostname is being used in some services or tests, so I think we should keep it optional for now. I'm closing this issue, feel free to reopen it if there are further problems. Thanks!

[mosesn]

@jakehschwartz it does look like documenting HttpClient a bit better here would have saved you some grief. Would you be interested in adding a comment for the constructor which explains the parameters?

[jakehschwartz]

I'm having issues setting up the project but it looks like changing the removing the default hostname would only affect the following classes:

examples/twitter-clone/src/main/scala/finatra/quickstart/modules/FirebaseHttpClientModule.scala
http/src/test/scala/com/twitter/finatra/http/tests/integration/multiserver/add2server/Add1HttpClientModule.scala
httpclient/src/main/scala/com/twitter/finatra/httpclient/HttpClient.scala
httpclient/src/main/scala/com/twitter/finatra/httpclient/modules/HttpClientModule.scala
httpclient/src/test/scala/com/twitter/finatra/httpclient/HttpClientStartupIntegrationTest.scala

[mosesn]

@jakehschwartz I think we're less worried about inside of finatra and more about users of finatra ;). If possible, we want to avoid breaking APIs for our users.

[jakehschwartz]

Totally understandable, but from my limited use of the API, it seems like it's already broken.

[mosesn]

@jakehschwartz it sounds like your server requires clients to provide a host header when they send a request, but this isn't necessarily the common case. The HTTP spec expects HTTP requests to be over the web, but since our clients are often used inside of a data center, it isn't always meaningful to provide a host header, especially if you're not using DNS for load balancing.

Does that make sense, or does that not jive with your understanding of the situation?

[jakehschwartz]

Ahh, that makes sense. After running a curl -v to my test endpoint, it seems that it is setting that Header for me. When I unset that header it fails.

And you point about the other use case about talking to other servers in a private network makes sense.

I'll create a PR with some documentation in the http client some time this weekend.

[jakehschwartz]

@mosesn https://github.com/twitter/finatra/pull/420

[mosesn]

Nice, the "enhance your calm" PR.