gnodet-bot commented on code in PR #29804:
URL: https://github.com/apache/camel/pull/29804#discussion_r4565312988
##########
dsl/camel-jbang/camel-jbang-run/src/main/java/org/apache/camel/dsl/jbang/run/common/GistHelper.java:
##########
@@ -83,11 +88,42 @@ private static void doFetchGistUrls(
static void resolveGistAsRawFiles(
String url, StringJoiner routes, StringJoiner kamelets,
StringJoiner properties, StringJoiner all)
throws Exception {
+ resolveGistAsRawFiles(url, routes, kamelets, properties, all,
Duration.ofSeconds(20));
+ }
+
+ /**
+ * Package-private overload accepting a custom timeout for testing.
+ */
+ static void resolveGistAsRawFiles(
+ String url, StringJoiner routes, StringJoiner kamelets,
StringJoiner properties, StringJoiner all,
+ Duration timeout)
+ throws Exception {
// use JDK http client to call github api
HttpClient hc =
HttpClient.newBuilder().proxy(ProxySelector.getDefault()).build();
- HttpResponse<String> res = hc.send(HttpRequest.newBuilder(new
URI(url)).timeout(Duration.ofSeconds(20)).build(),
- HttpResponse.BodyHandlers.ofString());
+ HttpRequest.Builder reqBuilder = HttpRequest.newBuilder(new
URI(url)).timeout(timeout);
+ // Send GITHUB_TOKEN as Authorization header when set, but only to
api.github.com (never leak to other hosts)
+ String token = System.getenv("GITHUB_TOKEN");
+ if (token != null && !token.isBlank() && "api.github.com".equals(new
URI(url).getHost())) {
Review Comment:
đĄ **`new URI(url)` parsed twice** â `new URI(url)` appears here and on the
`reqBuilder` line above. Extract to a local variable `URI uri = new URI(url);`
before the `reqBuilder` line and use `uri.getHost()` here. The second parse is
redundant and can itself throw `URISyntaxException`.
```suggestion
if (token != null && !token.isBlank() &&
"api.github.com".equals(uri.getHost())) {
```
(also change line 104 to use `uri` instead of `new URI(url)` â requires a
two-line suggestion block)
##########
dsl/camel-jbang/camel-jbang-run/src/main/java/org/apache/camel/dsl/jbang/run/common/GitHubHelper.java:
##########
@@ -152,11 +157,42 @@ private static void addSingleFileUrl(
static void resolveGithubAsRawFiles(
String url, String wildcard, StringJoiner routes, StringJoiner
kamelets, StringJoiner properties, StringJoiner all)
throws Exception {
+ resolveGithubAsRawFiles(url, wildcard, routes, kamelets, properties,
all, Duration.ofSeconds(20));
+ }
+
+ /**
+ * Package-private overload accepting a custom timeout for testing.
+ */
+ static void resolveGithubAsRawFiles(
+ String url, String wildcard, StringJoiner routes, StringJoiner
kamelets, StringJoiner properties, StringJoiner all,
+ Duration timeout)
+ throws Exception {
// use JDK http client to call github api
HttpClient hc =
HttpClient.newBuilder().proxy(ProxySelector.getDefault()).build();
- HttpResponse<String> res = hc.send(HttpRequest.newBuilder(new
URI(url)).timeout(Duration.ofSeconds(20)).build(),
- HttpResponse.BodyHandlers.ofString());
+ HttpRequest.Builder reqBuilder = HttpRequest.newBuilder(new
URI(url)).timeout(timeout);
+ // Send GITHUB_TOKEN as Authorization header when set, but only to
api.github.com (never leak to other hosts)
+ String token = System.getenv("GITHUB_TOKEN");
+ if (token != null && !token.isBlank() && "api.github.com".equals(new
URI(url).getHost())) {
Review Comment:
đĄ **`new URI(url)` parsed twice** â same pattern as `GistHelper.java`: `new
URI(url)` is called here and on the `reqBuilder` line above. Extract to `URI
uri = new URI(url);` before `reqBuilder` and use `uri.getHost()` here.
```suggestion
if (token != null && !token.isBlank() &&
"api.github.com".equals(uri.getHost())) {
```
##########
dsl/camel-jbang/camel-launcher/src/main/java/org/apache/camel/dsl/jbang/launcher/selfupdate/SelfUpdateCommand.java:
##########
@@ -368,10 +373,32 @@ private Integer runInstaller(String targetVersion) {
// pin-state file for this run. Set last so it can never be
dropped by the override block above.
pb.environment().put("CAMEL_INSTALL_SELF_UPDATE", "true");
Process process = pb.start();
- String output = new
String(process.getInputStream().readAllBytes(), StandardCharsets.UTF_8);
- int exit = process.waitFor();
- printer().print(output);
- return exit;
+ // Drain stdout in a separate thread to prevent the subprocess
from blocking on a full pipe buffer in
+ // the gap before waitFor() returns. Then wait with a timeout to
prevent an indefinitely stalled
+ // installer from hanging the CLI.
+ ExecutorService drainer = Executors.newSingleThreadExecutor(r -> {
+ Thread t = new Thread(r, "camel-self-update-drainer");
+ t.setDaemon(true);
+ return t;
+ });
+ Future<String> outputFuture = drainer
+ .submit(() -> new
String(process.getInputStream().readAllBytes(), StandardCharsets.UTF_8));
+ try {
+ boolean finished = process.waitFor(5, TimeUnit.MINUTES);
+ if (!finished) {
+ // Kill grandchildren first (curl/wget etc.) before the
shell, so they release the pipe
+
process.descendants().forEach(ProcessHandle::destroyForcibly);
+ process.destroyForcibly();
+ printer().printErr("installer timed out after 5 minutes");
+ return CliExitCode.TIMEOUT;
+ }
+ String output = outputFuture.get(10, TimeUnit.SECONDS);
+ int exit = process.exitValue();
+ printer().print(output);
+ return exit;
+ } finally {
+ drainer.shutdown();
Review Comment:
â ī¸ **`shutdown()` won't interrupt a blocked `readAllBytes()`** â on the
timeout path the process is killed, but the drainer thread may be stuck in
`readAllBytes()` waiting for more data. `shutdown()` waits for running tasks to
complete naturally; it does not interrupt them. Use `shutdownNow()` to
interrupt the thread.
```suggestion
drainer.shutdownNow();
```
##########
dsl/camel-jbang/camel-jbang-run/src/test/java/org/apache/camel/dsl/jbang/run/common/GitHubApiResponseTest.java:
##########
@@ -94,4 +104,95 @@ void trailingContentAndNonTextNamesAreTolerated() throws
Exception {
assertThat(all).hasToString(GitHubHelper.asGithubSingleUrl("https://github.com/me/repo/blob/main/route.java";));
}
+
+ @Test
+ void nonOkGithubResponseThrows() {
+ statusCode = 404;
+
+ assertThatThrownBy(() -> GitHubHelper.resolveGithubAsRawFiles(url,
null, null, null, null, new StringJoiner(",")))
+ .isInstanceOf(IOException.class)
+ .hasMessageContaining("404");
+ }
+
+ @Test
+ void rateLimitedGithubResponseThrowsWithHint() {
Review Comment:
đˇī¸ **Misleading test name** â this test sends HTTP 403 without an
`x-ratelimit-remaining: 0` header, so `rateLimitHint()` returns the
access-denied message, not a rate-limit message. The assertion confirms
`"access denied"`. Consider renaming to
`accessDeniedGithubResponseThrowsWithHint` and adding a separate test for the
actual rate-limit case (403 + `x-ratelimit-remaining: 0`).
##########
dsl/camel-jbang/camel-jbang-run/src/main/java/org/apache/camel/dsl/jbang/run/common/GistHelper.java:
##########
@@ -115,7 +151,35 @@ static void resolveGistAsRawFiles(
all.add(u);
}
}
+ } else if (res.statusCode() == 404) {
+ throw new FileNotFoundException("GitHub Gist API request failed
with HTTP 404 for URL: " + url);
+ } else {
+ String hint = rateLimitHint(res, token);
+ throw new IOException(
+ "GitHub Gist API request failed with HTTP " +
res.statusCode() + hint + " for URL: " + url);
+ }
+ }
+
+ /**
+ * Returns a hint string for rate-limit or access-denied errors (403/429).
+ */
+ private static String rateLimitHint(HttpResponse<String> res, String
token) {
Review Comment:
đ **`rateLimitHint()` duplicated in `GistHelper` and `GitHubHelper`** â both
classes are in the same package and contain identical implementations. Consider
extracting to a package-private utility (e.g. `GitHubHttpHelper`) to avoid
divergence.
##########
dsl/camel-jbang/camel-launcher/src/main/java/org/apache/camel/dsl/jbang/launcher/selfupdate/SelfUpdateCommand.java:
##########
@@ -368,10 +373,32 @@ private Integer runInstaller(String targetVersion) {
// pin-state file for this run. Set last so it can never be
dropped by the override block above.
pb.environment().put("CAMEL_INSTALL_SELF_UPDATE", "true");
Process process = pb.start();
- String output = new
String(process.getInputStream().readAllBytes(), StandardCharsets.UTF_8);
- int exit = process.waitFor();
- printer().print(output);
- return exit;
+ // Drain stdout in a separate thread to prevent the subprocess
from blocking on a full pipe buffer in
+ // the gap before waitFor() returns. Then wait with a timeout to
prevent an indefinitely stalled
+ // installer from hanging the CLI.
+ ExecutorService drainer = Executors.newSingleThreadExecutor(r -> {
+ Thread t = new Thread(r, "camel-self-update-drainer");
+ t.setDaemon(true);
+ return t;
+ });
+ Future<String> outputFuture = drainer
+ .submit(() -> new
String(process.getInputStream().readAllBytes(), StandardCharsets.UTF_8));
+ try {
+ boolean finished = process.waitFor(5, TimeUnit.MINUTES);
+ if (!finished) {
+ // Kill grandchildren first (curl/wget etc.) before the
shell, so they release the pipe
+
process.descendants().forEach(ProcessHandle::destroyForcibly);
+ process.destroyForcibly();
+ printer().printErr("installer timed out after 5 minutes");
+ return CliExitCode.TIMEOUT;
+ }
+ String output = outputFuture.get(10, TimeUnit.SECONDS);
Review Comment:
âšī¸ **Misleading error message on stdout-drain timeout** â
`outputFuture.get(10, TimeUnit.SECONDS)` can throw `TimeoutException` or
`ExecutionException`, which are caught by the outer `catch (Exception e)`
block. That outer handler prints `"failed to run the installer"`, but the
install may have actually succeeded (process exited 0). Consider catching these
explicitly here and printing a more accurate message like `"installer completed
but stdout drain timed out"` to avoid confusing users.
--
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]
