Skip to content

Migrate jetty-client to vertx-web-client #387

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
google-labs-jules wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Migrate jetty-client to vertx-web-client #387

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5584bb6
Migrate jetty-client to vertx-web-client
google-labs-jules[bot] Nov 21, 2025
f7562b1
Migrate jetty-client to vertx-web-client
google-labs-jules[bot] Nov 22, 2025
547e7b6
Remove unused disableWWWAuthenticationValidation parameter
google-labs-jules[bot] Nov 22, 2025
db69e2c
Migrate from Jetty to Vert.x WebClient with proper error handling and…
google-labs-jules[bot] Nov 22, 2025
fe097eb
Address review feedback for Vert.x WebClient migration
google-labs-jules[bot] Nov 22, 2025
253edb0
Address final review feedback for Vert.x WebClient migration
google-labs-jules[bot] Nov 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,9 +267,8 @@
<version>3.3.4</version>
</dependency>
<dependency>
<groupId>org.eclipse.jetty</groupId>
<artifactId>jetty-client</artifactId>
<version>${jetty.version}</version>
<groupId>io.vertx</groupId>
<artifactId>vertx-web-client</artifactId>
</dependency>
<dependency>
<groupId>jakarta.transaction</groupId>
Expand Down
4 changes: 0 additions & 4 deletions src/main/java/ai/labs/eddi/engine/httpclient/IHttpClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
package ai.labs.eddi.engine.httpclient;

import org.eclipse.jetty.http.HttpCookieStore;

import java.net.URI;

public interface IHttpClient {
Expand All @@ -14,8 +12,6 @@ enum Method {
PATCH
}

HttpCookieStore getCookieStore();

IRequest newRequest(URI uri);

IRequest newRequest(URI uri, Method method);
Expand Down
67 changes: 23 additions & 44 deletions src/main/java/ai/labs/eddi/engine/httpclient/bootstrap/HttpClientModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,25 +1,24 @@
package ai.labs.eddi.engine.httpclient.bootstrap;

import ai.labs.eddi.engine.httpclient.impl.JettyHttpClient;
import ai.labs.eddi.engine.httpclient.impl.VertxHttpClient;
import io.vertx.core.Vertx;
import io.vertx.ext.web.client.WebClient;
import io.vertx.ext.web.client.WebClientSession;
import io.vertx.ext.web.client.WebClientOptions;
import jakarta.enterprise.context.ApplicationScoped;
import jakarta.inject.Inject;
import jakarta.ws.rs.Produces;
import org.eclipse.jetty.client.HttpClient;
import org.eclipse.jetty.client.WWWAuthenticationProtocolHandler;
import org.eclipse.microprofile.config.inject.ConfigProperty;
import org.eclipse.microprofile.context.ManagedExecutor;

import java.util.Arrays;

@ApplicationScoped
public class HttpClientModule {

@Inject
ManagedExecutor executorService;
Vertx vertx;
Copy link

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Vertx instance is injected but Quarkus provides Vert.x through the quarkus-vertx extension. You need to add the Quarkus Vert.x dependency to pom.xml for this injection to work:

<dependency>
    <groupId>io.quarkus</groupId>
    <artifactId>quarkus-vertx</artifactId>
</dependency>

Without this dependency, the Vertx injection will fail at runtime.

Copilot uses AI. Check for mistakes.

@Produces
@ApplicationScoped
public JettyHttpClient provideHttpClient(@ConfigProperty(name = "httpClient.maxConnectionsQueued") Integer maxConnectionsQueued,
public VertxHttpClient provideHttpClient(@ConfigProperty(name = "httpClient.maxConnectionsQueued") Integer maxConnectionsQueued,
@ConfigProperty(name = "httpClient.maxConnectionPerRoute") Integer maxConnectionPerRoute,
Copy link

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The maxConnectionsQueued configuration parameter is read but never used. The comment on line 34 notes that there's no direct equivalent in Vert.x WebClientOptions, but the parameter should either be removed from the method signature or a comment should be added explaining why it's not applicable.

This could confuse users who configure this parameter expecting it to have an effect.

Suggested change
public VertxHttpClient provideHttpClient(@ConfigProperty(name = "httpClient.maxConnectionsQueued") Integer maxConnectionsQueued,
@ConfigProperty(name = "httpClient.maxConnectionPerRoute") Integer maxConnectionPerRoute,
public VertxHttpClient provideHttpClient(@ConfigProperty(name = "httpClient.maxConnectionPerRoute") Integer maxConnectionPerRoute,

Copilot uses AI. Check for mistakes.
@ConfigProperty(name = "httpClient.requestBufferSize") Integer requestBufferSize,
@ConfigProperty(name = "httpClient.responseBufferSize") Integer responseBufferSize,
Copy link

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, requestBufferSize and responseBufferSize parameters are read but never used. While the comments on lines 37-40 explain that Vert.x handles buffers dynamically, these unused parameters should either be removed from the method signature or explicitly documented as no longer applicable.

Users configuring these parameters will expect them to have an effect on buffer sizes.

Suggested change
@ConfigProperty(name = "httpClient.requestBufferSize") Integer requestBufferSize,
@ConfigProperty(name = "httpClient.responseBufferSize") Integer responseBufferSize,

Copilot uses AI. Check for mistakes.
Expand All @@ -29,45 +28,25 @@ public JettyHttpClient provideHttpClient(@ConfigProperty(name = "httpClient.maxC
@ConfigProperty(name = "httpClient.disableWWWAuthenticationValidation")
Boolean disableWWWAuthenticationValidation) {
Copy link

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The disableWWWAuthenticationValidation parameter is read but never used. The comment on lines 53-54 mentions that Vert.x handles auth differently, but this parameter should either be removed or its lack of effect should be clearly documented.

This could affect authentication behavior if users were relying on this configuration.

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Nov 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The disableWWWAuthenticationValidation parameter is accepted but never used. In the Jetty implementation, this was used to remove the WWWAuthenticationProtocolHandler. This parameter should either be removed or the functionality should be implemented if it's needed.

Copilot uses AI. Check for mistakes.

try {
HttpClient httpClient = new HttpClient();
httpClient.setExecutor(executorService);
httpClient.setMaxConnectionsPerDestination(maxConnectionsQueued);
httpClient.setMaxRequestsQueuedPerDestination(maxConnectionPerRoute);
httpClient.setRequestBufferSize(requestBufferSize);
httpClient.setResponseBufferSize(responseBufferSize);
httpClient.setMaxRedirects(maxRedirects);
httpClient.setIdleTimeout(idleTimeout);
httpClient.setConnectTimeout(connectTimeout);
httpClient.start();

if (disableWWWAuthenticationValidation) {
httpClient.getProtocolHandlers().remove(WWWAuthenticationProtocolHandler.NAME);
}
WebClientOptions options = new WebClientOptions();

registerHttpClientShutdownHook(httpClient);
// Mapping configuration
options.setMaxPoolSize(maxConnectionPerRoute);

return new JettyHttpClient(httpClient);
} catch (Exception e) {
System.out.println(Arrays.toString(e.getStackTrace()));
throw new RuntimeException(e.getLocalizedMessage(), e);
options.setMaxRedirects(maxRedirects);
int idleTimeoutSeconds = idleTimeout / 1000;
if (idleTimeout > 0 && idleTimeoutSeconds == 0) {
Copy link

Copilot AI Nov 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idleTimeout conversion logic may lose precision for timeout values between 1 and 999 milliseconds. For example, an idleTimeout of 500ms would be converted to 0 seconds, then adjusted to 1 second (doubling the intended timeout). Consider documenting this behavior or using a more precise conversion that preserves the millisecond granularity if Vert.x supports it, or at least warning users about this limitation in configuration documentation.

Suggested change
options.setMaxRedirects(maxRedirects);
int idleTimeoutSeconds = idleTimeout / 1000;
if (idleTimeout > 0 && idleTimeoutSeconds == 0) {
options.setMaxRedirects(maxRedirects);
// Vert.x WebClientOptions.setIdleTimeout only supports seconds granularity.
// Any idleTimeout value between 1 and 999 milliseconds will be rounded up to 1 second.
int idleTimeoutSeconds = idleTimeout / 1000;
if (idleTimeout > 0 && idleTimeoutSeconds == 0) {
// Warn users about rounding up sub-second timeouts
System.out.println("Warning: httpClient.idleTimeoutInMillis (" + idleTimeout + " ms) is less than 1000 ms and will be rounded up to 1 second due to Vert.x limitation.");

Copilot uses AI. Check for mistakes.
idleTimeoutSeconds = 1;
Copy link

Copilot AI Nov 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idle timeout conversion from milliseconds to seconds has potential issues. When idleTimeout is between 1 and 999 milliseconds, the result will be 0, which is then changed to 1 second on line 35. However, when idleTimeout is exactly 0, it bypasses the check and sets the timeout to 0 seconds. This inconsistency could cause unexpected behavior. Consider handling the conversion more consistently: if the input is > 0 but < 1000, round up to 1 second; if it's 0, keep it as 0 (which typically means no timeout).

Suggested change
int idleTimeoutSeconds = idleTimeout / 1000;
if (idleTimeout > 0 && idleTimeoutSeconds == 0) {
idleTimeoutSeconds = 1;
int idleTimeoutSeconds;
if (idleTimeout == 0) {
idleTimeoutSeconds = 0; // No timeout
} else if (idleTimeout > 0 && idleTimeout < 1000) {
idleTimeoutSeconds = 1; // Round up to 1 second
} else {
idleTimeoutSeconds = (int) Math.ceil(idleTimeout / 1000.0); // Round up to nearest second

Copilot uses AI. Check for mistakes.
}
Copy link

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The timeout conversion from milliseconds to seconds using integer division can result in a timeout of 0 seconds (no timeout) for values less than 1000ms. For example, if idleTimeout is 500ms, idleTimeoutSeconds will be 0, which in Vert.x means "no timeout" rather than 500ms.

Consider using TimeUnit conversions or rounding up:

int idleTimeoutSeconds = (int) Math.ceil(idleTimeout / 1000.0);

to ensure timeouts under 1 second are still applied as 1 second rather than being disabled.

Suggested change
int idleTimeoutSeconds = idleTimeout / 1000;
if (idleTimeout > 0 && idleTimeoutSeconds == 0) {
idleTimeoutSeconds = 1;
}
int idleTimeoutSeconds = (int) Math.ceil(idleTimeout / 1000.0);

Copilot uses AI. Check for mistakes.
}
options.setIdleTimeout(idleTimeoutSeconds);

options.setConnectTimeout(connectTimeout);
options.setFollowRedirects(true);
options.setDecompressionSupported(true);

WebClient webClient = WebClient.create(vertx, options);
WebClientSession webClientSession = WebClientSession.create(webClient);

private void registerHttpClientShutdownHook(final HttpClient httpClient) {
Runtime.getRuntime().addShutdownHook(new Thread("ShutdownHook_HttpClient") {
@Override
public void run() {
try {
if (!httpClient.isStopped()) {
httpClient.stop();
}
} catch (Throwable e) {
String message = "HttpClient did not stop as expected.";
System.out.println(message);
System.out.println(Arrays.toString(e.getStackTrace()));
}
}
});
return new VertxHttpClient(vertx, webClientSession);
}
Comment on lines 46 to 50
Copy link

Copilot AI Nov 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous Jetty implementation registered a shutdown hook to properly stop the HttpClient on application shutdown. The new implementation doesn't include any cleanup code for the WebClient or WebClientSession.

While Vert.x resources are typically managed by the Vert.x instance itself, it's good practice to explicitly close the WebClient and WebClientSession when the application shuts down to prevent potential resource leaks. Consider adding a @PreDestroy method or observing Quarkus shutdown events to call webClientSession.close() and webClient.close().

Copilot uses AI. Check for mistakes.
}
Loading
Loading