(TK-23) Port to apache HttpAsyncClient

[cprice404]

• Ports the code over to use the Apache HttpAsyncClient library instead of http-kit, as we were getting some weird and inconsistent SSL errors from http-kit, and
• Simplify the API (by eliminating a lot of the supported configuration options). Some of these options we'll eventually want to add back in, but for now, getting rid of them makes the API less cluttered and also gives us the opportunity to do a better job writing tests for new options when we do add support for them.

For now it only supports constructing a new client on every request. I intend to add API for creating a client explicitly, and a protocol for how to interact with the client. This will allow users to re-use an existing client as they see fit... but this hasn't been implemented yet.

[ghost]

Somehow the single commit in this PR was made by "Jenkins User." It would be sweet if Old Man Jenkins could write code of this quality, but I'm guessing he didn't do this.

[KevinCorcoran]

@cprice404 - Since part of your goal with this PR seems to be to validate the decision to swap-out http-kit with httpcomponents, would you mind explaining why you think that's the right move?

I haven't been following this issue closely, but from an outsider's perspective, I don't quite see this being worth it yet; the downside of http-kit seems rather small - the inconsistent exception type seems like a very minor issue to me, and these code changes seem rather large (although I haven't dug in, so I don't know how much of these changes are related to this and how much is made up of the API changes).

I'm sure you've thought about this more than I have, so I don't mean to put the kibosh on this, but I'd be interested to hear your reasoning.

Also, would it be possible to separate this work out into 2 pieces? One being the switch from http-kit -> httpcomponents, and the second part being the API changes? I think it'd make it a lot easier to review.

[cprice404]

Reasons to switch:

• The inconsistent exception behavior just makes me really nervous about the maturity of the library.
• I've found other minor bugs in it, such as it handling redirects differently under HTTP than HTTPS.
• It seems like a safe assumption that the apache library has much, much more mileage on it than http-kit does.
• At the time when we chose http-kit, we didn't realize that we were going to need to have a java API for ruby code to interact with, and that made http-kit slightly less of a good fit.
• The http-kit API involves passing in a map of largely mysterious options. That map is then passed around through a bunch of other functions and transformed in other mysterious ways, so by the time you get to the end of the call stack, it's pretty hard to reason about what keys can be / will be present in the map.

So, moving in this direction, in my mind, allows to be a lot more careful / prescriptive about what the API should look like, and also build on top of a library that gives me less concern about stability / maturity.

As to breaking this into two commits, I don't think that can realistically be done without investing a bunch more time into it and making the PR much larger... the reason is that the API changes are all removals; to submit a PR that didn't change the API I'd have to actually implement all of the flags/features that were exposed in the previous API.

[KevinCorcoran]

That makes sense, thanks.

[MSLilah]

It's very possible that I missed something, but in my view the test coverage and the reasoning for the changes both seem reasonable.

[nwolfe]

:see_no_evil:

[KevinCorcoran]

I briefly reviewed the tests and main clojure and java APIs and don't see any problems. :see_no_evil:

[cprice404]

Pushed commits to fix all of the obvious stuff above. There were a few comments above where I left a question, so if folks think that additional follow-up is needed for those threads then I can add more commits.

[nwolfe]

Assuming this is done and awaiting merge...

I think I'm done with reviewing this, and looks @KevinCorcoran is too. @jmay-puppet was in here a bit too. @camlow325 you want in on this or should I push the button?

[camlow325]

Sounds like we've had a pretty thorough review on this one so, yeah, @nwolfe, I'm good with you pushing the button on this one.

[cprice404]

@pcarlisle wants to take a quick look at this too.

[pcarlisle]

this seems fine for us

[KevinCorcoran]

I left one more nitpick but it's very minor. I'm going to merge this since it's been open for ages and it seems like all parties are :+1:

[cprice404]

The other usage of ByteArrayInputStream is in a test for the java API... so callers would likely not have access to the clojure libraries, so it seems better to test it this way.