HTTPCLIENT-2381: use system properties as defaults in builder classes

[ok2c]

This turned out to be a big change-set but mostly due to many test classes affected by the API changes.

@arturobernalg @rschmitt @garydgregory Please review. For context please see

https://issues.apache.org/jira/browse/HTTPCLIENT-2381
https://lists.apache.org/thread/n1ckyf4soz0djw0bfo2sy3bhsso30vmj

[arturobernalg]

@ok2c LGTM

[ok2c]

If I hear no loud objections I will merge the change-set in the coming days

[rschmitt]

I'm going to need some time to get caught up on this. I won't be able to do any non-trivial testing until I get back from vacation in about a week.

[rschmitt]

I think "defaults" are a red herring here. The Apache client defaults are never going to work for the original requester, because the client is obviously not going to use his particular corporate proxy as the default value for proxyHost and proxyPort. All he's asking for is a way to override the defaults, without changing code:

Should this project be changed so that by default without code you can use system properties to configure the proxy?

All we need to do to solve his problem is define a new system property for the Apache client that, when set, has the same effect as calling .useSystemProperties() everywhere. This fixes the actual problem (locus of control) in a fully backwards-compatible way and without all the API churn I'm seeing in this PR. We already seem to be in agreement that "the client never queries system properties by default" is a totally obsolete constraint (and the Java platform feature that motivated it is deprecated for removal), so we're well within our rights to add a new opt-in system property that meets this need. The API impact would be zero, and the code change would probably be small enough to backport to previous versions.

Regarding backwards compatibility: flipping the default behavior after twenty years is virtually guaranteed to cause problems. There may be applications with inert copy-pasted system properties that don't actually work. There may be applications that are using those system properties to configure other libraries, e.g. HttpsURLConnection, while supplying the equivalent configuration to Apache programmatically. There may be test suites with fixtures or parallelization configs that don't expect client configuration to be affected by system properties (global mutable state). There's just a huge list of things I don't have to worry about if we simply make this an opt-in behavior (which, as I pointed out, it already is -- it's not a "default" in the sense that a socket timeout value or a retry strategy is a default).

[ok2c]

Regarding backwards compatibility: flipping the default behavior after twenty years is virtually guaranteed to cause problems.

@rschmitt So, what are we going to have at the end of it? The old default behavior that we all agree is outdated and wrong will still be there but on top of it we are going to introduce some non-standard system property to flip the behavior. So, instead of fixing the problem we are going to make it even messier. Is this wise?

Every time we make a minor release there is always someone unhappy because it turns out they should have read the release notes and / or have moved off some deprecated APIs.

We should be fixing problems as we discover them even at the cost of upsetting a few folks. Fixing problems properly pays off in the long run.

[rschmitt]

@rschmitt So, what are we going to have at the end of it? The old default behavior that we all agree is outdated and wrong will still be there but on top of it we are going to introduce some non-standard system property to flip the behavior. So, instead of fixing the problem we are going to make it even messier. Is this wise?

Yes. It's wise, because it maintains backwards compatibility while following established conventions for Java library development. It's totally normal for Java libraries to define their own namespaces for system properties (e.g. io.netty for Netty, org.asynchttpclient for async-http-client). In fact, async-http-client did exactly what I'm proposing we do, in order to accommodate the same use case.

What I've conceded is both outdated and wrong is the insistence on not even querying system properties. Beyond that, it's not clear to me which system properties should be respected by default, even without taking backwards compatibility into account. The proxy-related properties seem reasonable; http.agent does not.

Every time we make a minor release there is always someone unhappy because it turns out they should have read the release notes and / or have moved off some deprecated APIs.

I don't like changes that presume upon our users' ability to "just" move off deprecated APIs. Not everyone controls their execution environment and their runtime classpath. The caller of some deprecated API may itself be a library, which doesn't get the final say in which versions of httpcore5 and httpclient5 are present at runtime. Or a user may be deploying code into a runtime image they don't control, which provides third-party dependencies like the Apache client (I've worked with EMR before, which is an example of this sort of thing). We shouldn't assume that users can upgrade their Apache client and modify their code to account for breaking changes in a single atomic update, and we shouldn't even assume that users can code against a single version of the Apache client as opposed to a whole range of versions.

We should be fixing problems as we discover them even at the cost of upsetting a few folks. Fixing problems properly pays off in the long run.

This brings me back to my original point: the actual problem we are trying to solve is one of locus of control, and this problem has an easy, 100% backwards-compatible fix. There's no need to get sidetracked arguing about what the defaults are, whether to change them, how many API symbols to add or deprecate, etc. My repeated experience dealing with software upgrades at scale is that most breaking changes are frivolous, in the sense that the goal they were aiming at could have been done nearly as easily in a backwards-compatible way. I'd rather save our breakage budget for the bugs that can't be fixed compatibly.

[ok2c]

@rschmitt I find "all cool kids do that" kind of argument completely unconvincing. Especially when async-http-client is used as an example. HttpClient should be using standard properties defined by the platform or use code configuration. I am strongly not in favor of defining project specific system properties. What we have now is outdated and wrong, but at least it is conceptually consistent. I rather keep what we have and drop this change-set and close the issue as WONTFIX.

[rschmitt]

HttpClient should be using standard properties defined by the platform or use code configuration.

My understanding is that the "standard properties" are part of the standard for the JDK itself. The documentation states that these properties "alter the mechanisms and behavior of the various classes of the java.net package." Third-party libraries are not called upon to do anything in particular with the java.net system properties.

I prefer configuration to generally be done through code (the use of system properties in java.net to control things like User-Agent and maxConnections is utterly archaic), but the unique advantage of system properties is that they allow the default behavior of the code to be overridden by someone who knows better config values than the author of the code. This is a really important special case for proxy configuration and TLS trust stores. I'm sympathetic to the idea that these system properties should be respected by default, but I think it's even more important that a Java API like ignoreSystemProperties() have a corresponding system property that allows end users to override that behavior. If everyone were to start using ignoreSystemProperties(), then we'd be right back where we started: end users can't get their applications working through their proxies without making code changes. That's the problem I want to solve.

I'd be willing to consider a default behavior change just for the proxy behavior. I'm concerned that categorically switching the default behavior for all system properties we handle is too broad a change; that would potentially affect a lot of unrelated things at three or four different OSI layers. But I'd also like you to consider that there are really good reasons why most libraries define their own system properties, and that "standardization" isn't really the right framework for making this decision. Imagine an HTTP client that only supported "standard" HTTP headers.

[garydgregory]

FWIW, I like the concept in general that system properties are the escape hatch of last resort.

I see the hierarchy in general as:

My Java code overrides the default behavior of objects constructed (using constructors, methods, and builders). My code can take system properties into account when calling a library's APIs. This let's my app decide whether or not to override what.

System properties provide default values for objects in the library. If I don't specify a value, it comes from a library's defaults which itself can take its value from a system property.

While defaults can be overriden from system properties, it should still be the case that my app can call the right APIs and hard-wire what I want.

My 2c.

[ok2c]

@garydgregory What you described above is exactly what this change-set intends to accomplish.

I'd be willing to consider a default behavior change just for the proxy behavior.

@rschmitt This cannot be just for the proxy behavior, because there is at the very least JSSE settings that are standard and are not archaic,

http.proxyHost
http.proxyPort
https.proxyHost
https.proxyPort
http.nonProxyHosts
https.proxyUser
http.proxyUser
https.proxyPassword
http.proxyPassword

http.keepAlive
http.agent
 
ssl.TrustManagerFactory.algorithm
javax.net.ssl.trustStoreType
javax.net.ssl.trustStore
javax.net.ssl.trustStoreProvider
javax.net.ssl.trustStorePassword
ssl.KeyManagerFactory.algorithm
javax.net.ssl.keyStoreType
javax.net.ssl.keyStore
javax.net.ssl.keyStoreProvider
javax.net.ssl.keyStorePassword
https.protocols
https.cipherSuites

I am fine with dropping 'http.keepAlive' and 'http.agent' entirely and making use of all other properties listed above by default without any API changes other than deprecation of useSystemProperties method. If that works for you, it works for me, though I have to say I find this approach potentially massively more disruptive than my current one.