httpConfiguration silently discards setTimeout/setReadTimeout

[slachiewicz]

Affected version

3.5.3 and master — ConfigurationUtils and AbstractHttpClientWagon are byte-identical on both branches.

Bug description

Any httpConfiguration block that applies to a method discards the timeouts set through Wagon.setTimeout() and Wagon.setReadTimeout(), even when the block says nothing about timeouts.

AbstractHttpClientWagon.execute() sets them first:

requestConfigBuilder.setConnectTimeout(getTimeout());      // :855
requestConfigBuilder.setSocketTimeout(getReadTimeout());   // :856

then, eleven lines later, hands the builder to copyConfig:

ConfigurationUtils.copyConfig(config, requestConfigBuilder);   // :866

which overwrites both unconditionally, with no test for whether the user configured anything:

public static void copyConfig(HttpMethodConfiguration config, RequestConfig.Builder builder) {
    builder.setConnectTimeout(config.getConnectionTimeout());   // :61
    builder.setSocketTimeout(config.getReadTimeout());          // :62

The replacement values are HttpMethodConfiguration's field defaults. So configuring, say, a single header for PUT silently resets that method's timeouts.

This is invisible while nothing calls setTimeout/setReadTimeout, because the defaults coincide. It becomes visible as soon as something does.

Two related cases in the same code

An empty per-method block can override an explicit <all> read timeout, but only under -Dmaven.wagon.rto. The merge treats "differs from the constant default" as "explicitly set":

if (local.getReadTimeout() != Wagon.DEFAULT_READ_TIMEOUT) {   // ConfigurationUtils:159
    result.setReadTimeout(local.getReadTimeout());
}

while the field default is itself read from that property:

private int readTimeout =
        Integer.parseInt(System.getProperty("maven.wagon.rto", Integer.toString(Wagon.DEFAULT_READ_TIMEOUT)));

So with -Dmaven.wagon.rto=60000, an <all> of 600000 and a <put> block with only headers, the put inherits 60000 rather than the 600000 that was asked for. connectionTimeout has no equivalent problem, because its field default is the constant.

A per-method timeout cannot be set back to the default value, by the same comparison — writing <readTimeout>1800000</readTimeout> is indistinguishable from omitting it.

Test coverage

None, in either direction. Nothing under wagon-http-shared/src/test, wagon-http/src/test or wagon-provider-test references ConfigurationUtils, so no test locks in the current behaviour and none would catch a fix. The existing timeout tests miss it by accident: HttpWagonTimeoutTest either calls setReadTimeout with no httpConfiguration at all, or sets an explicit <all> connection timeout.

One thing not verified here

Whether maven-resolver-transport-wagon calls setTimeout/setReadTimeout from the resolver's own timeout settings — that code is outside this repository. If it does, this is reachable in an ordinary mvn deploy; if it does not, it needs an embedder that sets them. The in-repo half — that copyConfig overwrites unconditionally — stands either way.

Found while writing the HTTP configuration guide in #905. Related to #906: same file, same class of defect, different mechanism.