HttpClient#doOnRequest doesnt execute BEFORE request is sent

[simonbasle]

Expected behavior

HttpClient#doOnRequest listener should be invoked before the request is sent, allowing to mutate it (eg. add a basic set of headers on every request).

Actual behavior

It seems to be invoked AFTER the request is sent. In the listener:

• doing req.requestHeaders().set("foo", "bar") mutates the headers but is never used
• doing req.header("foo", "bar") throws an java.lang.IllegalStateException: Status and headers already sent

Steps to reproduce

@Test
public void postMutatesHeaders() {
	HttpClient client = HttpClient.create()
			.doOnRequest((req, con) -> req.header("foo", "dufafa"))
			.baseUrl("https://postman-echo.com/");

	String resp =
			client.post()
					.uri("/post")
					.send((req, outbound) -> {
						String foo = req.requestHeaders().get("foo");
						if (foo != null) {
							req.header("foo", foo+"bar");
						}
						else {
							req.header("foo", "baz");
						}
						return outbound.sendString(Flux.just("post"));
					})
					.response((res, byteBufFlux) -> byteBufFlux.asString())
					.blockFirst();

	//check what the echo server saw as headers
	Assertions.assertThat(resp).contains("\"foo\":\"dufafabar\"");
}

Analysis

HttpClientConnect is always added as the first observer, after doOnRequest's HttpClientDoOn has set itself as observer, resulting if I understand correctly in HCC calling the handler and sending the request before the doOnRequest is invoked.

Both react to the CONFIGURED state, and due to the order HttpClientConnect "wins".

The workaround would be to add a new intermediate state, eg. CONFIGURING, letting some observers alter the request before HttpClientConnect does its thing. For instance, doOnRequest would match on this one.

CONFIGURING would be fired right before CONFIGURED everywhere the later is currently fired.

Reactor Netty version

0.8.x

[crankydillo]

Hi, my project has a need for this fix. I've been allotted a small amount of time to look into this and started with this failing test.

If I revert the observer reordering from this commit, my test passes. Any thoughts on this?

I'm still new to this, so I'm not entirely sure how to proceed, but I'll do the best in the time I've got!

[crankydillo]

I should note that other tests, unsurprisingly, fail when I change that back.

If I apply this diff to my branch:

diff --git a/src/main/java/reactor/netty/http/client/HttpClientConnect.java b/src/main/java/reactor/netty/http/client/HttpClientConnect.java
index 88f764f..a8768f6 100644
--- a/src/main/java/reactor/netty/http/client/HttpClientConnect.java
+++ b/src/main/java/reactor/netty/http/client/HttpClientConnect.java
@@ -326,7 +326,7 @@ final class HttpClientConnect extends HttpClient {
 				}
 
 				BootstrapHandlers.connectionObserver(finalBootstrap,
-						new HttpObserver(sink, handler).then(BootstrapHandlers.connectionObserver(finalBootstrap)));
+					BootstrapHandlers.connectionObserver(finalBootstrap).then(new HttpObserver(sink, handler)));
 
 				tcpClient.connect(finalBootstrap)
 				         .subscribe(new TcpClientSubscriber(sink));

I get the following failing tests:

Note that my newly added hookFiringOrder test is failing as well; however, that's due to the doAfterResponse hook not firing (or not firing in time). Not sure what's going on there, but my colleague is complaining of similar behavior even after integrating the fix for #651. When I run the test in isolation, it 'always' passes. Anyhow, not trying to sidetrack the original issue.. I just wanted to explain why that test was in the image:)

[violetagg]

@crankydillo Thanks for the analysis. Please check this PR #701

[crankydillo]

Thanks @violetagg !