HTTPCLIENT-2371: Improved request re-execution strategy

[ok2c]

@arturobernalg @michael-o @rschmitt @garydgregory Please do feel free to take a look at the proposed request re-execution API changes. Do feel free to propose better naming / better API ideas.

[rschmitt]

What's the context here? What problem are you trying to solve?

[ok2c]

What's the context here? What problem are you trying to solve?

@rschmitt HttpRequestRetryStrategy got out of hands. It requires two method calls where just one method should suffice. I would like to replace HttpRequestRetryStrategy by a cleaner, simpler interface.

[rschmitt]

@ok2c I think there should be a higher bar for breaking API changes than just general cleanup. It's a lot of churn to inflict on users, and a potential source of bugs. There was one issue for the 5.2 -> 5.4 upgrade which I never reported, which involved HttpVersionPolicy being moved to TlsConfig:

    @Deprecated
    public final HttpAsyncClientBuilder setVersionPolicy(final HttpVersionPolicy versionPolicy) {
        this.tlsConfig = versionPolicy != null ? TlsConfig.custom().setVersionPolicy(versionPolicy).build() : null;
        return this;
    }

This blows away the prior tlsConfig, which in our case contained our HttpVersionPolicy, so the upgraded client suddenly started speaking HTTP/2 in production, which caused an outage for reasons that aren't relevant here.

Additionally, a recurring issue I have as a maintainer is the lack of late binding in many of HC's APIs, especially internal ones. There are vast amounts of constructor delegation, default interface methods, arity overloads etc caused by the insertion of more and more parameters over time. I'd like any future APIs we add to permit more flexible evolution. The use of IOException here strikes me as suspicious, for example: checked exceptions don't compose with most of Java 8's functional features and may be wrapped any number of times, and in async code this gets even more complicated due to ExecutionException, frequent use of catch (final Throwable t), etc.

Finally, I've come to realize that Reinier Zwitserloot was right about option types (see also this post). The lack of any type system relationship between T and Optional<T> adds a ton of friction for basic code reuse and composition; it's worse than const_cast in C++. Option types only make sense in a limited context where you need to be able to call methods on a receiver (e.g. in streams, which is why Optional<T> was added). In the long term I think that Java, through the Valhalla project, is moving in a direction where nullability is not just explicitly modeled in source code, but is reified at runtime as well in a way that enables JIT compiler optimizations like layout flattening. So I think we should stay the course with nulls instead of trying to use option types in new APIs. We can use JSpecify annotations if we really must, although they entail some annoyances for consumers like breaking -Werror.

Edit: Here's the JEP for null-restricted and nullable types: https://openjdk.org/jeps/8303099

[garydgregory]

For constructors, I suggest we use the builder pattern: A builder class (which could be a record in recent Java versions maybe) and one private constructor which takes a builder instance as its single parameter. When you need a new toggle/parameter, you add it to the builder class and update the implementation of the one constructor.

[ok2c]

@rschmitt

1. I see Optional being abused on my day project to produce hideous results. In this particular instance however I felt the use of optional return type was justified as the intent of the interface call was to optionally trigger request re-execution. But conceptually I fully agree.
2. I do not understand your take on making API changes, especially given there were no breaking changes in this very change-set. You seem to be averse to the idea of doing a major release that would enable us to get rid of deprecated APIs and various default methods in interfaces, drop interfaces and primitives now provided by JRE that we have accumulated in the past 10 years. This I understand. But we cannot stop making gradual API improvements simply because that entails some extra work to upgrade. Otherwise the project will stagnate and fail. This I do not understand.
3. I do not understand your aversion to the use of IOException. HC is a low level transport library, after all. The problem that checked exceptions do not mix well with lambdas is a problem with Java lambdas, not our APIs.

[rschmitt]

I do not understand your take on making API changes, especially given there were no breaking changes in this very change-set.

True, but today's deprecation is tomorrow's breaking change, since the goal of deprecation is always in principle to remove the deprecated API.

But we cannot stop making gradual API improvements simply because that entails some extra work to upgrade. Otherwise the project will stagnate and fail.

In my dayjob, I'm responsible for maintaining the Apache client's integration with our main service framework, as well as the rollout of the Apache client itself company-wide, and a number of other open source libraries as well. I have the ability to rebuild an extremely large amount of code against the latest changes to a given library (including running all the unit tests), which means I eventually find and have to deal with every breaking change in every release. (This is why I've had so many bug reports to Log4j2 recently.) So I have a very good sense of how difficult upgrades are at scale, and how limited the "breaking change budget" is for a widely-used library.

I prefer to see this budget spent on things like bugfixes, which will often break people who are already broken and don't realize it. (Excessive bugwards compatibility can really stagnate a project.) API reorganizations are okay as long as there's a (working) migration path, but I still think there needs to be a good reason to commit to the eventual removal of an API. Otherwise there's no end to the refactoring that you can do, and I've seen lots of frankly frivolous deprecations cause issues at scale. A good example is File.exists? being removed from Ruby 3.2.0 when the trivial delegation to File.exist? could have been left in place indefinitely.

Similarly, major version bumps can be reasonable, but there has to be a really compelling reason to do them, and I don't think they should be done more than every ten years or so. I specifically think that JEP 14 is a terrible idea: Java libraries should not follow the latest LTS release of the platform, that's way too much instability at the scale of the Java ecosystem, and we've done well so far with workarounds (like reflection) to make use of newer JDK features (like Unix domain sockets, or ALPN/TLSv1.3 before they got backported to JDK 8 in MR3).

I do not understand your aversion to the use of IOException.

My concern was that IOException is needlessly specific in a public API, considering that widening this to Exception or Throwable is a breaking change. I tend to not assign a lot of meaning to specific exception types, because of the pervasive and often forced use of wrap-and-rethrow.