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 5 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
79 changes: 30 additions & 49 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,73 +1,54 @@
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.enterprise.inject.Disposes;
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,
@ConfigProperty(name = "httpClient.maxConnectionPerRoute") Integer maxConnectionPerRoute,
@ConfigProperty(name = "httpClient.requestBufferSize") Integer requestBufferSize,
@ConfigProperty(name = "httpClient.responseBufferSize") Integer responseBufferSize,
public VertxHttpClient provideHttpClient(@ConfigProperty(name = "httpClient.maxConnectionPerRoute") Integer maxConnectionPerRoute,
@ConfigProperty(name = "httpClient.maxRedirects") Integer maxRedirects,
@ConfigProperty(name = "httpClient.idleTimeoutInMillis") Integer idleTimeout,
@ConfigProperty(name = "httpClient.connectTimeoutInMillis") Integer connectTimeout,
@ConfigProperty(name = "httpClient.disableWWWAuthenticationValidation")
Boolean disableWWWAuthenticationValidation) {

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();
@ConfigProperty(name = "httpClient.connectTimeoutInMillis") Integer connectTimeout) {
Comment on lines 22 to 25
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 configuration property httpClient.maxConnectionsQueued is no longer used after migrating to Vert.x but still exists in application.properties (line 13). Similarly, httpClient.requestBufferSize, httpClient.responseBufferSize, and httpClient.disableWWWAuthenticationValidation are no longer consumed. Consider documenting which configuration properties are deprecated or removing them from the configuration file to avoid confusion.

Copilot uses AI. Check for mistakes.

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);

return new VertxHttpClient(vertx, webClientSession);
}

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()));
}
}
});
public void close(@Disposes VertxHttpClient client) {
if (client.getWebClient() != null) {
client.getWebClient().close();
}
Comment on lines 52 to 55
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 dispose method only closes the WebClientSession but doesn't close the underlying WebClient that was created on line 43. According to Vert.x documentation, both the session and the base WebClient should be closed to properly release resources. Consider also closing the base WebClient or storing a reference to it in VertxHttpClient for proper cleanup.

Copilot uses AI. Check for mistakes.
}
}
Loading
Loading