Upgrade reboot to Async http client 2.1 and unfiltered 0.9.0 final

[fbascheper]

One of the problems with pull request #128 is that it brings in netty 4.0 and netty 4.1 which are incompatible with each other. Instead I propose to upgrade to AHC 2.1 (alpha 2 currently) and ws.unfiltered 0.9.0 (final) which both bring in netty 4.1.7.

I've rebased against the 0.12.0 branch and updated the version to 0.13.0 as well.

[fbascheper]

Note that this AHC 2.1.x (and 2.0.x requires JDK8) which should be mentioned in the release notes.

[leonardehrenfried]

Java 7 is EOL and Scala 2.12 requires Java 8 so I think it's totally reasonable to depend on it.

[fbascheper]

@mdedetrich Can you take a look at this?

I added a test for the exchange class, based on this blog entry: http://akiomik.hatenablog.jp/entry/2014/02/03/055408 . LGTM for now.

[ashawley]

@mdedetrich Seems a 0.13.0 branch should be started from this PR. No need to actually merge it to 0.12.0.

Things like fixing OAuth tests should be a separate pull request. Seems like it could be useful to the 0.12.0 branch. Unless, it only affects AHC 2.1... @fbascheper, is the OAuth a bug that only affects AHC 2.1?

[fbascheper]

Yes, because the oauth code no longer compiled because AHC 2.1 changed the org.asynchttpclient.oauth.OAuthSignatureCalculator substantially. Most importantly the public method calculateSignature became package private, and the parameters changed as well.
So I changed the code to use the public void calculateAndAddSignature() method instead.
But I had to extract the result from the auth headers. I also made a mistake and decided to add a test for this code as well.

[fbascheper]

And since AHC 2.1 changed their public API and brings in a new Netty version as well a new version range (0.13) seemed appropriate.

[fbascheper]

@ashawley Thanks for the hands-up. I've created a separate PR for the 0.12.0 branch (#140) with the oauth exchange test (only the imports are different). And this test passes there too.

[ashawley]

This plug-in doesn't need to be brought in to the project. Best to use it as a global plug-in by adding it to ~/.sbt/0.13/plugins/plugins.sbt on your local machine.

[fbascheper]

Oops, thanks for finding this. I'll update the PR to remove this plugin ASAP

[fbascheper]

Done.

[ashawley]

Seems the motion on the floor is

• Skip AHC 2.0 support for the reasons stated above by @fbascheper
• Start working with AHC 2.1 early even though it's alpha.

I presume there isn't anything redeeming in AHC 2.0 since it seemed to be a work-in-progress of a major version update anyway.

[fbascheper]

AHC 2.1 will be released once Gatling can upgrade to Netty 4.1.
See AHC mailing list.

[saucam]

+1 Tested using this branch with elasticsearch 5.0. Works great

[farmdawgnation]

Most of this looks pretty straightforward, but I want to spend some more time groking the best way to bring these changes in before I give this the seal of approval.

The biggest issue that I noted was the fact that as this stands it's pulling it a pre-release version of AHC, which isn't ideal. We should ideally only pull in fully release versions of AHC.

Let me know what you think. I'm going to keep noodling the best way to bring in these changes without risking breaking anything (or, ideally, unexpectedly breaking someone else's code).

[farmdawgnation]

We shouldn't ship something depending on a pre-release version of AHC. 2.0.x would be preferable.

[fbascheper]

Agreed, but don't forget that dispatch is currently using 1.9.x which is a actually pre-alpha of 2.0.x. The problem with AHC 2.0.x is that it's using a version of netty which is incompatible with the netty version used by unfiltered.ws

[fbascheper]

That's why this should not go in a 0.12.x release, but live inside the 0.13.x branch. We might consider to ship this in a beta release, to allow people to test this before a final version.
I contacted the AsyncHttpClient author about a release schedule, but that's dependent on gatling.io, an open source / commercial testing framework.

[fbascheper]

I will update this PR against AHC 2.1.0 alpha 16, but probably not before Wednesday. I'll fix the branch conflicts as well in the updated PR.

[farmdawgnation]

Alright, I've given this some careful consideration.

First, I think this PR introduces enough changes that I'm wary about including all of it on the 0.12.x line. There are some things in here that look like some important bugfixes for 0.11.x and 0.12.x so I'm going to work toward including those and spinning out builds where I can.

The problem with AHC 2.0.x is that it's using a version of netty which is incompatible with the netty version used by unfiltered.ws

That's a problem today, yes? As in, we're not making things worse for folks who use unfiltered by preserving stability on our end, are we?

Transitive dependencies are an exponentially increasing problem. Nobody using unfiltered will want to use Dispatch if we bring in a bunch of bugs from an alpha version of AHC. That said, I'm sympathetic to the need to move forward and we should be continuously testing against the next version of AHC if we can.

A compromise then. I'm not going to merge an alpha build of AHC into master, but we can stick it in a branch and publish snapshot or beta designated builds from it. How does that sound?

[fbascheper]

I agree. Actually, that's exactly what I intended originally, even though I hoped that AHC 2.1 would be released sooner rather than later. A lot of development has happened on their side recently, so who knows. If you appreciate it, I can try to keep your upstream branch up to date with the AHC releases. I've fixed up a couple of API changes already.

[farmdawgnation]

@fbascheper Yes, that would be super helpful.

I'm at Kafka Summit in NYC today, but later this week I'm going to finish some of the branch reorganization work and (hopefully) release some bugfixes to 0.12. Once I have that new branch (I'll probably name it master-ahc-upgrade or something) set up I think you should create a new PR that only includes the AHC version bumps and associated changes targeted at that branch.

I'll make sure to periodically sync this branch as PRs get merged into master. I may ping you for help if that causes issues with the AHC upgrade.

For now, let's leave this PR open so I don't forget to create the new branch for you later this week.

Does all that seem reasonable? :)

[fbascheper]

Definitely, yes.

[simonstuck]

New guy here, hello, world!
It seems the decision is to skip AHC 2.0, any chance of reconsideration here? :) Play 2.5 uses AHC 2.0 so if it's not too much of a hassle, it would be greatly appreciated if 2.0 could also be supported.

So 0.13 -> AHC 2.0, 0.14 -> AHC 2.1 ?

[farmdawgnation]

New guy here, hello, world!
It seems the decision is to skip AHC 2.0, any chance of reconsideration here? :) Play 2.5 uses AHC 2.0 so if it's not too much of a hassle, it would be greatly appreciated if 2.0 could also be supported.

Wowza. Dependency hell.

@fbascheper Is there any chance that AHC 2.0 and AHC 2.1 are binary compatible? We could declare our compatibility with either if they are. If there have been significant API changes, however, we're kind of in a pickle.

In any event, I've finished my branch re-shuffling. Can you please do the following for me?:

• Open a PR against master upgrading it to AHC 2.0
• Open a PR against master_with_ahc2.1 upgrading it to the latest AHC 2.1 alpha/beta.

I'm going to continue thinking about the best way to move forward so that we can wrest ourselves out of this version interlocking hell with a bunch of other libraries. I openly welcome suggestions here, because the best idea I've got so far is to cut our own abstraction interface - an idea that does not thrill me.

[farmdawgnation]

@simonstuck Also, if it wasn't clear, we're going to take a look at doing at release with AHC 2.0 given that it's the current stable release.

[simonstuck]

Brilliant, thanks all!

[fbascheper]

I've looked (again) at building dispatch against AHC 2.0. Everything compiles OK but I cannot get the tests to pass. The reason for this is simple: AHC 2.0 uses netty 4.0.x and Unfiltered uses netty 4.1.x and their interfaces conflict in too many ways. The end result is usually something like:

java.lang.NoSuchMethodError: io.netty.handler.codec.http.HttpRequest.uri()Ljava/lang/String;
at unfiltered.netty.RequestBinding.uri(bindings.scala:71)

The only viable way is when we can find a way to drop the unfiltered dependency, and start a server ourselves and run the tests against that. I'm open to suggestions here (library, etc.)

[farmdawgnation]

Womp womp. Okay. Thanks for looking at this. I'm going to take a closer look at this tonight after work and see what suggestions I can come up with.

[fbascheper]

I've pushed my changes to https://github.com/fbascheper/reboot/tree/feature/upgrade-AHC-2.0.32 . Doesn't make sense to make a PR in this state yet. I've spent an hour trying to get scalatra working as a replacement, but no luck just now.

[farmdawgnation]

Okay, I've come up with a solution.

Unfiltered 0.8.4 has a compatible version of netty. It was already published for Scala 2.11. I published a 2.12 version on my personal bintray. If you open a PR with your changes and set it to let me contribute to it, I can make the build file changes to make it happy.

[ashawley]

In unfiltered/unfiltered#338, I had suggested publishing unfiltered 0.8.5 for Scala 2.12, but it didn't find any backers.

[farmdawgnation]

Well, if they publish it officially they have to support it. I understand their reluctance to doing it.

However, since we just need it for tests I have no problems just publishing the modules we need for our tests to my bintray account. =)

[fbascheper]

It's all understandable from both sides. But the idea of depending upon a library in someone's personal bintray doesn't seem very attractive to me either. If we want to accept PR's and maintain this branch in the future, then that's not going to work out well I'm afraid.

If we are going to create a custom build of unfiltered 0.8.4 for our testing purposes, why can't we publish it to sonatype under the net.databinder.dispatch group, with the artifact name something like dispatch-testing-unfiltered? Maybe not the ideal solution, but at least this allows future contributors to build and test the dispatch library.

But before doing this, it's fair to contact the unfiltered guys first to see if they have any objections.
Let's decide first how to proceed and then I'll create another PR to move forward.

[farmdawgnation]

Let me clarify a few points that weren't clear:

First, this is only for test code as far as I can tell. If I'm wrong on that, please correct me. This would not be a "valid" solution in my mind if we were shipping unfiltered as a real dependency.

Second, I do not consider this a long term solution. We'll move to unfiltered 0.9.x in the branch that upgrades AHC to 2.1. My personal, free bintray account isn't going anywhere and I'm not planning on deleting those artifacts anytime soon. With the proper build settings in the build.sbt file, anyone who spins up a build of Dispatch that depends on that library will be able to build it.

Third, I emphatically do not want to publish that to Sonatype and risk confusion that either a) this is a valid release of unfiltered for people to use or b) this is some component of dispatch that people should consider. Sticking it in an isolated repository prevents any kind of confusion for The Folks Who Come After Us™. I think creating any kind of confusion for folks that are exploring Sonatype is a far worse offense than spinning a custom, isolated build of unfiltered to get us over this dependency hump.

I think for the reasons outlined above, we stick with the Bintray dependency and move forward from there. WDYT?

[ashawley]

With the proper build settings in the build.sbt file, anyone who spins up a build of Dispatch that depends on that library will be able to build it.

Sounds like the matter is settled then?

[fbascheper]

I've just verified that unfiltered is only used for testing dispatch. No dependencies are leaked other than ufcheck, as you can check at maven central.

[fbascheper]

I've squashed all this in the PR to upgrade to AHC 2.0.32. See #149 . Once you've updated the build in that PR against your unfiltered 0.8.4 I'll try to fix the tests.

[farmdawgnation]

This was superseded by #149 being merged.