Adopt reactor-netty 0.9.0.RELEASE

[nictas]

Several notes in no particular order:

• The code in this pull request may be formatted a bit differently than the rest of the code base, because the formatter I've used is not exactly the same. Most notably, the imports may be ordered with a different algorithm.
• There is one HUGE problem that is a result of a limitation in Reactor Netty's 0.8.10 version. Upload of files (application binaries, buildpacks, etc.) can lead to OutOfDirectMemoryErrors if the size of the file exceeds a certain threshold (around 10MBs). This problem is marked with a FIXME comment in the MultipartHttpClientRequest and is reported to Reactor Netty in Chaining methods in NettyOutbound has weird results reactor/reactor-netty#811. In my opinion, this pull request should NOT be merged until that bug/limitation is fixed and a newer version of the library can be adopted. With that said, the pull request can be reviewed, as the fix for the issue should not affect any other classes besides MultipartHttpClientRequest.
• I've tested this pull request with the following integration tests:
  • Create an application, upload binaries, bind it to services, start, stop, delete it, assign routes to it, etc.
  • Create services synchronously and asynchronously (with polling of last_operation), create service keys.
  • Retrieve information about existing services, applications, space users (developers/auditors/managers) and other entities.
  • Start tasks and monitor them.
  • Test whether the requests/responses are logged when cloudfoundry-client.request and cloudfoundry-client.response are set to DEBUG.
• The adoption of the new version of Reactor Netty required significant refactoring and changes. I hope you don't mind. If you don't like something, let me know, and I'll see whether it could be improved.
• I couldn't find any good way of splitting this commit into several smaller ones, so apologies for the massive change. Hope you'll find the time to review it. :)
• This pull request may be a fix for Netty Leak & App Crash with OutOfDirectMemoryError #903, Incompatibility with Spring WebFlux 5.1 WebClient #938 and hopefully SSLEngine and NPE failures under high load #908 (which is why I'm doing this pull request in the first place).

[cfdreddbot]

✅ Hey nictas! The commit authors and yourself have already signed the CLA.

[twoseat]

I'm testing this change now, and in the meantime have flagged the related reactor-netty issue so hopefully we'll get a fix soon.

[nictas]

Thanks! Let me know if I can help in any way.

[nictas]

I ran our load tests against this change and unfortunately, I reproduced another issue, which seems to come from the Netty client. I tracked it here:
netty/netty#9459

The "good" news is that for about 20000 requests, I was able to reproduce it only 3 times.

[twoseat]

@nictas I've done an initial test run (takes a while!) and all looks good except for downloading a droplet. I'm pretty sure that's just an artifact of the way I'm running the tests, but if you have time and the ability to try downloading a droplet in your own environment I'd be interested in the result.

[nictas]

I'll try it out tomorrow.

[violetagg]

I ran our load tests against this change and unfortunately, I reproduced another issue, which seems to come from the Netty client. I tracked it here:
netty/netty#9459

Why do you use an old Netty version (4.1.29). Reactor Netty 0.8.10 runs with 4.1.36 (even pretty well with 4.1.38 except this issue netty/netty#9436, which I believe does not affect you)

[nictas]

Why do you use an old Netty version (4.1.29). Reactor Netty 0.8.10 runs with 4.1.36 (even pretty well with 4.1.38 except this issue netty/netty#9436, which I believe does not affect you)

I hadn't noticed that the Netty client's version is being downgraded until I saw your comment. Apparently, the version of Netty is not coming from Reactor Netty's transitive dependency, but from the spring-boot-dependencies POM, which this project inherits. I'll run our load tests again with Netty 4.1.38, but I don't see any mention of the NPE I encountered in their release notes. Fingers crossed though.

[twoseat]

For reference I've updated locally to Boot 2.0.9, which provides netty 4.1.34 (I'm also looking at Boot 2.1, but that's a little more involved). Seems to work ok, though I don't know about the impact on the load test issue.

[nictas]

Apparently, the Netty issue will be fixed in their next release:
netty/netty@23cf85f

[nictas]

@twoseat I reproduced the issue with the download of droplets. The problem is that the client does not follow redirects and even when it does, it forwards the Authorization header, which leads to 400 Bad Requests. I'll fix it ASAP.

[nictas]

Maybe I'm doing something wrong, but I can't seem to convince the Reactor Netty client to stop forwarding the Authorization header. I've opened this issue:
https://github.com/reactor/reactor-netty/issues/818
I'll update this pull request when I have a working solution or a workaround.

[nictas]

@twoseat I updated the pull request with my latest changes. The issue with the download of droplets and applications should be fixed now. I tried to download an application, a droplet and a package and all seemed good with one exception - I could not open the downloaded droplet. The file had a significant size of around 300MB, but apparently was not in a ZIP format like the application and the package. Is this normal?

[nictas]

Btw, we still should NOT merge this pull request yet. The upload of applications is still a problem for applications larger than 10MBs. @violetagg gave me a workaround for reactor/reactor-netty#811, but I'm having some problems with applying it to the cf-java-client. We're currently looking into what's going wrong and I'll update this pull request as soon as we have a solution.

[twoseat]

I believe the droplet is a .tgz file, did you try that?

[nictas]

You're right! The droplet is in fact a .tgz file. Well in that case, everything works at least as far as I can see (application upload excluded, of course). You can re-run your tests if you want.

[twoseat]

Thanks for the update (and the ongoing great work!) I'll take a look as soon as I have time. Have you tried using the 0.9.x RC? Depending on when netty release the fix we're waiting for it's possible we could move straight to 0.9.x.

[violetagg]

@twoseat @nictas Then if you would like to try 0.9.x, can you try instead of using your own implementation of multipart, to use the one in reactor netty -> sendForm?

[nictas]

@twoseat In terms of implementation, this PR is finished (at least until your review - I'll rework it, if you don't like how something turned out). I tested how the file upload works with reactor-netty 0.9.0.RC1 and everything's good at least so far. The only thing left from my side is to run our load tests against this change. I'll post results on Monday (probably).

[scottfrederick]

Thanks for the work on this @nictas and @twoseat. I know several teams who are anxious to see this working!

[nictas]

@twoseat All of our load and functional tests pass, so from my point-of-view this pull request can now be merged (assuming that you have no comments that need to be addressed). We could also wait for the official 0.9.0 release of reactor-netty.

One important note: We only use a subset of the cf-java-client's features, so our tests can't confirm that everything is working properly. I trust your integration tests to catch any issues that have slipped by our tests.

[twoseat]

Thanks @nictas - we'll wait for the official 0.9.0 release, but I'll start testing on this version.

[violetagg]

You should be able to update this PR to Reactor Dysprosium GA

[royclarkson]

@nictas and @twoseat I'm adding my thanks and 👍 for the work on this too. Really appreciate it.