Skip to content
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.

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

Can't transfer headers to clients without reading request body (http2) #12669

Open
mpfau opened this issue Jan 3, 2025 · 3 comments
Open

Can't transfer headers to clients without reading request body (http2) #12669

mpfau opened this issue Jan 3, 2025 · 3 comments
Labels
Bug For general bugs on Jetty side

Comments

@mpfau
Copy link

mpfau commented Jan 3, 2025

Jetty version(s)
12.0.15

Jetty Environment
core

Java version/vendor (use: java -version)
21

OS type/version
Linux

Description
In some cases we don't want to read the full body of a request but still send a response code and headers back to the client. Think of a resource that is used for uploading 10MB large files that wants to throttle clients by sending 429 status code and Retry-After headers to the client.

This has been discussed on the following mailing list thread:
https://www.eclipse.org/lists/jetty-users/msg10850.html

How to reproduce?

import org.eclipse.jetty.alpn.server.ALPNServerConnectionFactory;
import org.eclipse.jetty.client.BytesRequestContent;
import org.eclipse.jetty.client.HttpClient;
import org.eclipse.jetty.http.HttpException;
import org.eclipse.jetty.http.HttpHeader;
import org.eclipse.jetty.http2.client.HTTP2Client;
import org.eclipse.jetty.http2.client.transport.HttpClientTransportOverHTTP2;
import org.eclipse.jetty.http2.server.HTTP2ServerConnectionFactory;
import org.eclipse.jetty.io.ClientConnector;
import org.eclipse.jetty.server.*;
import org.eclipse.jetty.util.Callback;
import org.eclipse.jetty.util.ssl.SslContextFactory;
import org.junit.After;
import org.junit.Before;
import org.junit.Ignore;
import org.junit.Test;

import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;

import static org.junit.Assert.assertEquals;

public class CancelRequestTest {

	private Server server;

	@Before
	public void prepareServer() throws IOException {
		var keystore = Path.of("build/keystore");
		if (!Files.exists(keystore)) {
			var process = new ProcessBuilder()
					.inheritIO()
					.command("keytool -keystore build/keystore -alias localhost -keyalg EC -genkey -validity 3650 -storepass testkey -dname CN=localhost".split(" "))
					.start();
			try {
				process.waitFor();
			} catch (InterruptedException e) {
				throw new RuntimeException(e);
			}
		}

		var cf = new SslContextFactory.Server();
		cf.setKeyStorePath(keystore.toAbsolutePath().toString());
		cf.setKeyStorePassword("testkey");
		cf.setKeyManagerPassword("testkey");

		server = new Server();
		HttpConfiguration httpsConfig = new HttpConfiguration();
		httpsConfig.setSecurePort(7004);
		ALPNServerConnectionFactory alpn = new ALPNServerConnectionFactory();
		HttpConnectionFactory http1 = new HttpConnectionFactory(httpsConfig);
		alpn.setDefaultProtocol(http1.getProtocol());
		SslConnectionFactory ssl = new SslConnectionFactory(cf, alpn.getProtocol());

		HTTP2ServerConnectionFactory http2 = new HTTP2ServerConnectionFactory(httpsConfig);

		ServerConnector connector = new ServerConnector(server, ssl, alpn, http2, http1);

		connector.setPort(7004);
		connector.setIdleTimeout(1000 * 20);
		server.addConnector(connector);
	}

	@After
	public void stopServer() throws Exception {
		if (server != null) {
			server.stop();
		}
	}

	@Test
	public void callback_succeeded_with_unconsumed_request_content() throws Exception {
		var handler = new Handler.Abstract() {

			@Override
			public boolean handle(Request request, Response response, Callback callback) throws Exception {
				response.getHeaders().put("Retry-After", "120");
				response.setStatus(429);
				callback.succeeded();
				return true;
			}
		};

		server.setHandler(handler);
		server.start();

		var sslContextFactory = new SslContextFactory.Client();
		sslContextFactory.setTrustAll(true);
		var clientConnector = new ClientConnector();
		clientConnector.setSslContextFactory(sslContextFactory);
		HTTP2Client http2Client = new HTTP2Client(clientConnector);
		HttpClientTransportOverHTTP2 transport = new HttpClientTransportOverHTTP2(http2Client);
		transport.setUseALPN(true);

		HttpClient client = new HttpClient(transport);
		client.start();

		int i = 0;
		while (true) {
			i++;
			var res = client.newRequest("https://localhost:7004")
					.method("POST")
					.body(new BytesRequestContent(new byte[1024 * 1024 * 10]))
					.send();

			assertEquals("failed on attempt: " + i, 429, res.getStatus());
			assertEquals("failed on attempt: " + i, "120", res.getHeaders().get(HttpHeader.RETRY_AFTER));
			System.out.println("successful attempt " + i);
		}
	}

	@Test
	/**
	 * When invoked with the following curl command, 429 is returned as status but the Retry-After header is not sent to the client.
	 *
	 * curl -k -v https://localhost:7004 -F "data=/tmp/100m.bin"
	 */
	public void callback_failed() throws Exception {
		var handler = new Handler.Abstract() {

			@Override
			public boolean handle(Request request, Response response, Callback callback) throws Exception {
				response.getHeaders().put("Retry-After", "120");
				response.setStatus(429);
				callback.failed(new TooManyRequestsException());
				return true;
			}
		};

		server.setHandler(handler);
		server.start();

		var sslContextFactory = new SslContextFactory.Client();
		sslContextFactory.setTrustAll(true);
		var clientConnector = new ClientConnector();
		clientConnector.setSslContextFactory(sslContextFactory);
		HTTP2Client http2Client = new HTTP2Client(clientConnector);
		HttpClientTransportOverHTTP2 transport = new HttpClientTransportOverHTTP2(http2Client);
		transport.setUseALPN(true);

		HttpClient client = new HttpClient(transport);
		client.start();

		int i = 0;
		while (true) {
			i++;
			var res = client.newRequest("https://localhost:7004")
					.method("POST")
					.body(new BytesRequestContent(new byte[1024 * 1024 * 10]))
					.send();

			assertEquals("failed on attempt: " + i, 429, res.getStatus());
			assertEquals("failed on attempt: " + i, "120", res.getHeaders().get(HttpHeader.RETRY_AFTER));
			System.out.println("successful attempt " + i);
		}
	}


	class TooManyRequestsException extends RuntimeException implements HttpException {

		@Override
		public int getCode() {
			return 429;
		}

		@Override
		public String getReason() {
			return "reason";
		}
	}

}
@mpfau mpfau added the Bug For general bugs on Jetty side label Jan 3, 2025
@mpfau
Copy link
Author

mpfau commented Jan 21, 2025

Hey @sbordet @gregw,
could you please confirm if this is an issue or if we did something wrong?

Thanks!

Best,
Matthias

@joakime
Copy link
Contributor

joakime commented Jan 21, 2025

callback.failed(new TooManyRequestsException());

In this code path, you don't have a ErrorHandler processing that Exception into what you want the response to look like.
So the default error handling is executing, nothing you did in the Response is kept, as that Response is uncommitted at the point in time that you failed with the TooManyRequestsException.

@sbordet
Copy link
Contributor

sbordet commented Jan 21, 2025

@mpfau you need to:

On the server, call callback.succeeded() on the Handler's Callback parameter.

On the client, the response will arrive, but likely the request will fail to upload, causing the whole exchange to fail, so client.newRequest(..)...send() will likely throw.

You need to use the asynchronous APIs, and use send(result -> {}) where you can inspect result.getResponse() to get the 429 and the Retry-After header, and organize yourself a retry using the HttpClient.scheduler.
The Result itself will be failed due to a request failure, but the response should be ok.

If you send response content with the 429, it is best if you intercept the onResponseHeaders(response -> {}) event, and organize the retry from there, because the response content may be dropped and cause a response failure as well (although the response headers will still be available).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side
Projects
None yet
Development

No branches or pull requests

3 participants