slachiewicz opened a new issue, #908:
URL: https://github.com/apache/maven-wagon/issues/908
### 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:
```java
requestConfigBuilder.setConnectTimeout(getTimeout()); // :855
requestConfigBuilder.setSocketTimeout(getReadTimeout()); // :856
```
then, eleven lines later, hands the builder to `copyConfig`:
```java
ConfigurationUtils.copyConfig(config, requestConfigBuilder); // :866
```
which overwrites both unconditionally, with no test for whether the user
configured anything:
```java
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":
```java
if (local.getReadTimeout() != Wagon.DEFAULT_READ_TIMEOUT) { //
ConfigurationUtils:159
result.setReadTimeout(local.getReadTimeout());
}
```
while the field default is itself read from that property:
```java
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
