gnodet-bot commented on code in PR #2206:
URL: https://github.com/apache/maven-resolver/pull/2206#discussion_r4252372226
##########
maven-resolver-transport-jetty/src/main/java/org/eclipse/aether/transport/jetty/JettyTransporter.java:
##########
@@ -404,6 +407,35 @@ protected void implPut(PutTask task) throws Exception {
})
.send(listener);
response = listener.get(requestTimeout, TimeUnit.MILLISECONDS);
+ // On some platforms (macOS) with certain configurations (proxy
auth, TLS/HTTP2),
+ // Jetty may complete a small PUT without firing onRequestContent
callbacks, leaving
+ // the progress listener uninformed even though the upload was
sent.
+ // Fire the missing events regardless of response status so the
listener contract
+ // is honoured on both success and error paths.
+ if (task.getDataLength() > 0 && !progressed.get()) {
+ if (started.compareAndSet(false, true)) {
+ try {
+ task.getListener().transportStarted(0,
task.getDataLength());
+ } catch (TransferCancelledException e) {
+ return; // upload already sent — retroactive
notification cannot be cancelled
Review Comment:
⚠️ **Silent failure on error response when listener throws
`TransferCancelledException`**
Commit 5 moved the retroactive progress block from after the status check to
before it — necessary to honour the listener contract on error paths. But this
reordering exposes a regression: if `transportStarted()` throws
`TransferCancelledException` and the server returned a 4xx/5xx, `implPut` now
returns **without throwing `HttpTransporterException`**. The caller silently
believes the PUT succeeded even though the server rejected it.
**Before commit 5:** the retroactive block ran only after a successful
status check (the `generateException` call threw for 3xx/4xx/5xx before
reaching it), so `return` on TCE was always in the 2xx case — safe.
**After commit 5:** the retroactive block runs before the status check. On a
400 Bad Request response, if the listener throws TCE from `transportStarted`:
1. `return` exits `implPut()` — no exception is thrown
2. `transportPropertiesAvailable` is skipped
3. `generateException` is never called
4. The caller receives no signal that the upload failed
Fix: instead of `return`, skip only the remainder of the retroactive block
and fall through to `transportPropertiesAvailable` + the status check:
```suggestion
} catch (TransferCancelledException e) {
// upload already sent — retroactive notification
cannot be cancelled;
// fall through to transportPropertiesAvailable and
status check
}
```
The same applies to the `break` in the `transportProgressed` loop — it
already falls through correctly (the `break` exits only the while loop, not the
method). Only the `return` here is the problem.
--
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]
